[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)

2011-03-29 Thread jlabanca 

I think the entire attribute state could be useful, and I think thats a
good reason to use state booleans instead of more enums.  The number of
enums could grow exponentially (CSS_ATTRIBUTE_START_ENTIRE?).

Since this is implementation code and we can change it as needed, I'll
LGTM if you want to submit this version.  We can refactor later if
needed.


On 2011/03/29 18:49:04, xtof wrote:

FWIW, I think I will also want to add another state that indicates

that the

template variable comprises the entire attribute, for URLs (i.e. img
src={0}, but not img src={0}/{1}).  So there will be at least

one more

state coming.



On Tue, Mar 29, 2011 at 11:37, Christoph Kern mailto:x...@google.com

wrote:


 I could do that. One concern I have though with this approach is

that the

 state exposed in HtmlContext would get more complicated.  Right now,

code

 using the parser can simply do a switch statement over the contexts

(see

 SafeHtmlTemplatesImplMethodCreator#emitParameterExpression); if we

make the

 state multi-dimensional, client code would likely get more

complicated.


 What do you think?


 On Tue, Mar 29, 2011 at 11:26, mailto:jlaba...@google.com wrote:

 Knowing that you are at the start of a context type seems generally
 useful for all context types, including TEXT/ATTRIBUTE/URL (we

already

 have URL_START).

 Instead of adding CSS_ATTRIBUTE_START, would it be better to add
 HtmlContext#isStart(), or event HtmlContext#getValueIndex()?


 http://gwt-code-reviews.appspot.com/1392801/





http://gwt-code-reviews.appspot.com/1392801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)

2011-03-29 Thread xtof 

http://gwt-code-reviews.appspot.com/1392801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)

2011-03-29 Thread jlabanca 

LGTM

I don't actually see the TODO though.  You can add it before submitting.

On 2011/03/29 19:28:17, xtof wrote:

On Tue, Mar 29, 2011 at 12:14, mailto:jlaba...@google.com wrote:



 I think the entire attribute state could be useful, and I think

thats a

 good reason to use state booleans instead of more enums.  The number

of

 enums could grow exponentially (CSS_ATTRIBUTE_START_ENTIRE?).

Right.




 Since this is implementation code and we can change it as needed,

I'll

 LGTM if you want to submit this version.  We can refactor later if
 needed.

 Sounds good.  I added TODO(xtof): Consider refactoring such that

state

related to the position of the template variable in an attribute is

exposed

separately (as HtmlContext#isAttributeStart(), etc). In doing so,

consider

trade off between combinatorial explosion of possible states vs

complexity

of client code.



Sound reasonable?





 On 2011/03/29 18:49:04, xtof wrote:

 FWIW, I think I will also want to add another state that indicates

 that the

 template variable comprises the entire attribute, for URLs (i.e.

img

 src={0}, but not img src={0}/{1}).  So there will be at least

 one more

 state coming.


  On Tue, Mar 29, 2011 at 11:37, Christoph Kern

mailto:x...@google.com


 wrote:

   I could do that. One concern I have though with this approach is

 that the

  state exposed in HtmlContext would get more complicated.  Right

now,


 code

  using the parser can simply do a switch statement over the

contexts


 (see

  SafeHtmlTemplatesImplMethodCreator#emitParameterExpression); if

we


 make the

  state multi-dimensional, client code would likely get more

 complicated.

 
  What do you think?

 
 
  On Tue, Mar 29, 2011 at 11:26, mailto:jlaba...@google.com

wrote:

 
  Knowing that you are at the start of a context type seems

generally

  useful for all context types, including TEXT/ATTRIBUTE/URL (we

 already

  have URL_START).
 
  Instead of adding CSS_ATTRIBUTE_START, would it be better to add
  HtmlContext#isStart(), or event HtmlContext#getValueIndex()?
 
 
  http://gwt-code-reviews.appspot.com/1392801/
 
 
 


 http://gwt-code-reviews.appspot.com/1392801/





http://gwt-code-reviews.appspot.com/1392801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)

2011-03-29 Thread Christoph Kern 
On Tue, Mar 29, 2011 at 12:14, jlaba...@google.com wrote:

 I think the entire attribute state could be useful, and I think thats a
 good reason to use state booleans instead of more enums.  The number of
 enums could grow exponentially (CSS_ATTRIBUTE_START_ENTIRE?).

Right.


 Since this is implementation code and we can change it as needed, I'll
 LGTM if you want to submit this version.  We can refactor later if
 needed.

 Sounds good.  I added TODO(xtof): Consider refactoring such that state
related to the position of the template variable in an attribute is exposed
separately (as HtmlContext#isAttributeStart(), etc). In doing so, consider
trade off between combinatorial explosion of possible states vs complexity
of client code.

Sound reasonable?



 On 2011/03/29 18:49:04, xtof wrote:

 FWIW, I think I will also want to add another state that indicates

 that the

 template variable comprises the entire attribute, for URLs (i.e. img
 src={0}, but not img src={0}/{1}).  So there will be at least

 one more

 state coming.


  On Tue, Mar 29, 2011 at 11:37, Christoph Kern mailto:x...@google.com

 wrote:

   I could do that. One concern I have though with this approach is

 that the

  state exposed in HtmlContext would get more complicated.  Right now,

 code

  using the parser can simply do a switch statement over the contexts

 (see

  SafeHtmlTemplatesImplMethodCreator#emitParameterExpression); if we

 make the

  state multi-dimensional, client code would likely get more

 complicated.

 
  What do you think?

 
 
  On Tue, Mar 29, 2011 at 11:26, mailto:jlaba...@google.com wrote:
 
  Knowing that you are at the start of a context type seems generally
  useful for all context types, including TEXT/ATTRIBUTE/URL (we

 already

  have URL_START).
 
  Instead of adding CSS_ATTRIBUTE_START, would it be better to add
  HtmlContext#isStart(), or event HtmlContext#getValueIndex()?
 
 
  http://gwt-code-reviews.appspot.com/1392801/
 
 
 


 http://gwt-code-reviews.appspot.com/1392801/


-- 
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)

2011-03-29 Thread xtof 

On 2011/03/29 19:30:16, jlabanca wrote:

LGTM



I don't actually see the TODO though.  You can add it before

submitting.

Oops, forgot to export git before uploading.  Done.

Thanks!
--xtof

http://gwt-code-reviews.appspot.com/1392801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)

2011-03-29 Thread xtof 

http://gwt-code-reviews.appspot.com/1392801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors