I addressed all of your comments in this latest patch.
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/24 23:32:16, skybrian wrote:
On 2011/06/24 16:48:18, jlabanca wrote:
> 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
That's general XML and I'm not sure if it makes sense for HTML; do we
care about
the ability to generate unknown HTML tags? Does that even work? I have
never
seen tag name escaping in HTML and I'm not sure if that works either.
(There
would be interesting security implications if it did; for example you
could hide
script tags from naive scanners that look for "<script".)
Since this is a new API and we're generating, not parsing, we could be
relatively restrictive here and just allow [a-z][a-z0-9]+. I believe
that covers
all known HTML5 tags. If there's some odd use-case we can relax the
restriction
later.
Agreed. I added the Regex check "^[a-z][a-z0-9]*$" to match a-z plus 0
or more alphanumeric characters (case insensitive).
Or we could just not expose any way to create arbitrary tags, and then
it's
irrelevant.
New elements are added periodically. I don't want to create a whitelist
that will need to be updated. We haven't even created all of the
Element types yet for the Element APIs.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/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/8002/user/src/com/google/gwt/dom/builder/client/DomBuilderImpl.java#newcode154
user/src/com/google/gwt/dom/builder/client/DomBuilderImpl.java:154: if
(stackElements.isEmpty()) {
On 2011/06/24 23:32:16, skybrian wrote:
It looks like we have the invariant that currentElement is null when
the stack
is empty, so these isEmpty() checks could be replaced by null checks
(if it
matters).
Done.
isEmpty is just a integer comparison, but the null check is fine too.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/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/8002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java#newcode76
user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java:76: *
@throws ClassCastException if the parent builder does not match the
On 2011/06/24 23:32:16, skybrian wrote:
This needs a bit more explanation. The end() method isn't really
throwing the
exception, is it?
The ClassCastException is thrown by Java, but I think its important to
call it out so users know that they can only cast to the builder type of
the parent element.
I also think we need to give an example of the casting trick in the
Javadoc,
since it's a fairly obscure part of the Java language.
Done.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java#newcode175
user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java:175:
DivBuilder startDiv();
On 2011/06/24 23:32:16, skybrian wrote:
SelectBuilder and OptionBuilder are descendants of this interface, but
this
method doesn't make sense for them. (Do we care? Future CL?)
I didn't want to be overly restrictive. There are some combinations
that seem invalid, but work in practice, such as putting a div in a
table element. Also, each startXXX() method is valid for most elements,
but not all, which makes it difficult to create a hierarchy. We would
end up with a bunch of containsDiv/containsWhatever interfaces to
describe how each element works.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java#newcode192
user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java:192:
* Start the style element.
On 2011/06/24 23:32:16, skybrian wrote:
Did you mean the style attribute?
Done.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java#newcode214
user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java:214:
* String based implementation will escape the text to prevent
HTML/javascript
On 2011/06/24 23:32:16, skybrian wrote:
"A string-based implementation"
Done.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderFactory.java
File
user/src/com/google/gwt/dom/builder/shared/ElementBuilderFactory.java
(right):
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderFactory.java#newcode133
user/src/com/google/gwt/dom/builder/shared/ElementBuilderFactory.java:133:
public abstract ElementBuilder
createElementBuilderFromTrustedTagName(String tagName);
On 2011/06/24 23:32:16, skybrian wrote:
On 2011/06/24 17:40:01, jlabanca wrote:
> Does this method name work? createTrustedElementBuilder(String
tagName)
sounds
> like the return value is trusted rather than the parameter tagName.
Hopefully
> it won't be used very often, as we'll provide builders for most
elements.
What do you think about trustedCreate("tag")? We seem to have a
convention going
of putting "trusted" first in the names of method calls to be checked,
and that
seems like enough of a heads-up to look at the javadoc.
Done.
I like that. Its clear and concise.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/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/8002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderImpl.java#newcode88
user/src/com/google/gwt/dom/builder/shared/ElementBuilderImpl.java:88:
doEndTagImpl(tagName);
On 2011/06/24 23:32:16, skybrian wrote:
It looks like this could be trustedEnd()
All of the methods that need to be overridden are named doXXXImpl() to
make it clear that the subclass can just perform the task assuming the
necessary checks have been done. trustedStart() calls into
ElementBuilderImpl#onStart(), but ElementBuilderImpl calls out to
doEndTagImpl(). trustedStart() and doEndTagImpl() are not analogous to
each other.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderImpl.java#newcode177
user/src/com/google/gwt/dom/builder/shared/ElementBuilderImpl.java:177:
* Get the {@link StylesBuilder} used to add style properties.
On 2011/06/24 23:32:16, skybrian wrote:
"used to add style properties to the current element."
Done.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/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/8002/user/src/com/google/gwt/dom/builder/shared/HtmlBuilderImpl.java#newcode119
user/src/com/google/gwt/dom/builder/shared/HtmlBuilderImpl.java:119:
sb.append("</").append(escape(tagName)).append(">");
On 2011/06/24 23:32:16, skybrian wrote:
Another escape() here. Should this be trustedEnd()?
Done.
I should have removed the escape() call when I removed it from
onStart(). The tagName is safe because it comes from the stack of tag
names, and tag names are checked before adding them to the stack. I
added a comment to that end.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/HtmlElementBuilderBase.java
File
user/src/com/google/gwt/dom/builder/shared/HtmlElementBuilderBase.java
(right):
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/HtmlElementBuilderBase.java#newcode87
user/src/com/google/gwt/dom/builder/shared/HtmlElementBuilderBase.java:87:
return (B) delegate.end();
On 2011/06/24 23:32:16, skybrian wrote:
I'm wondering what this cast actually does after type erasure.
You get a ClassCastException if you reference the returned object. If
you don't reference the returned object, its ignored, which is safe
anyway.
This does not generate an error because the return value is ignored:
ElementBuilderFactory.get().createDivBuilder().startDiv().<SelectBuilder>
end();
This does:
ElementBuilderFactory.get().createDivBuilder().startDiv().<SelectBuilder>
end().id("test");
And so does this:
SelectBuilder select =
ElementBuilderFactory.get().createDivBuilder().startDiv().<SelectBuilder>
end();
In GWT, I think there is an experimental compiler option to disable
class cast checks (to reduce code size), but the description explains
that you'll miss these errors.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/HtmlStylesBuilder.java
File user/src/com/google/gwt/dom/builder/shared/HtmlStylesBuilder.java
(right):
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/HtmlStylesBuilder.java#newcode43
user/src/com/google/gwt/dom/builder/shared/HtmlStylesBuilder.java:43: *
Regex to match a camelCase word.
On 2011/06/24 23:32:16, skybrian wrote:
A bit unclear what this is supposed to match. Examples?
Done.
I clarified that the regex matches words within a camelCase phrase. I
also added a note that it doesn't validate the name; SafeSylesUtils does
that.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/HtmlStylesBuilder.java#newcode298
user/src/com/google/gwt/dom/builder/shared/HtmlStylesBuilder.java:298:
private String fromCamelCase(String name) {
On 2011/06/24 23:32:16, skybrian wrote:
There will be a limited number of inputs. I wonder if we should cache
this?
Done.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/OptionBuilder.java
File user/src/com/google/gwt/dom/builder/shared/OptionBuilder.java
(right):
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/OptionBuilder.java#newcode35
user/src/com/google/gwt/dom/builder/shared/OptionBuilder.java:35: * The
control is unavailable in this context.
On 2011/06/24 23:32:16, skybrian wrote:
"Prevents the user from selecting this option."
Done.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/OptionBuilder.java#newcode44
user/src/com/google/gwt/dom/builder/shared/OptionBuilder.java:44: *
Option label for use in hierarchical menus.
On 2011/06/24 23:32:16, skybrian wrote:
remove "hierarchical"? Does it also get used in simple dropdown menus?
Done.
It only gets used in drop down menus. I copied these JavaDoc from
OptionElement, which copied them from the w3c specs. We added dozens of
Element types at the same time, so some of the method descriptions
probably weren't screened.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java
File user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java
(right):
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java#newcode35
user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java:35: *
Builds the style object.
On 2011/06/24 23:32:16, skybrian wrote:
Needs a bit of explanation - which styles are we setting?
Perhaps: "Builds the style attribute on an element, or sets the
element's styles
directly."
Done.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java#newcode40
user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java:40: *
StyleBuilder uses camelCase names to be consistent with Style.
Regardless
On 2011/06/24 23:32:16, skybrian wrote:
"StyleBuilder methods take camelCase property names to be consistent"
Done.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java#newcode41
user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java:41: * of
whether we use camcelCase of hyphenated, one of the implementations
On 2011/06/24 23:32:16, skybrian wrote:
sp: "camelCase or hyphenated"
Done.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java#newcode233
user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java:233: *
@param value the background color
On 2011/06/24 23:32:16, skybrian wrote:
@param seems redundant?
Done.
http://gwt-code-reviews.appspot.com/1455802/diff/8002/user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java#newcode308
user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java:308: *
@param name the camelCase name
On 2011/06/24 23:32:16, skybrian wrote:
I was curious, so I looked at what jQuery does. It takes both formats
and
converts to camelCase internally. It looks like they can convert from
dashes to
camelCase in a single string.replace call.
https://github.com/jquery/jquery/blob/master/src/core.js
(Line 596.)
Modified so we now accept either camelCase of hyphenated form.
The jsquery string.replace call takes a function as a second argument
that is executed for each match. There is no equivalent in GWT, so I'm
using RegExp to accomplish the same. There is a regex pattern that
means "toUppercase", but JavaScript doesn't support it.
http://gwt-code-reviews.appspot.com/1455802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
