Wrapperless dart:html generator

Wrapperless dart:html generator

BUG=
TEST=

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

[Jacob, 2012-02-15 07:24:34]

Please focus the review on the python scripts and templates.
The generated output is not hand edited in any way.

Note: these changes are sufficient to run swarm and other samples correctly but
are incomplete (e.g. DocumentFragment as an Element hasn't been supported and
custom constructors are only implemented for Event and not all event subclasses.
I plan to checkin this version without the /frog change to switch this as the
implementation of html.

Design:
Implement dart:html without requiring wrappers for almost all dom classes.
Wrappers are still required for ElementList but that will be cleaned up as well.
We make the merge of Document and DocumentElement work by always using the
DocumentElement and never exposing the Document to user visible code.  This
means that === is still safe to use but means we must be careful when dealing
with the few cases where a Document can be returned by other DOM methods (e.g.
Event.target).  Please comment on any cases where a document can be returned
that I missed.

We expose raw dom methods that are removed from dart:html but required to
implement dart:html by using their regular dom names but prefixed with an _.
TBD: we need some way of suppressing cross-library checks for _ methods in
dartium so that we can make the same dart:html tests run both with the Frog
version and the dart:html version 

Note: I also through in an orthogonal change that enabled caching on the IDL
database parse to make iterating on the bindings more pleasant.

[nweiz, 2012-02-15 21:35:59]

lgtm

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
File client/dom/scripts/dartdomgenerator.py (right):

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartdomgenerator.py:31: _webkit_renames_inverse = None
This doesn't need to be defined top-level; it's only used in GenerateDOM.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/tests/client/...
File client/tests/client/html/ElementTests.dart (right):

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/tests/client/...
client/tests/client/html/ElementTests.dart:279: var rawElement = element;
The rawElement variable should probably just be removed.

[sra1, 2012-02-16 04:43:28]

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
File client/dom/scripts/dartdomgenerator.py (right):

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartdomgenerator.py:112: for interface in
common_database.GetInterfaces():
It would be nice to move this out of the main flow into a separate member
function, class or global function.
I think it can it be made to work without global variables (constant tables are
OK).

e.g.

   html_renames = self._MakeHtmlRenames(...)
   generator.RenameTypes(webkit_database, html_renames, True)

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartdomgenerator.py:178: num_html_systems = ('htmlfrog' in
systems) + ('htmldartium' in systems)
Move validation as early as feasible.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
File client/dom/scripts/dartgenerator.py (right):

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:61: _private_html_properties = {
For each of these tables, add a comment saying what the elements are.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:97: _html_library_remove = set([
These tables are pretty huge.
Could you get a small start on refactoring by moving them into their own file?

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:100: "Attr.*",
Does this work?

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:109: # the same field.
This should be fixed - can you try it?

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:706: generate_html_systems=False):
We are passing 'systems' and 'generate_html_systems'.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:740: html_interface_system =
HtmlInterfacesSystem(
Is this needed when 'not generate_html_systems' is True?
Perhaps this should be under if-then-else?
I would have thought this should only be run if
'htmlfrog' in systems or 'htmldartium' in systems.

You probably need to test that the default command line args and 'native' still
work :-)

(I git checkout master, blow away  generated, run, copy generated to generated0,
git checkout branch, run again and diff ../generated0 ../generated with meld).

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:900: for manual_event_name in
_html_manual_events[interface.id]:
Move this _html specific code into the generator.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:1151: def ParametersNameDeclaration(self):
This is generating an argument list, no?
Then call it ParametersAsArgumentList.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:1924: if getter and not
self._system._AllowInHtmlLibrary(self._interface, 'get:' + getter.id):
Line length

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2504: native_name = interface.native_name
Rather than 'rename' the interface.native_name, can you compute it here and keep
the IDL 'clean' ?

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2835: self._AddSetter(setter, True)
If you return after these two ifs you can drop the 'else:' on the next line and
outdent the rest to fix the line lengths.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2837: if getter and not
self._system._AllowInHtmlLibrary(self._interface, 'get:' + getter.id):
Line length.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2862: if getter:
I've seen this before.
Copy _AddAttributeUsingProperties from the DomFrog version, adding the new
parameter.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2917: '  $TYPE $PRIVATE$NAME($PARAMS) native
"$(RETURN)$(THIS).$NAME($PARAMNAMES);";\n',
Line length.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2923:
PARAMNAMES=info.ParametersNameDeclaration(),
ARGUMENTS=info.ParemetersAsArguments(),

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2939: self._members_emitter.Emit('\n  $TYPE
get on() =>\n    new $TYPE($EVENTTARGET);\n',
Line length.  Break like so:
        .....Emit(
            '...'

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2955: 'implements $INTERFACE {\n'
Does this fit on above line?

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
File client/dom/scripts/database.py (right):

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/database.py:125: def Cache(self):
nit: Cache is both a noun and a verb.  Maybe SaveToCache?

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/i...
File client/dom/scripts/idlnode.py (right):

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/i...
client/dom/scripts/idlnode.py:357: def has_attribute(self, candidate):
This is pretty much _FindMatchingAttribute

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/i...
client/dom/scripts/idlnode.py:363: def merge(self, other):
Please move the merge logic, including above function, out of the IDL  AST
layer.

[Jacob, 2012-02-17 00:44:22]

I also split the base, html, and frog system logic into separate files to make
things easier to maintain going forward.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
File client/dom/scripts/dartdomgenerator.py (right):

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartdomgenerator.py:31: _webkit_renames_inverse = None
On 2012/02/15 21:36:00, nweiz wrote:
> This doesn't need to be defined top-level; it's only used in GenerateDOM.

Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartdomgenerator.py:112: for interface in
common_database.GetInterfaces():
On 2012/02/16 04:43:28, sra1 wrote:
> It would be nice to move this out of the main flow into a separate member
> function, class or global function.
> I think it can it be made to work without global variables (constant tables
are
> OK).
> 
> e.g.
> 
>    html_renames = self._MakeHtmlRenames(...)
>    generator.RenameTypes(webkit_database, html_renames, True)

Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartdomgenerator.py:112: for interface in
common_database.GetInterfaces():
On 2012/02/16 04:43:28, sra1 wrote:
> It would be nice to move this out of the main flow into a separate member
> function, class or global function.
> I think it can it be made to work without global variables (constant tables
are
> OK).
> 
> e.g.
> 
>    html_renames = self._MakeHtmlRenames(...)
>    generator.RenameTypes(webkit_database, html_renames, True)

Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartdomgenerator.py:178: num_html_systems = ('htmlfrog' in
systems) + ('htmldartium' in systems)
On 2012/02/16 04:43:28, sra1 wrote:
> Move validation as early as feasible.

Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
File client/dom/scripts/dartgenerator.py (right):

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:61: _private_html_properties = {
On 2012/02/16 04:43:28, sra1 wrote:
> For each of these tables, add a comment saying what the elements are.

Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:97: _html_library_remove = set([
On 2012/02/16 04:43:28, sra1 wrote:
> These tables are pretty huge.
> Could you get a small start on refactoring by moving them into their own file?

Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:100: "Attr.*",
On 2012/02/16 04:43:28, sra1 wrote:
> Does this work?
Added a TODO.  only parts of this table of stuff generated by Nathan are
actually used by my code.  Specifically methods are removed but classes aren't.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:109: # the same field.
On 2012/02/16 04:43:28, sra1 wrote:
> This should be fixed - can you try it?
listed the bug# in the TODO

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:706: generate_html_systems=False):
On 2012/02/16 04:43:28, sra1 wrote:
> We are passing 'systems' and 'generate_html_systems'.

done

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:740: html_interface_system =
HtmlInterfacesSystem(
On 2012/02/16 04:43:28, sra1 wrote:
> Is this needed when 'not generate_html_systems' is True?
> Perhaps this should be under if-then-else?
> I would have thought this should only be run if
> 'htmlfrog' in systems or 'htmldartium' in systems.
> 
> You probably need to test that the default command line args and 'native'
still
> work :-)
> 
> (I git checkout master, blow away  generated, run, copy generated to
generated0,
> git checkout branch, run again and diff ../generated0 ../generated with meld).

Nope. Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:900: for manual_event_name in
_html_manual_events[interface.id]:
On 2012/02/16 04:43:28, sra1 wrote:
> Move this _html specific code into the generator.

Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:1151: def ParametersNameDeclaration(self):
On 2012/02/16 04:43:28, sra1 wrote:
> This is generating an argument list, no?
> Then call it ParametersAsArgumentList.

Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:1924: if getter and not
self._system._AllowInHtmlLibrary(self._interface, 'get:' + getter.id):
On 2012/02/16 04:43:28, sra1 wrote:
> Line length

Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2504: native_name = interface.native_name
On 2012/02/16 04:43:28, sra1 wrote:
> Rather than 'rename' the interface.native_name, can you compute it here and
keep
> the IDL 'clean' ?
discussed offline. this is needed.
changed name to javascript_binding_name

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2835: self._AddSetter(setter, True)
On 2012/02/16 04:43:28, sra1 wrote:
> If you return after these two ifs you can drop the 'else:' on the next line
and
> outdent the rest to fix the line lengths.

Done

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2837: if getter and not
self._system._AllowInHtmlLibrary(self._interface, 'get:' + getter.id):
On 2012/02/16 04:43:28, sra1 wrote:
> Line length.

Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2862: if getter:
On 2012/02/16 04:43:28, sra1 wrote:
> I've seen this before.
> Copy _AddAttributeUsingProperties from the DomFrog version, adding the new
> parameter.

Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2917: '  $TYPE $PRIVATE$NAME($PARAMS) native
"$(RETURN)$(THIS).$NAME($PARAMNAMES);";\n',
On 2012/02/16 04:43:28, sra1 wrote:
> Line length.

Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2923:
PARAMNAMES=info.ParametersNameDeclaration(),
On 2012/02/16 04:43:28, sra1 wrote:
> ARGUMENTS=info.ParemetersAsArguments(),

Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2939: self._members_emitter.Emit('\n  $TYPE
get on() =>\n    new $TYPE($EVENTTARGET);\n',
On 2012/02/16 04:43:28, sra1 wrote:
> Line length.  Break like so:
>         .....Emit(
>             '...'

Done.

https://chromiumcodereview.appspot.com/9403004/diff/1273/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2955: 'implements $INTERFACE {\n'
On 2012/02/16 04:43:28, sra1 wrote:
> Does this fit on above line?

Done.

[sra1, 2012-02-17 21:13:07]

There still some cleanup I would like to see but I think we need to land this
and then cleanup.

There is one exception - I would like to see systembase.py split into
generator.py, systembase.py and systeminterface.py
Most of the 'util'-like stuff that came from dartgenerator (like
RecognizeCallback, AnalyzeOperation ...) should be in generator.py.

Systembase might end up quite small with only the abstract part of system - that
is OK.

[sra1, 2012-02-17 21:49:08]

LGTM with the above mentioned file split.

http://codereview.chromium.org/9403004/diff/15001/client/dom/scripts/idlnode.py
File client/dom/scripts/idlnode.py (right):

http://codereview.chromium.org/9403004/diff/15001/client/dom/scripts/idlnode....
client/dom/scripts/idlnode.py:362: def merge(self, other):
Please move this - the classes in this file should be plain data.