See comments. Updated patch coming within the next hour.
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/client/DomBuilderFactory.java
File user/src/com/google/gwt/dom/builder/client/DomBuilderFactory.java
(right):
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/client/DomBuilderFactory.java#newcode57
user/src/com/google/gwt/dom/builder/client/DomBuilderFactory.java:57:
public DivElementBuilder createDivElementBuilder() {
On 2011/06/23 22:46:24, skybrian wrote:
"createDivBuilder"?
Done.
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/client/DomBuilderImpl.java
File user/src/com/google/gwt/dom/builder/client/DomBuilderImpl.java
(right):
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/client/DomBuilderImpl.java#newcode145
user/src/com/google/gwt/dom/builder/client/DomBuilderImpl.java:145:
Element getCurrentElement() {
On 2011/06/23 22:46:24, skybrian wrote:
It's unlikely to matter, but perhaps more efficient to keep the
current element
in a field, so we don't access the list here. Then it's just a field
access and
can be inlined.
List<Element> parentStack = new ArrayList<Element>();
Element currentElement;
Done.
Except that "parentStack" is still called "stackElements" and contains
the currentElement. It simplifies the code slightly to push the current
element onto the stack.
We use two additional stacks in the parent class ElementBuilderImpl to
maintain the tag names and builders, but we only access them on
start/end, so we don't need the same operation.
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/DivElementBuilder.java
File user/src/com/google/gwt/dom/builder/shared/DivElementBuilder.java
(right):
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/DivElementBuilder.java#newcode21
user/src/com/google/gwt/dom/builder/shared/DivElementBuilder.java:21:
public interface DivElementBuilder extends
ElementBuilderBase<DivElementBuilder> {
On 2011/06/23 22:46:24, skybrian wrote:
"DivBuilder"?
Done.
I changed other class names as well to them sound better. The new
hierarchy is:
ElementBuilderBase
ElementBuilder
DomElementBuilder
HtmlElementBuilder
DivBuilder
DomDivBuilder
HtmlDivBuilder
OptionBuilder
DomOptionBuilder
HtmlOptionBuilder
SelectBuilder
DomSelectBuilder
HtmlSelectBuilder
I renamed "StyleBuilder" to "StylesBuilder" to leave a slot for
StyleBuilder, which builds the style element.
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java
File user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java
(right):
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java#newcode40
user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java:40:
Element asElement();
On 2011/06/23 22:46:24, skybrian wrote:
Perhaps "finishElement()" or just "finish()" since "as" implies it's
just a
cast.
Done.
finish() seems reasonable.
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java#newcode47
user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java:47: *
@return this builder
On 2011/06/23 22:46:24, skybrian wrote:
I'd remove the @param since they don't actually tell us anything.
Done.
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java#newcode83
user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java:83: *
@param draggable a String constants
On 2011/06/23 22:46:24, skybrian wrote:
"constant".
Done.
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java#newcode106
user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java:106:
<B extends ElementBuilderBase<?>> B end(Class<B> clazz);
On 2011/06/23 22:46:24, skybrian wrote:
I'm wondering if we need the variations on end() that take a class. In
particular, this looks like a mismatch:
out.startDiv().startH1().text("Heading").end(DivBuilder.class)
It also doubles the number of end() method variants.
Though it's ugly, perhaps this would be better:
out.startDiv()
.startH1().text("Heading").end().backTo(DivBuilder.class)
.text("Some text")
.startH2().text("Hello").endH2()
.end()
Although, my personal preference is to not chain so many methods.
Perhaps:
I agree, but I don't want to preclude users from chaining methods if
they want to.
out.startDiv();
out.asDiv().startH1().text("hello").endH1();
out.asDiv().text("hello");
out.asDiv().startH2().text("hello").endH2();
out.endDiv();
If you squint at it right, the "asDiv()" call looks sort of like an
indent.
Or perhaps:
DivBuilder __div = out.asDiv();
out.startDiv();
__div.startH1().text("Heading").end();
__div.text("Some text");
__div.startH2().text("Hello").end();
out.end();
Normally I'd be wary of mixing up local variables, but since they all
point to
the same instance, it seems fairly harmless.
We could add an ElementBuilderBase.as(Class<?>) method that casts the
instance to the specific class. The endXXX() methods would return
ElementBuilderBase (which they currently do if the user doesn't specify
a class parameter). The user can then cast it to a specific subtype if
needed.
There is precedent for as() method. Element.as() will cast one element
type to another, although Elements are JSOs, so its always safe to
switch between them. In our case, we rely on Java to throw a
ClassCastException.
Alternatively, we can define end() using a parameterized return type:
<E extends ElementBuilderBase<?>> E end();
By default, end() returns an ElementBuilderBase, but users can
parameterize the return type. The advantage of this approach is that
you don't need an additional call to "as()" or "backTo", and it drops
the ".class", so its slightly less verbose.
// Returns ElementBuilderBase
out.startDiv().startH1().text("Heading").end();
// Returns DivBuilder.
out.startDiv().startH1().text("Heading").<DivBuilder>end()
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java#newcode269
user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java:269:
* Once you append text to the element, you can no longer set attributes.
On 2011/06/23 22:46:24, skybrian wrote:
Perhaps say that the text will be escaped.
Done.
Noted that its implementation specific. The HTML implementation escapes
the text, but the DOM implementation doesn't need to because there is a
native method to set innerText on an element.
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderImpl.java
File user/src/com/google/gwt/dom/builder/shared/ElementBuilderImpl.java
(right):
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderImpl.java#newcode54
user/src/com/google/gwt/dom/builder/shared/ElementBuilderImpl.java:54:
/**
On 2011/06/23 22:46:24, skybrian wrote:
oops
Done.
Auto-format didn't like the <> symbols, so I escaped them.
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/HtmlBuilderImpl.java
File user/src/com/google/gwt/dom/builder/shared/HtmlBuilderImpl.java
(right):
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/HtmlBuilderImpl.java#newcode160
user/src/com/google/gwt/dom/builder/shared/HtmlBuilderImpl.java:160:
sb.append("<").append(escape(tagName));
On 2011/06/23 22:46:24, skybrian wrote:
Perhaps have trustedStart() to use internally, which skips escaping
the tag.
Actually, escaping it seems like the wrong thing; really we want to
reject it if
it needs escaping.
I think it would be difficult to know for sure when we need escaping. I
tracked down the w3c syntax for element tag names:
http://www.w3.org/TR/xml11/#NT-Name
For now, I suggest that we go with only the "trusted" method. We can
add a version that does syntax checking in the future, but I think it
would be very unusual to use a non-trusted string as a tagName.
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/StyleBuilder.java
File user/src/com/google/gwt/dom/builder/shared/StyleBuilder.java
(right):
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/StyleBuilder.java#newcode228
user/src/com/google/gwt/dom/builder/shared/StyleBuilder.java:228: * Set
the trusted background color, i.e., without escaping the value. No
On 2011/06/23 22:46:24, skybrian wrote:
Possible rewording to be a bit less boilerplatey:
Appends the equivalent of "background-color:{value};" to the style
tag's
contents. Does not check or escape the color string. The calling code
should be
carefully reviewed to ensure that the provided color string won't
cause a
security issue if appended to innerHtml. For details, see {link}.
Also, the last paragraph of the boilerplate was garbled a bit using
copy-and-paste. I'd suggest putting it in one place and linking to it.
Done.
I used the term "sets" instead of "appends" because the DOM version sets
the style directly on the Element. Only the HTML version appends the
style property to a string.
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/StyleBuilder.java#newcode243
user/src/com/google/gwt/dom/builder/shared/StyleBuilder.java:243: *
@return a SafeStylescom.google.gwt.safecss.shared.SafeStyles instance
On 2011/06/23 22:46:24, skybrian wrote:
oops
Done.
http://gwt-code-reviews.appspot.com/1455802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
