Port DocumentFragment to the new wrapperless DOM.

Port DocumentFragment to the new wrapperless DOM.

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

[Jacob, 2012-03-06 05:09:00]

https://chromiumcodereview.appspot.com/9610011/diff/1/client/dom/scripts/dart...
File client/dom/scripts/dartgenerator.py (right):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:402: # Hack to generate no-op Element events
for DocumentFragment.
Hacks like this should not go here.  Place the hack in systemhtml.py instead.
Also, instead of hard coding the name DocumentFragment, I'd track a set of all
types like this as I expect we'll soon have multiple types to force adding
events for.  I know it will be a little bit more work to make the hack only
exist in systemhtml but that is better than bleeding our logic into
dartgenerator more than we have to.

https://chromiumcodereview.appspot.com/9610011/diff/1/client/dom/scripts/syst...
File client/dom/scripts/systemhtml.py (left):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:480: return ['ElementEvents']
Why are you hard coding the parent for 'Document'?

https://chromiumcodereview.appspot.com/9610011/diff/1/client/html/generated/h...
File client/html/generated/html/dartium/Clipboard.dart (left):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/html/generated/h...
client/html/generated/html/dartium/Clipboard.dart:18: 
can you keep CLs purely regenerating the DOM bindings separate from CLs with
actual changes. That will save a lot of time looking at the diffs.

https://chromiumcodereview.appspot.com/9610011/diff/1/client/html/src/shared_...
File client/html/src/shared_FactoryProviders.dart (right):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/html/src/shared_...
client/html/src/shared_FactoryProviders.dart:109: var fragment = new
DocumentFragment();
use final instead of var here and elsewhere.

https://chromiumcodereview.appspot.com/9610011/diff/1/client/tests/client/htm...
File client/tests/client/html/DocumentFragmentTests.dart (right):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/tests/client/htm...
client/tests/client/html/DocumentFragmentTests.dart:42: // in a wrapperless
world?
yes. just don't use a real style object and you'll be fine.  For example you
could a non-native class that inherits from a  native class but overrides
getValue.  Not sure if that is worthwhile though.  Doesn't seem like that
important a thing to enforce.

[nweiz, 2012-03-06 20:10:47]

https://chromiumcodereview.appspot.com/9610011/diff/1/client/dom/scripts/dart...
File client/dom/scripts/dartgenerator.py (right):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:402: # Hack to generate no-op Element events
for DocumentFragment.
On 2012/03/06 05:09:00, Jacob wrote:
> Hacks like this should not go here.  Place the hack in systemhtml.py instead.
> Also, instead of hard coding the name DocumentFragment, I'd track a set of all
> types like this as I expect we'll soon have multiple types to force adding
> events for.  I know it will be a little bit more work to make the hack only
> exist in systemhtml but that is better than bleeding our logic into
> dartgenerator more than we have to.

I'm not sure how to put this in systemhtml.py, since all the logic for whether
or not to call AddEventAttributes is in dartgenerator.py.

https://chromiumcodereview.appspot.com/9610011/diff/1/client/dom/scripts/syst...
File client/dom/scripts/systemhtml.py (left):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:480: return ['ElementEvents']
On 2012/03/06 05:09:00, Jacob wrote:
> Why are you hard coding the parent for 'Document'?

That was there prior to this change.

https://chromiumcodereview.appspot.com/9610011/diff/1/client/html/generated/h...
File client/html/generated/html/dartium/Clipboard.dart (left):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/html/generated/h...
client/html/generated/html/dartium/Clipboard.dart:18: 
On 2012/03/06 05:09:00, Jacob wrote:
> can you keep CLs purely regenerating the DOM bindings separate from CLs with
> actual changes. That will save a lot of time looking at the diffs.

Do you mean the CLs shouldn't include unrelated DOM regenerations like this one?
Or do you mean they shouldn't include DOM any regenerations, even for the
template changes included in the main CL?

If it's the latter, I disagree; bundling those regenerations with the CLs that
produced them makes it easier for other people working on the bindings (since
they don't have to deal with a bunch of unrelated changes) and makes rollbacks
much more feasible.

https://chromiumcodereview.appspot.com/9610011/diff/1/client/html/src/shared_...
File client/html/src/shared_FactoryProviders.dart (right):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/html/src/shared_...
client/html/src/shared_FactoryProviders.dart:109: var fragment = new
DocumentFragment();
On 2012/03/06 05:09:00, Jacob wrote:
> use final instead of var here and elsewhere.

Done.

https://chromiumcodereview.appspot.com/9610011/diff/1/client/tests/client/htm...
File client/tests/client/html/DocumentFragmentTests.dart (right):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/tests/client/htm...
client/tests/client/html/DocumentFragmentTests.dart:42: // in a wrapperless
world?
On 2012/03/06 05:09:00, Jacob wrote:
> yes. just don't use a real style object and you'll be fine.  For example you
> could a non-native class that inherits from a  native class but overrides
> getValue.  Not sure if that is worthwhile though.  Doesn't seem like that
> important a thing to enforce.

I don't follow... how would you get an instance of a subclass of
CSSStyleDeclaration in a wrapperless world?

I think enforcing this is correct if we're enforcing that other meaningless
setters throw errors.

[Jacob, 2012-03-06 22:41:59]

https://chromiumcodereview.appspot.com/9610011/diff/1/client/dom/scripts/dart...
File client/dom/scripts/dartgenerator.py (right):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:402: # Hack to generate no-op Element events
for DocumentFragment.
On 2012/03/06 20:10:47, nweiz wrote:
> On 2012/03/06 05:09:00, Jacob wrote:
> > Hacks like this should not go here.  Place the hack in systemhtml.py
instead.
> > Also, instead of hard coding the name DocumentFragment, I'd track a set of
all
> > types like this as I expect we'll soon have multiple types to force adding
> > events for.  I know it will be a little bit more work to make the hack only
> > exist in systemhtml but that is better than bleeding our logic into
> > dartgenerator more than we have to.
> 
> I'm not sure how to put this in systemhtml.py, since all the logic for whether
> or not to call AddEventAttributes is in dartgenerator.py.
AddEventAttributes is really dart:html specific.  Go ahead and move all the
EventAttributes logic out of dartgenerator.py and stick it in the
HtmlSystemShared class or some other reasonable location in systemhtml.py

https://chromiumcodereview.appspot.com/9610011/diff/1/client/html/generated/h...
File client/html/generated/html/dartium/Clipboard.dart (left):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/html/generated/h...
client/html/generated/html/dartium/Clipboard.dart:18: 
I mean avoid unrelated DOM regenerations whenever possible.  Naturally actually
regenerating the DOM to show what your .py script changes did is a good thing.
On 2012/03/06 20:10:47, nweiz wrote:
> On 2012/03/06 05:09:00, Jacob wrote:
> > can you keep CLs purely regenerating the DOM bindings separate from CLs with
> > actual changes. That will save a lot of time looking at the diffs.
> 
> Do you mean the CLs shouldn't include unrelated DOM regenerations like this
one?
> Or do you mean they shouldn't include DOM any regenerations, even for the
> template changes included in the main CL?
> 
> If it's the latter, I disagree; bundling those regenerations with the CLs that
> produced them makes it easier for other people working on the bindings (since
> they don't have to deal with a bunch of unrelated changes) and makes rollbacks
> much more feasible.

https://chromiumcodereview.appspot.com/9610011/diff/1/client/tests/client/htm...
File client/tests/client/html/DocumentFragmentTests.dart (right):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/tests/client/htm...
client/tests/client/html/DocumentFragmentTests.dart:42: // in a wrapperless
world?
On 2012/03/06 20:10:47, nweiz wrote:
> On 2012/03/06 05:09:00, Jacob wrote:
> > yes. just don't use a real style object and you'll be fine.  For example you
> > could a non-native class that inherits from a  native class but overrides
> > getValue.  Not sure if that is worthwhile though.  Doesn't seem like that
> > important a thing to enforce.
> 
> I don't follow... how would you get an instance of a subclass of
> CSSStyleDeclaration in a wrapperless world?
> 
> I think enforcing this is correct if we're enforcing that other meaningless
> setters throw errors.

You do it by

class _FrozenCSSStyleDeclation extends _CSSStyleDeclarationImpl {
  _FrozenCSSStyleDeclation() {
    ...
  }
  ....
}


new _FrozenCSSStyleDeclation()
I believe that works but sra can confirm whether that is a horrible thing to do.

I agree that enforcing this is correct but pragmatically speaking i think it
would be alright if we had to punt it if it is hard to do or makes the code
harder for the compiler optimize.

[nweiz, 2012-03-08 23:02:49]

https://chromiumcodereview.appspot.com/9610011/diff/1/client/dom/scripts/dart...
File client/dom/scripts/dartgenerator.py (right):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:402: # Hack to generate no-op Element events
for DocumentFragment.
On 2012/03/06 22:41:59, Jacob wrote:
> On 2012/03/06 20:10:47, nweiz wrote:
> > On 2012/03/06 05:09:00, Jacob wrote:
> > > Hacks like this should not go here.  Place the hack in systemhtml.py
> instead.
> > > Also, instead of hard coding the name DocumentFragment, I'd track a set of
> all
> > > types like this as I expect we'll soon have multiple types to force adding
> > > events for.  I know it will be a little bit more work to make the hack
only
> > > exist in systemhtml but that is better than bleeding our logic into
> > > dartgenerator more than we have to.
> > 
> > I'm not sure how to put this in systemhtml.py, since all the logic for
whether
> > or not to call AddEventAttributes is in dartgenerator.py.
> AddEventAttributes is really dart:html specific.  Go ahead and move all the
> EventAttributes logic out of dartgenerator.py and stick it in the
> HtmlSystemShared class or some other reasonable location in systemhtml.py

Done.

https://chromiumcodereview.appspot.com/9610011/diff/1/client/tests/client/htm...
File client/tests/client/html/DocumentFragmentTests.dart (right):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/tests/client/htm...
client/tests/client/html/DocumentFragmentTests.dart:42: // in a wrapperless
world?
On 2012/03/06 22:41:59, Jacob wrote:
> On 2012/03/06 20:10:47, nweiz wrote:
> > On 2012/03/06 05:09:00, Jacob wrote:
> > > yes. just don't use a real style object and you'll be fine.  For example
you
> > > could a non-native class that inherits from a  native class but overrides
> > > getValue.  Not sure if that is worthwhile though.  Doesn't seem like that
> > > important a thing to enforce.
> > 
> > I don't follow... how would you get an instance of a subclass of
> > CSSStyleDeclaration in a wrapperless world?
> > 
> > I think enforcing this is correct if we're enforcing that other meaningless
> > setters throw errors.
> 
> You do it by
> 
> class _FrozenCSSStyleDeclation extends _CSSStyleDeclarationImpl {
>   _FrozenCSSStyleDeclation() {
>     ...
>   }
>   ....
> }
> 
> 
> new _FrozenCSSStyleDeclation()
> I believe that works but sra can confirm whether that is a horrible thing to
do.
> 
> I agree that enforcing this is correct but pragmatically speaking i think it
> would be alright if we had to punt it if it is hard to do or makes the code
> harder for the compiler optimize.

Done.

[nweiz, 2012-03-08 23:52:47]

https://chromiumcodereview.appspot.com/9610011/diff/1/client/tests/client/htm...
File client/tests/client/html/DocumentFragmentTests.dart (right):

https://chromiumcodereview.appspot.com/9610011/diff/1/client/tests/client/htm...
client/tests/client/html/DocumentFragmentTests.dart:42: // in a wrapperless
world?
On 2012/03/08 23:02:49, nweiz wrote:
> On 2012/03/06 22:41:59, Jacob wrote:
> > On 2012/03/06 20:10:47, nweiz wrote:
> > > On 2012/03/06 05:09:00, Jacob wrote:
> > > > yes. just don't use a real style object and you'll be fine.  For example
> you
> > > > could a non-native class that inherits from a  native class but
overrides
> > > > getValue.  Not sure if that is worthwhile though.  Doesn't seem like
that
> > > > important a thing to enforce.
> > > 
> > > I don't follow... how would you get an instance of a subclass of
> > > CSSStyleDeclaration in a wrapperless world?
> > > 
> > > I think enforcing this is correct if we're enforcing that other
meaningless
> > > setters throw errors.
> > 
> > You do it by
> > 
> > class _FrozenCSSStyleDeclation extends _CSSStyleDeclarationImpl {
> >   _FrozenCSSStyleDeclation() {
> >     ...
> >   }
> >   ....
> > }
> > 
> > 
> > new _FrozenCSSStyleDeclation()
> > I believe that works but sra can confirm whether that is a horrible thing to
> do.
> > 
> > I agree that enforcing this is correct but pragmatically speaking i think it
> > would be alright if we had to punt it if it is hard to do or makes the code
> > harder for the compiler optimize.
> 
> Done.

Actually, that doesn't work... Dart requires that you call the superclass
constructor, which isn't possible in a wrapperless world.

I think for now the thing to do is to allow property assignment to be a no-op.

[Jacob, 2012-03-09 00:12:14]

On 2012/03/08 23:52:47, nweiz wrote:
>
https://chromiumcodereview.appspot.com/9610011/diff/1/client/tests/client/htm...
> File client/tests/client/html/DocumentFragmentTests.dart (right):
> 
>
https://chromiumcodereview.appspot.com/9610011/diff/1/client/tests/client/htm...
> client/tests/client/html/DocumentFragmentTests.dart:42: // in a wrapperless
> world?
> On 2012/03/08 23:02:49, nweiz wrote:
> > On 2012/03/06 22:41:59, Jacob wrote:
> > > On 2012/03/06 20:10:47, nweiz wrote:
> > > > On 2012/03/06 05:09:00, Jacob wrote:
> > > > > yes. just don't use a real style object and you'll be fine.  For
example
> > you
> > > > > could a non-native class that inherits from a  native class but
> overrides
> > > > > getValue.  Not sure if that is worthwhile though.  Doesn't seem like
> that
> > > > > important a thing to enforce.
> > > > 
> > > > I don't follow... how would you get an instance of a subclass of
> > > > CSSStyleDeclaration in a wrapperless world?
> > > > 
> > > > I think enforcing this is correct if we're enforcing that other
> meaningless
> > > > setters throw errors.
> > > 
> > > You do it by
> > > 
> > > class _FrozenCSSStyleDeclation extends _CSSStyleDeclarationImpl {
> > >   _FrozenCSSStyleDeclation() {
> > >     ...
> > >   }
> > >   ....
> > > }
> > > 
> > > 
> > > new _FrozenCSSStyleDeclation()
> > > I believe that works but sra can confirm whether that is a horrible thing
to
> > do.
> > > 
> > > I agree that enforcing this is correct but pragmatically speaking i think
it
> > > would be alright if we had to punt it if it is hard to do or makes the
code
> > > harder for the compiler optimize.
> > 
> > Done.
> 
> Actually, that doesn't work... Dart requires that you call the superclass
> constructor, which isn't possible in a wrapperless world.
> 
> I think for now the thing to do is to allow property assignment to be a no-op.

Sounds good.

lgtm

[Jacob, 2012-03-09 02:10:54]

https://chromiumcodereview.appspot.com/9610011/diff/13001/client/html/generat...
File client/html/generated/html/dartium/DocumentFragment.dart (right):

https://chromiumcodereview.appspot.com/9610011/diff/13001/client/html/generat...
client/html/generated/html/dartium/DocumentFragment.dart:238: HTMLCollection
_children;
define with getters and setters instead of fields so these don't bloat the size
of the DocumentFragment object.

https://chromiumcodereview.appspot.com/9610011/diff/13001/client/tests/client...
File client/tests/client/html/DocumentFragmentTests.dart (right):

https://chromiumcodereview.appspot.com/9610011/diff/13001/client/tests/client...
client/tests/client/html/DocumentFragmentTests.dart:41: // Ideally these would
throw errors, but it's not possible to create a class
This comment should be marked with a TODO

[nweiz, 2012-03-09 02:18:04]

https://chromiumcodereview.appspot.com/9610011/diff/13001/client/html/generat...
File client/html/generated/html/dartium/DocumentFragment.dart (right):

https://chromiumcodereview.appspot.com/9610011/diff/13001/client/html/generat...
client/html/generated/html/dartium/DocumentFragment.dart:238: HTMLCollection
_children;
On 2012/03/09 02:10:55, Jacob wrote:
> define with getters and setters instead of fields so these don't bloat the
size
> of the DocumentFragment object.

I decided to define them as fields specifically to avoid code bloat under Frog.
I guess there's a tradeoff here... given that DocumentFragment is probably going
to be used relatively rarely, isn't it worth more to reduce code size under frog
than memory footprint under dartium?

https://chromiumcodereview.appspot.com/9610011/diff/13001/client/tests/client...
File client/tests/client/html/DocumentFragmentTests.dart (right):

https://chromiumcodereview.appspot.com/9610011/diff/13001/client/tests/client...
client/tests/client/html/DocumentFragmentTests.dart:41: // Ideally these would
throw errors, but it's not possible to create a class
On 2012/03/09 02:10:55, Jacob wrote:
> This comment should be marked with a TODO

There's not really anything to do... this is the best solution, barring unlikely
changes to the inheritance semantics of Dart.

[Jacob, 2012-03-09 22:45:36]

lgtm