Refactor a simpler consistent api for constructing parse trees

ParserContext is a parse tree node builder which accumulates inputs
and builds a parse tree node with the information it has.

This change just introduces the builder and some tests.  A separate
CL will replace the many variations of creating PTNs with this api.

[ihab.awad, 2010-10-20 21:15:56]

Looks like a great direction! Not sure what level of comments you wanted -- I
did both high and low level to be sure. :)

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
File src/com/google/caja/parser/ParserContext.java (right):

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:73: public
ParserContext(MessageQueue mq) {
Why not make 'mq' part of the Builder guffery too?

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:78: public
ParserContext(MessageQueue mq, InputSource is, CharProducer cp,
Given that you have the Builder interface, I think this should be private.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:94: public ParserContext
with(ContentType type) {
I think a bunch of "with(T)" for some type T, where T determines the function of
the particular "with", is fine, but I'm afraid of the need for a new function
"withSomething(S)" if we discover we need another "with" function for
"Something" where S == T for some previously defined T.

What would this API look like if at least some of the with-es where turned into
withSomething-ses? Like perhaps "withInput(T)" for T in { CharProducer,
InputSource }?

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:107: return this.cp!= cp ?
space around operator

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:125: return new ParserContext(mq,
is, cp, type, charset,
So this clobbers "this.inputStream"? Perhaps throw if this.inputStream != null?

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:147: public ParserContext
withSourceMap(Map<InputSource, CharSequence> sourceMap) {
So the "sourceMap" is not built by the parser as it fetches resources? Should
there not be some API for a "resource fetcher" thingey (PluginMeta already has
the necessary scaffolding...)?

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:153: private InputSource
guessInputSource(InputSource is) {
Since you are supplying the inputs to the "guess*" functions explicitly (which I
think is a good idea), perhaps they should be static to clarify this?

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:161: * Fetches content from an
input source, optionally storing it in sourceMap
I don't see where it stuffs stuff into "sourceMap".

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:169: throws IOException {
I'm confused about why 'sourceMap' is not used in case where all you have is an
InputSource and where the caller has already supplied a sourceMap with the
requisite thingey stuffed in it.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:192: throw new
IllegalStateException("Not enough arguments to create a CharProducer");
Long line.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:259: throw new
IllegalStateException("Required " + name.getSimpleName() + " not provided");
Long line.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:318: 
Extra line.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/plugin/Pl...
File src/com/google/caja/plugin/PluginCompilerMain.java (right):

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/plugin/Pl...
src/com/google/caja/plugin/PluginCompilerMain.java:232:
.withSourceMap(originalSources)
Are you expecting the build() method to *populate* "originalSources" as a side
effect of the parsing? It was not clear to me from reading ParserContext how
that would happen.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/plugin/Pl...
src/com/google/caja/plugin/PluginCompilerMain.java:233: .build();
Nifty.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/plugin/Pl...
src/com/google/caja/plugin/PluginCompilerMain.java:382: private Map<InputSource,
CharSequence> buildOriginalInputCharSequences()
Should you not remove this as well then?

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/service/H...
File src/com/google/caja/service/HtmlHandler.java (right):

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/service/H...
src/com/google/caja/service/HtmlHandler.java:140: .build();
Nifty.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/service/J...
File src/com/google/caja/service/JsHandler.java (right):

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/service/J...
src/com/google/caja/service/JsHandler.java:109: .build();
Nifty.

http://codereview.appspot.com/2470041/diff/3001/tests/com/google/caja/parser/...
File tests/com/google/caja/parser/ParserContextTest.java (right):

http://codereview.appspot.com/2470041/diff/3001/tests/com/google/caja/parser/...
tests/com/google/caja/parser/ParserContextTest.java:1: // Copyright (C) 2008
Google Inc.
Copyright date

http://codereview.appspot.com/2470041/diff/3001/tests/com/google/caja/parser/...
tests/com/google/caja/parser/ParserContextTest.java:23: import
com.google.appengine.repackaged.com.google.common.collect.Maps;
Is this really where you want to get Maps from?

http://codereview.appspot.com/2470041/diff/3001/tests/com/google/caja/parser/...
tests/com/google/caja/parser/ParserContextTest.java:47: } catch (Throwable e) {
The following:

  } catch (IllegalStateException e) {
    return;
  }
  fail();

would also work. See comment re "throws Exception" below.

http://codereview.appspot.com/2470041/diff/3001/tests/com/google/caja/parser/...
tests/com/google/caja/parser/ParserContextTest.java:58:
.withSourceMap(sourceMap)
4 space continuation indent

http://codereview.appspot.com/2470041/diff/3001/tests/com/google/caja/parser/...
tests/com/google/caja/parser/ParserContextTest.java:64: } catch (Throwable e) {
Can just declare the test method "throws Exception" and let JUnit fail your test
if it throws.

http://codereview.appspot.com/2470041/diff/3001/tests/com/google/caja/parser/...
tests/com/google/caja/parser/ParserContextTest.java:108:
"cssparserinput1.css").toURL().openStream();
Continuation indents.

http://codereview.appspot.com/2470041/diff/3001/tests/com/google/caja/parser/...
tests/com/google/caja/parser/ParserContextTest.java:110:
"amazon.com.html").toURL().openStream();
Can we call this something more generic? example.com.html or whatever?

[Jasvir, 2010-10-26 00:04:32]

Thanks for the comments - its exactly the kind of feedback I was looking for.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
File src/com/google/caja/parser/ParserContext.java (right):

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:73: public
ParserContext(MessageQueue mq) {
Because I'd like the message queue to be required.

On 2010/10/20 21:15:56, ihab.awad wrote:
> Why not make 'mq' part of the Builder guffery too?

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:78: public
ParserContext(MessageQueue mq, InputSource is, CharProducer cp,
On 2010/10/20 21:15:56, ihab.awad wrote:
> Given that you have the Builder interface, I think this should be private.

Done.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:94: public ParserContext
with(ContentType type) {
The argument to "with" is usually descriptive and it makes the with(something)
read better than withSomething(something).  I can change this to group certain
inputs to minimize the likelihood of collision.

On 2010/10/20 21:15:56, ihab.awad wrote:
> I think a bunch of "with(T)" for some type T, where T determines the function
of
> the particular "with", is fine, but I'm afraid of the need for a new function
> "withSomething(S)" if we discover we need another "with" function for
> "Something" where S == T for some previously defined T.
> 
> What would this API look like if at least some of the with-es where turned
into
> withSomething-ses? Like perhaps "withInput(T)" for T in { CharProducer,
> InputSource }?

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:107: return this.cp!= cp ?
On 2010/10/20 21:15:56, ihab.awad wrote:
> space around operator

Done.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:147: public ParserContext
withSourceMap(Map<InputSource, CharSequence> sourceMap) {
The only fetch the parser should make is if its provided a reference to the
input but not the content.  Rewired to use the resource fetcher from the plugin
meta that's provided. 

On 2010/10/20 21:15:56, ihab.awad wrote:
> So the "sourceMap" is not built by the parser as it fetches resources? Should
> there not be some API for a "resource fetcher" thingey (PluginMeta already has
> the necessary scaffolding...)?

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:153: private InputSource
guessInputSource(InputSource is) {
On 2010/10/20 21:15:56, ihab.awad wrote:
> Since you are supplying the inputs to the "guess*" functions explicitly (which
I
> think is a good idea), perhaps they should be static to clarify this?

Done.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:161: * Fetches content from an
input source, optionally storing it in sourceMap
On 2010/10/20 21:15:56, ihab.awad wrote:
> I don't see where it stuffs stuff into "sourceMap".

Removed from comment. Done.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:169: throws IOException {
Added

On 2010/10/20 21:15:56, ihab.awad wrote:
> I'm confused about why 'sourceMap' is not used in case where all you have is
an
> InputSource and where the caller has already supplied a sourceMap with the
> requisite thingey stuffed in it.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:192: throw new
IllegalStateException("Not enough arguments to create a CharProducer");
On 2010/10/20 21:15:56, ihab.awad wrote:
> Long line.

Done.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:259: throw new
IllegalStateException("Required " + name.getSimpleName() + " not provided");
On 2010/10/20 21:15:56, ihab.awad wrote:
> Long line.

Done.

http://codereview.appspot.com/2470041/diff/3001/src/com/google/caja/parser/Pa...
src/com/google/caja/parser/ParserContext.java:318: 
On 2010/10/20 21:15:56, ihab.awad wrote:
> Extra line.

Done.

[ihab.awad, 2011-03-14 22:49:35]

I think the with() methods should be grouped, as you noted in your earlier
reply, into stuff like "withInput(...)" for a variety of types of inputs, etc.

Various minor issues called out in-place.

Otherwise, LGTM++

http://codereview.appspot.com/2470041/diff/13001/src/com/google/caja/parser/P...
File src/com/google/caja/parser/ParserContext.java (right):

http://codereview.appspot.com/2470041/diff/13001/src/com/google/caja/parser/P...
src/com/google/caja/parser/ParserContext.java:108: public ParserContext
with(InputSource is) {
Why should InputSource be specified independently of some input that it
describes? Like maybe with(String content) should be with(InputSource is, String
content) instead?

http://codereview.appspot.com/2470041/diff/13001/src/com/google/caja/parser/P...
src/com/google/caja/parser/ParserContext.java:218: private static CharProducer
guessCharProducer(InputSource is, PluginMeta meta) {
Long line.

http://codereview.appspot.com/2470041/diff/13001/src/com/google/caja/parser/P...
src/com/google/caja/parser/ParserContext.java:318: cp = guessCharProducer(cp,
content, sourceMap, meta, is, inputStream, charset);
Long line.

http://codereview.appspot.com/2470041/diff/13001/src/com/google/caja/plugin/P...
File src/com/google/caja/plugin/PluginCompilerMain.java (right):

http://codereview.appspot.com/2470041/diff/13001/src/com/google/caja/plugin/P...
src/com/google/caja/plugin/PluginCompilerMain.java:232:
.withSourceMap(originalSources)
'originalSources' is never populated! What gives?

http://codereview.appspot.com/2470041/diff/13001/tests/com/google/caja/parser...
File tests/com/google/caja/parser/ParserContextTest.java (right):

http://codereview.appspot.com/2470041/diff/13001/tests/com/google/caja/parser...
tests/com/google/caja/parser/ParserContextTest.java:1: // Copyright (C) 2008
Google Inc.
Is this correct?

http://codereview.appspot.com/2470041/diff/13001/tests/com/google/caja/parser...
tests/com/google/caja/parser/ParserContextTest.java:21: import
com.google.appengine.repackaged.com.google.common.collect.Maps;
Is this correct?

http://codereview.appspot.com/2470041/diff/13001/tests/com/google/caja/parser...
tests/com/google/caja/parser/ParserContextTest.java:62: } catch (Throwable e) {
Can just declare test function "throws Exception" and be done with it.

http://codereview.appspot.com/2470041/diff/13001/tests/com/google/caja/parser...
tests/com/google/caja/parser/ParserContextTest.java:106:
"cssparserinput1.css").toURL().openStream();
Indentation

[Jasvir, 2011-03-19 20:31:04]

Updated patch to use withInputs, withConfig to group options.

Submitted @4398

http://codereview.appspot.com/2470041/diff/13001/src/com/google/caja/parser/P...
File src/com/google/caja/parser/ParserContext.java (right):

http://codereview.appspot.com/2470041/diff/13001/src/com/google/caja/parser/P...
src/com/google/caja/parser/ParserContext.java:108: public ParserContext
with(InputSource is) {
Because an InputSource might be all that is provided instead of rather than in
addition to textual content.

On 2011/03/14 22:49:35, ihab.awad wrote:
> Why should InputSource be specified independently of some input that it
> describes? Like maybe with(String content) should be with(InputSource is,
String
> content) instead?

http://codereview.appspot.com/2470041/diff/13001/tests/com/google/caja/parser...
File tests/com/google/caja/parser/ParserContextTest.java (right):

http://codereview.appspot.com/2470041/diff/13001/tests/com/google/caja/parser...
tests/com/google/caja/parser/ParserContextTest.java:1: // Copyright (C) 2008
Google Inc.
On 2011/03/14 22:49:35, ihab.awad wrote:
> Is this correct?

Done.

http://codereview.appspot.com/2470041/diff/13001/tests/com/google/caja/parser...
tests/com/google/caja/parser/ParserContextTest.java:21: import
com.google.appengine.repackaged.com.google.common.collect.Maps;
On 2011/03/14 22:49:35, ihab.awad wrote:
> Is this correct?

Done.

http://codereview.appspot.com/2470041/diff/13001/tests/com/google/caja/parser...
tests/com/google/caja/parser/ParserContextTest.java:62: } catch (Throwable e) {
On 2011/03/14 22:49:35, ihab.awad wrote:
> Can just declare test function "throws Exception" and be done with it.

Done.

http://codereview.appspot.com/2470041/diff/13001/tests/com/google/caja/parser...
tests/com/google/caja/parser/ParserContextTest.java:106:
"cssparserinput1.css").toURL().openStream();
On 2011/03/14 22:49:35, ihab.awad wrote:
> Indentation

Done.