First rev of Safe DOM

First rev of Safe DOM

BUG=
R=jacobr@google.com, jmesserly@google.com

Committed: https://code.google.com/p/dart/source/detail?r=26681

[blois, 2013-06-05 00:58:24]

This is still a ways away from getting in (primarily need the MDV sanitization
plan to be settled so we can have one shared impl), but I'm hoping that the
changes are not drastic.

I believe that the primary outstanding items are around handling of namespaced
attributes which is still unresolved for MDV and I'm hoping to follow for this.

Would be great to get feedback on API usability and defaults, can discuss in
detail if it'd help.

[Jacob, 2013-06-05 23:42:50]

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dartium/html...
File sdk/lib/html/dartium/html_dartium.dart (right):

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dartium/html...
sdk/lib/html/dartium/html_dartium.dart:7312: +    {NodeValidator validator,
NodeTreeSanitizer treeSanitizer}) {
it would be interesting to see how much this slows down WebUI as it relies on
html fragments a lot

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dartium/html...
sdk/lib/html/dartium/html_dartium.dart:9298: +    var doc =
document.implementation.createHtmlDocument('');
creating a document every time seems painfully slow

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dartium/html...
sdk/lib/html/dartium/html_dartium.dart:9307: +    if
(Range.supportsCreateContextualFragment) {
nice... I was just writing a comment on the previous method suggesting using
this API

https://codereview.chromium.org/16374007/diff/13001/tools/dom/scripts/htmlren...
File tools/dom/scripts/htmlrenamer.py (right):

https://codereview.chromium.org/16374007/diff/13001/tools/dom/scripts/htmlren...
tools/dom/scripts/htmlrenamer.py:328: 'Element.innerHTML':
'deprecatedInnerHtml',
why not just use $dom_ as for the other deprecated methods?

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Validators....
File tools/dom/src/Validators.dart (right):

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Validators....
tools/dom/src/Validators.dart:100: return
_hiddenAnchor.href.startsWith(window.location.origin);
I think a more robust solution is:

final AnchorElement _hiddenAnchor = new AnchorElement()
final Location _loc = window.location;

bool allowsUri(String uri) {
  _hiddenAnchor.href = uri;
  return hiddenAnchor.hostname == _loc.hostname
    && _hiddenAnchor.port = loc.port
    && _hiddenAnchor.protocol == loc.protocol;
}

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Validators....
tools/dom/src/Validators.dart:130: case Node.ELEMENT_NODE:
what if the element has a shadow DOM?

[Jennifer Messerly, 2013-06-06 05:55:53]

This is nice! I like the API design of the validators.

My main concern is this seems to be colliding with web components/MDV. Happy to
chat in person.

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
File sdk/lib/html/dart2js/html_dart2js.dart (left):

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:8961: static const _CUSTOM_PARENT_TAG_MAP
= const {
awesome to see this go!

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:7459: * Creates an HTML element from a
valid fragment of HTML.
is this still true given restrictions around table etc?

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:7465: * data. Without strict data
validation it is impossible to prevent script
update doc comment?

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:7473: factory Element.html(String html,
add context argument? that way it can still be used for tables, etc.

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:7477: return fragment.nodes.where((e) =>
e is Element).single;
singleWhere ?

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:8177: }
add error condition for case where validator is unused?

else if (validator != null) {
  throw new ArgumentError('validator can only be passed if treeSanitizer is
null');
}

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:26078: this.uriPolicy = uriPolicy != null
? uriPolicy : new UriPolicy() {
style, I think ":" goes on next line, and "this." can be omitted.

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dartium/html...
File sdk/lib/html/dartium/html_dartium.dart (right):

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dartium/html...
sdk/lib/html/dartium/html_dartium.dart:7312: +    {NodeValidator validator,
NodeTreeSanitizer treeSanitizer}) {
On 2013/06/05 23:42:50, Jacob wrote:
> it would be interesting to see how much this slows down WebUI as it relies on
> html fragments a lot

For what it's worth, I think we'll move away from this in Web UI. As things move
to being more runtime based, we'll just be cloning templates that were either 1)
in the HTML page so everything is parsed by HTML parser (deploy time), or 2) by
XHR loading the <link rel=import> tags (development time).

The HTML Imports polyfill does use innerHTML (see
https://github.com/Polymer/HTMLImports/blob/stable/src/HTMLImports.js#L101) but
since we can (and probably need to) write that code in JavaScript I think we'll
be okay on that concern.

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Html5NodeVa...
File tools/dom/src/Html5NodeValidator.dart (right):

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Html5NodeVa...
tools/dom/src/Html5NodeValidator.dart:18: class _Html5NodeValidator implements
NodeValidator {
out of curiosity, how much size does this add?

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Html5NodeVa...
tools/dom/src/Html5NodeValidator.dart:107: 'TEXTAREA',
why no TEMPLATE?
it's an HTML5 element.

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Html5NodeVa...
tools/dom/src/Html5NodeValidator.dart:424: return
_allowedElements.contains(element.tagName);
I worry about the lack of custom element support here. Rejecting them from
innerHTML, by default, is a pretty Big Deal, given how much effort has been made
to support precisely this scenario.

I'm not sure how to fix it. The whole notion of a whitelist of
elements/attributes is flawed, because new stuff is being added to the DOM all
the time by the browsers, the standards folks, and now the application
developer.

There are a few big bugs in HTML, such as JavaScript execution and not enforcing
same-origin for some URIs. Ideally we could target the bugged stuff directly,
instead of trying to express the much bigger set of non-bugged things. CSP does
this, for example.

On the other hand, we can't expect every custom element on the web to be secure;
it may choose to read its attribute and copy it into an <img src=>, for example.
And we can't know this at validation time because that <img> tag is inside the
component's ShadowRoot.

Ultimately these bugs are caused by treating user/attacker controlled data as if
it was application developer controlled data. One of the things I liked about
SafeHtml was it provided a way for the developer to say "I wrote this HTML and
it contains no user-provided strings, please treat it as if I constructed this
via DOM calls." Is there anything like SafeHtml in this design?

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/NodeValidat...
File tools/dom/src/NodeValidatorBuilder.dart (right):

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/NodeValidat...
tools/dom/src/NodeValidatorBuilder.dart:123: var tagNameUpper =
tagName.toUpperCase();
validate that the tagName is actually a custom element name?
https://github.com/dart-lang/web-ui/blob/master/lib/src/html5_utils.dart#L21

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Validators....
File tools/dom/src/Validators.dart (right):

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Validators....
tools/dom/src/Validators.dart:130: case Node.ELEMENT_NODE:
On 2013/06/05 23:42:50, Jacob wrote:
> what if the element has a shadow DOM?

good question. I think right now you cannot expose this, because ShadowRoot
isn't itself expressible in HTML, it can only be constructed via DOM calls.

https://codereview.chromium.org/16374007/diff/13001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_DocumentFragment.darttemplate (right):

https://codereview.chromium.org/16374007/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_DocumentFragment.darttemplate:14: validator:
validator, treeSanitizer: treeSanitizer);
+2 spaces?

[Jennifer Messerly, 2013-06-06 06:50:34]

On 2013/06/06 05:55:53, John Messerly wrote:
> This is nice! I like the API design of the validators.
> 
> My main concern is this seems to be colliding with web components/MDV. Happy
to
> chat in person.

also, btw, no need to hold up the CL on the existential questions :). The code
here looks very nice.

[blois, 2013-06-06 16:59:42]

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:7459: * Creates an HTML element from a
valid fragment of HTML.
On 2013/06/06 05:55:53, John Messerly wrote:
> is this still true given restrictions around table etc?

Done.

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:7465: * data. Without strict data
validation it is impossible to prevent script
On 2013/06/06 05:55:53, John Messerly wrote:
> update doc comment?

Done. Doc comments in general for this CL need a bit of polish before
submitting.

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:7473: factory Element.html(String html,
On 2013/06/06 05:55:53, John Messerly wrote:
> add context argument? that way it can still be used for tables, etc.

I was intending that table.createFragment(...) would be used instead.

Could add another function which is table.parseElement(...) which returns a
single Element?

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:7477: return fragment.nodes.where((e) =>
e is Element).single;
On 2013/06/06 05:55:53, John Messerly wrote:
> singleWhere ?

I did this to avoid the case (which seems somewhat common) of leading and
trailing whitespace:

var html = '''
<div>
  content
</div>
''';

In this case there will be two text nodes in addition to the element.

Would prefer to document this for usability, thoughts?

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:8177: }
On 2013/06/06 05:55:53, John Messerly wrote:
> add error condition for case where validator is unused?
> 
> else if (validator != null) {
>   throw new ArgumentError('validator can only be passed if treeSanitizer is
> null');
> }

Done.

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:26078: this.uriPolicy = uriPolicy != null
? uriPolicy : new UriPolicy() {
On 2013/06/06 05:55:53, John Messerly wrote:
> style, I think ":" goes on next line, and "this." can be omitted.

Done.

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dartium/html...
File sdk/lib/html/dartium/html_dartium.dart (right):

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dartium/html...
sdk/lib/html/dartium/html_dartium.dart:7312: +    {NodeValidator validator,
NodeTreeSanitizer treeSanitizer}) {
On 2013/06/06 05:55:53, John Messerly wrote:
> On 2013/06/05 23:42:50, Jacob wrote:
> > it would be interesting to see how much this slows down WebUI as it relies
on
> > html fragments a lot
> 
> For what it's worth, I think we'll move away from this in Web UI. As things
move
> to being more runtime based, we'll just be cloning templates that were either
1)
> in the HTML page so everything is parsed by HTML parser (deploy time), or 2)
by
> XHR loading the <link rel=import> tags (development time).
> 
> The HTML Imports polyfill does use innerHTML (see
> https://github.com/Polymer/HTMLImports/blob/stable/src/HTMLImports.js#L101)
but
> since we can (and probably need to) write that code in JavaScript I think
we'll
> be okay on that concern.

In general I think that there will still be a decent amount of usage of
innerHtml and I'd like it to have minimal overhead. I think not caching the
document for parsing is probably the largest overhead, but will do some
benchmarks.

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dartium/html...
sdk/lib/html/dartium/html_dartium.dart:9298: +    var doc =
document.implementation.createHtmlDocument('');
On 2013/06/05 23:42:50, Jacob wrote:
> creating a document every time seems painfully slow

Hoping not painfully slow, but yes, this should be cached.

https://codereview.chromium.org/16374007/diff/13001/tools/dom/scripts/htmlren...
File tools/dom/scripts/htmlrenamer.py (right):

https://codereview.chromium.org/16374007/diff/13001/tools/dom/scripts/htmlren...
tools/dom/scripts/htmlrenamer.py:328: 'Element.innerHTML':
'deprecatedInnerHtml',
On 2013/06/05 23:42:50, Jacob wrote:
> why not just use $dom_ as for the other deprecated methods?

Done.

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Html5NodeVa...
File tools/dom/src/Html5NodeValidator.dart (right):

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Html5NodeVa...
tools/dom/src/Html5NodeValidator.dart:18: class _Html5NodeValidator implements
NodeValidator {
On 2013/06/06 05:55:53, John Messerly wrote:
> out of curiosity, how much size does this add?

Appears to be ~4k unzipped to Swarm.

FWIW, I originally looked at using Angular's whitelist:
https://github.com/angular/angular.js/blob/master/src/ngSanitize/sanitize.js#...

I ran into a number of issues and in the end decided it was insufficient. We can
definitely do something where this one is optional in a separate package and the
default is more minimal.

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Html5NodeVa...
tools/dom/src/Html5NodeValidator.dart:107: 'TEXTAREA',
On 2013/06/06 05:55:53, John Messerly wrote:
> why no TEMPLATE?
> it's an HTML5 element.

Will split this out into a separate CL. This file is auto-generated from the
Caja whitelists and it's not in those whitelists (but there's a mechanism for
overrides).

I'll add the tool to auto-generate this file and this file as a separate CL.

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Html5NodeVa...
tools/dom/src/Html5NodeValidator.dart:424: return
_allowedElements.contains(element.tagName);
On 2013/06/06 05:55:53, John Messerly wrote:
> I worry about the lack of custom element support here. Rejecting them from
> innerHTML, by default, is a pretty Big Deal, given how much effort has been
made
> to support precisely this scenario.
> 
> I'm not sure how to fix it. The whole notion of a whitelist of
> elements/attributes is flawed, because new stuff is being added to the DOM all
> the time by the browsers, the standards folks, and now the application
> developer.
> 
> There are a few big bugs in HTML, such as JavaScript execution and not
enforcing
> same-origin for some URIs. Ideally we could target the bugged stuff directly,
> instead of trying to express the much bigger set of non-bugged things. CSP
does
> this, for example.
> 
> On the other hand, we can't expect every custom element on the web to be
secure;
> it may choose to read its attribute and copy it into an <img src=>, for
example.
> And we can't know this at validation time because that <img> tag is inside the
> component's ShadowRoot.
> 
> Ultimately these bugs are caused by treating user/attacker controlled data as
if
> it was application developer controlled data. One of the things I liked about
> SafeHtml was it provided a way for the developer to say "I wrote this HTML and
> it contains no user-provided strings, please treat it as if I constructed this
> via DOM calls." Is there anything like SafeHtml in this design?

See NodeValidationBuilder- that's the primary API for users to specify what
should and should not be accepted. This has methods to override the defaults.

This class is intended to capture the core DOM APIs from the Caja whitelists.

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/NodeValidat...
File tools/dom/src/NodeValidatorBuilder.dart (right):

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/NodeValidat...
tools/dom/src/NodeValidatorBuilder.dart:123: var tagNameUpper =
tagName.toUpperCase();
On 2013/06/06 05:55:53, John Messerly wrote:
> validate that the tagName is actually a custom element name?
> https://github.com/dart-lang/web-ui/blob/master/lib/src/html5_utils.dart#L21

As-is this method works for allowing additional tags which may not be normally
allowed (you could use it to allow a <script> tag if desired). This seems
useful- could either add the validation and split out to a separate method,
change the name or leave as is?

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Validators....
File tools/dom/src/Validators.dart (right):

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Validators....
tools/dom/src/Validators.dart:100: return
_hiddenAnchor.href.startsWith(window.location.origin);
On 2013/06/05 23:42:50, Jacob wrote:
> I think a more robust solution is:
> 
> final AnchorElement _hiddenAnchor = new AnchorElement()
> final Location _loc = window.location;
> 
> bool allowsUri(String uri) {
>   _hiddenAnchor.href = uri;
>   return hiddenAnchor.hostname == _loc.hostname
>     && _hiddenAnchor.port = loc.port
>     && _hiddenAnchor.protocol == loc.protocol;
> }
> 

Done.

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Validators....
tools/dom/src/Validators.dart:130: case Node.ELEMENT_NODE:
On 2013/06/06 05:55:53, John Messerly wrote:
> On 2013/06/05 23:42:50, Jacob wrote:
> > what if the element has a shadow DOM?
> 
> good question. I think right now you cannot expose this, because ShadowRoot
> isn't itself expressible in HTML, it can only be constructed via DOM calls.

One similar thing that I was just looking into was Template elements-
<template>
  <script></script>
</template>

puts the script into a DocumentFragment in the contents property.

Added support & tests for this.

https://codereview.chromium.org/16374007/diff/13001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_DocumentFragment.darttemplate (right):

https://codereview.chromium.org/16374007/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_DocumentFragment.darttemplate:14: validator:
validator, treeSanitizer: treeSanitizer);
On 2013/06/06 05:55:53, John Messerly wrote:
> +2 spaces?

Done.

[Jacob, 2013-06-06 17:04:06]

lgtm

[Jennifer Messerly, 2013-06-06 17:42:45]

https://codereview.chromium.org/16374007/diff/23001/tests/html/safe_dom_test....
File tests/html/safe_dom_test.dart (right):

https://codereview.chromium.org/16374007/diff/23001/tests/html/safe_dom_test....
tests/html/safe_dom_test.dart:77: contextElement.deprecatedInnerHtml = html;
$dom?

[Jennifer Messerly, 2013-06-06 19:31:32]

lgtm -- just a few minor things :)

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/16374007/diff/13001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:7477: return fragment.nodes.where((e) =>
e is Element).single;
On 2013/06/06 16:59:42, blois wrote:
> On 2013/06/06 05:55:53, John Messerly wrote:
> > singleWhere ?
> 
> I did this to avoid the case (which seems somewhat common) of leading and
> trailing whitespace:
> 
> var html = '''
> <div>
>   content
> </div>
> ''';
> 
> In this case there will be two text nodes in addition to the element.
> 
> Would prefer to document this for usability, thoughts?

oh, I was just thinking these two are equivalent:

fragment.nodes.where((e) => e is Element).single;
fragment.nodes.singleWhere((e) => e is Element);

It looks like "singleWhere" only throws if two are matching
(sdk/lib/collection/list.dart)

Granted, it only saves one character, but it does save an allocation of the
WhereIterator.

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Html5NodeVa...
File tools/dom/src/Html5NodeValidator.dart (right):

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/Html5NodeVa...
tools/dom/src/Html5NodeValidator.dart:107: 'TEXTAREA',
On 2013/06/06 16:59:42, blois wrote:
> On 2013/06/06 05:55:53, John Messerly wrote:
> > why no TEMPLATE?
> > it's an HTML5 element.
> 
> Will split this out into a separate CL. This file is auto-generated from the
> Caja whitelists and it's not in those whitelists (but there's a mechanism for
> overrides).
> 
> I'll add the tool to auto-generate this file and this file as a separate CL.

btw, we chatted about this. I was confused; we don't need template in this
validator because the default validator set allows <template> and its
attributes.

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/NodeValidat...
File tools/dom/src/NodeValidatorBuilder.dart (right):

https://codereview.chromium.org/16374007/diff/13001/tools/dom/src/NodeValidat...
tools/dom/src/NodeValidatorBuilder.dart:123: var tagNameUpper =
tagName.toUpperCase();
On 2013/06/06 16:59:42, blois wrote:
> On 2013/06/06 05:55:53, John Messerly wrote:
> > validate that the tagName is actually a custom element name?
> > https://github.com/dart-lang/web-ui/blob/master/lib/src/html5_utils.dart#L21
> 
> As-is this method works for allowing additional tags which may not be normally
> allowed (you could use it to allow a <script> tag if desired). This seems
> useful- could either add the validation and split out to a separate method,
> change the name or leave as is?

Ah good point. Yeah that's a nice use case. Maybe name this method
"allowElement"?
Or add a comment that it works for any element name. Not a big deal either way

https://codereview.chromium.org/16374007/diff/23001/tools/dom/src/Validators....
File tools/dom/src/Validators.dart (right):

https://codereview.chromium.org/16374007/diff/23001/tools/dom/src/Validators....
tools/dom/src/Validators.dart:159: if (element is TemplateElement) {
I don't think it matters because usually you're sanitizing a just-parsed tree,
but just to be safe, it might be worth using:

element.isTemplate

That way, if the browser doesn't have native template support and they used
UnknownElement, TableRowElement, etc, it will still traverse the content.

(The "polyfilled" templates lift their child nodes into "content" automatically
when certain properties are accessed. See:
http://api.dartlang.org/docs/releases/latest/dart_html/Element.html#content and
http://api.dartlang.org/docs/releases/latest/dart_html/Element.html#isTemplate)

[blois, 2013-08-09 21:31:46]

I had this on the back burner while we figured out our versioning story (it's
the largest divergence from the DOM that we've done), but we have the basic
versioning contract settled and Lars is quite interested in mitigating XSS, so
refreshing it.

I added a bit more documentation & added NodeValidator.throws to make it easier
to get exceptions rather than silent sanitization. Other than that, just updated
for latest bits.

If you have a chance, please take another glance.

Thanks!

[Jennifer Messerly, 2013-08-17 06:07:07]

this is going to be awesome!

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/src/Html...
File tools/dom/src/Html5NodeValidator.dart (right):

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/src/Html...
tools/dom/src/Html5NodeValidator.dart:16: * *
https://code.google.com/p/google-caja/wiki/CajaWhitelists
this is internal API but, would be nice to use markdown angle brackets for the
link: < >

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/src/Node...
File tools/dom/src/NodeValidatorBuilder.dart (right):

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/src/Node...
tools/dom/src/NodeValidatorBuilder.dart:52: factory
NodeValidatorBuilder.common() {
as a factory constructor, this is hard to use in a subclass. Perhaps:

NodeValidatorBuilder.common() {
  allowHtml5();
  allowTemplating();
}

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/template...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/template...
tools/dom/templates/html/impl/impl_Element.darttemplate:334: return
fragment.nodes.where((e) => e is Element).single;
how's the performance of this? I dunno if it matters, but it seems there are
some pretty easy micro-opts here avoiding the closure and "nodes" allocations.

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/template...
tools/dom/templates/html/impl/impl_Element.darttemplate:1208: validator = new
NodeValidatorBuilder.common();
I wonder about performance here, in particular if we should be able to create
these with const constructors to avoid allocations.

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/template...
tools/dom/templates/html/impl/impl_Element.darttemplate:1215: return
_ElementFactoryProvider._parseHtml(this, html, treeSanitizer);
out of curiosity, why is this method on _ElementFactoryProvider?
Also, which element of the factory provides an abstraction for the provider for
the factory of the elements?

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/template...
tools/dom/templates/html/impl/impl_Element.darttemplate:1226: String get
innerHtml => $dom_innerHtml;
on an unrelated note: is all this $dom_ stuff going away before 1.0?

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/template...
tools/dom/templates/html/impl/impl_Element.darttemplate:1243: if (context ==
null || context is BodyElement) {
curious about the special case here for "body". could use a comment. should it
apply to "html", "head"?

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/template...
File tools/dom/templates/html/impl/impl_HTMLTemplateElement.darttemplate
(right):

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/template...
tools/dom/templates/html/impl/impl_HTMLTemplateElement.darttemplate:293: // An
override to place the contents into content rather than as child nodes.
Perhaps link spec here?
https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/templates/index.html#i...

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/template...
File tools/dom/templates/html/impl/impl_SVGElement.darttemplate (right):

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/template...
tools/dom/templates/html/impl/impl_SVGElement.darttemplate:43: ..allowSvg();
remove newline?

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/template...
File tools/dom/templates/html/impl/impl_SVGSVGElement.darttemplate (right):

https://chromiumcodereview.appspot.com/16374007/diff/36001/tools/dom/template...
tools/dom/templates/html/impl/impl_SVGSVGElement.darttemplate:10: // The SVG
spec requires the version attribute to match the spec version
curious why we need to do this? why doesn't DOM do it for us?

[blois, 2013-08-19 22:02:08]

Updated, but need to figure out a conflict with custom elements where elements
aren't getting upgraded.

https://codereview.chromium.org/16374007/diff/36001/tools/dom/src/Html5NodeVa...
File tools/dom/src/Html5NodeValidator.dart (right):

https://codereview.chromium.org/16374007/diff/36001/tools/dom/src/Html5NodeVa...
tools/dom/src/Html5NodeValidator.dart:16: * *
https://code.google.com/p/google-caja/wiki/CajaWhitelists
On 2013/08/17 06:07:07, John Messerly wrote:
> this is internal API but, would be nice to use markdown angle brackets for the
> link: < >

Done.

https://codereview.chromium.org/16374007/diff/36001/tools/dom/src/NodeValidat...
File tools/dom/src/NodeValidatorBuilder.dart (right):

https://codereview.chromium.org/16374007/diff/36001/tools/dom/src/NodeValidat...
tools/dom/src/NodeValidatorBuilder.dart:52: factory
NodeValidatorBuilder.common() {
On 2013/08/17 06:07:07, John Messerly wrote:
> as a factory constructor, this is hard to use in a subclass. Perhaps:
> 
> NodeValidatorBuilder.common() {
>   allowHtml5();
>   allowTemplating();
> }

Done. Though most extension should be via composition rather than subclassing.

https://codereview.chromium.org/16374007/diff/36001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/16374007/diff/36001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:334: return
fragment.nodes.where((e) => e is Element).single;
On 2013/08/17 06:07:07, John Messerly wrote:
> how's the performance of this? I dunno if it matters, but it seems there are
> some pretty easy micro-opts here avoiding the closure and "nodes" allocations.

Done.

https://codereview.chromium.org/16374007/diff/36001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1208: validator = new
NodeValidatorBuilder.common();
On 2013/08/17 06:07:07, John Messerly wrote:
> I wonder about performance here, in particular if we should be able to create
> these with const constructors to avoid allocations.

Added caching.

https://codereview.chromium.org/16374007/diff/36001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1215: return
_ElementFactoryProvider._parseHtml(this, html, treeSanitizer);
On 2013/08/17 06:07:07, John Messerly wrote:
> out of curiosity, why is this method on _ElementFactoryProvider?
> Also, which element of the factory provides an abstraction for the provider
for
> the factory of the elements?

FactoryProviders are a legacy thing that we've been phasing out- removed it in
this case.

https://codereview.chromium.org/16374007/diff/36001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1226: String get
innerHtml => $dom_innerHtml;
On 2013/08/17 06:07:07, John Messerly wrote:
> on an unrelated note: is all this $dom_ stuff going away before 1.0?

That's the plan- been having hiccups with benchmarks, good reminder to check on
status again.

https://codereview.chromium.org/16374007/diff/36001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1243: if (context ==
null || context is BodyElement) {
On 2013/08/17 06:07:07, John Messerly wrote:
> curious about the special case here for "body". could use a comment. should it
> apply to "html", "head"?

It's defaulting to body and just re-using that element if available. But head or
html will fail when it is added to the document. Based on our old code, I don't
think there's a scenario for supporting these.

https://codereview.chromium.org/16374007/diff/36001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_HTMLTemplateElement.darttemplate
(right):

https://codereview.chromium.org/16374007/diff/36001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_HTMLTemplateElement.darttemplate:293: // An
override to place the contents into content rather than as child nodes.
On 2013/08/17 06:07:07, John Messerly wrote:
> Perhaps link spec here?
>
https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/templates/index.html#i...

Done.

https://codereview.chromium.org/16374007/diff/36001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_SVGElement.darttemplate (right):

https://codereview.chromium.org/16374007/diff/36001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_SVGElement.darttemplate:43: ..allowSvg();
On 2013/08/17 06:07:07, John Messerly wrote:
> remove newline?

Done.

https://codereview.chromium.org/16374007/diff/36001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_SVGSVGElement.darttemplate (right):

https://codereview.chromium.org/16374007/diff/36001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_SVGSVGElement.darttemplate:10: // The SVG
spec requires the version attribute to match the spec version
On 2013/08/17 06:07:07, John Messerly wrote:
> curious why we need to do this? why doesn't DOM do it for us?

Not sure- this code was just moved from the factory to here, so it's not new
code.

[Jennifer Messerly, 2013-08-26 22:00:30]

btw, this LGTM still

https://codereview.chromium.org/16374007/diff/36001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/16374007/diff/36001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1215: return
_ElementFactoryProvider._parseHtml(this, html, treeSanitizer);
On 2013/08/19 22:02:09, blois wrote:
> On 2013/08/17 06:07:07, John Messerly wrote:
> > out of curiosity, why is this method on _ElementFactoryProvider?
> > Also, which element of the factory provides an abstraction for the provider
> for
> > the factory of the elements?
> 
> FactoryProviders are a legacy thing that we've been phasing out- removed it in
> this case.

yay :D

[Jennifer Messerly, 2013-08-26 22:00:32]

btw, this LGTM still

[blois, 2013-08-27 01:48:28]

Committed patchset #6 manually as r26681 (presubmit successful).