Generate dart:html bindings for Dartium as well as Frog. All unittests now pass (or are disabled fo…

Generate dart:html bindings for Dartium as well as Frog. All unittests now pass (or are disabled for functionality that isn't yet ported over to the new dart:html implementation).

Files in
client/html/src that have been ported over have been removed while files that need to be ported have been left to simplify tracking the porting effort.
Created a client/dom/templates/html/impl directory that contains templates shared by the frog and dartium bindings.

There is still a bit too much duplicated code in the dartium and frog generators.

BUG=
TEST=

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

[Jacob, 2012-02-29 04:55:45]

Added Siggi for changes to provide both unittest_html.dart and unittest.dart as
code cannot import both dart:html and dart:dom anymore.

This CL makes the wrapperless dart:html code go live, adds a wrapper based
solution for dartium using a shared generator.

[Siggi Cherem (dart-lang), 2012-02-29 18:11:02]

+rnystrom to see if he has other suggestions for unittest_html.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/testing/unittest...
File client/testing/unittest/unittest_html.dart (right):

https://chromiumcodereview.appspot.com/9537001/diff/1/client/testing/unittest...
client/testing/unittest/unittest_html.dart:10: #import("dart:html");
I'm not convinced about duplicating everything in this file. Maybe we can create
a file with the common functionality (e.g. all the _toHTML part of the code)? 

Bob, any suggestion?

https://chromiumcodereview.appspot.com/9537001/diff/1/client/tests/client/cli...
File client/tests/client/client.status (right):

https://chromiumcodereview.appspot.com/9537001/diff/1/client/tests/client/cli...
client/tests/client/client.status:48: [ $component == frogium ]
did you mean to keep here webdriver as well? 'frog_html_tests' will be expected
to pass.

https://chromiumcodereview.appspot.com/9537001/diff/1/samples/tests/samples/t...
File samples/tests/samples/total/total_dartest.dart (right):

https://chromiumcodereview.appspot.com/9537001/diff/1/samples/tests/samples/t...
samples/tests/samples/total/total_dartest.dart:10:
#import('../../../../testing/unittest/unittest_html.dartest.dart');
mmm... sounds like you used a very aggressive perl regexp!

[Jacob, 2012-02-29 22:46:25]

https://chromiumcodereview.appspot.com/9537001/diff/1/client/tests/client/cli...
File client/tests/client/client.status (right):

https://chromiumcodereview.appspot.com/9537001/diff/1/client/tests/client/cli...
client/tests/client/client.status:48: [ $component == frogium ]
On 2012/02/29 18:11:02, sigmund wrote:
> did you mean to keep here webdriver as well? 'frog_html_tests' will be
expected
> to pass.
oops. fixed.

https://chromiumcodereview.appspot.com/9537001/diff/1/samples/tests/samples/t...
File samples/tests/samples/total/total_dartest.dart (right):

https://chromiumcodereview.appspot.com/9537001/diff/1/samples/tests/samples/t...
samples/tests/samples/total/total_dartest.dart:10:
#import('../../../../testing/unittest/unittest_html.dartest.dart');
On 2012/02/29 18:11:02, sigmund wrote:
> mmm... sounds like you used a very aggressive perl regexp!
correct.. i clearly got lazy and used a "." instead of "\." in my regexp...

And these tests were busted and weren't actually being run by test.py at all as
the filenames didn't match the pattern. Thanks to Emily for explaining this.
Fixed them to run and removed the dartest code that runs the same tests and the
regular version as I don't know what purpose it serves and it would be broken
given that total depends on html and it uses dart:dom

[sra1, 2012-02-29 23:11:55]

Initial pass at comments.

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

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartdomgenerator.py:176: print 'Cannot generate html and dom
bindings at the same time'
Just a question: could we generate both by partitioning the systems and calling
GenerateDOM twice?

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

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:284: TemplateLoader(self._template_dir,
['html/frog', 'html/impl', 'html',
I'd rather break the line between the arguments than inside the list.

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

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:13: from systeminterface import *
Are you using this import?

You could comment it out to see :-)

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:14: from systemwrapping import *
Are you using this import?

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:439: DartType(interface_name) + '.' +
member_prefix + member in candidates):
These two tests do the same when member_prefix='', so you can remove the first.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:452: # and they all appear to be same. 
Consider the alternate strategy of
same = sane?

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:852: if getter:
_AddAttributeUsingProperties?
Otherwise you might as well not have _AddAttributeUsingProperties since it is
called once.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:891: NAME=attr.id, TYPE=return_type,
I think it is easier to find TYPE if it is on its own line.  Ditto below.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:892: THIS='this.parentNode' if
self._interface.id == 'Document' else 'this'
THIS is not used

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:926: if (self._interface.id == 'Document' or
html_name != info.name or
Why self._interface.id == 'Document' ok I see - 'this'.

Perhaps break this down with comments?

# Do we need a native body?
if (self._interface.id == 'Document' or  # Need alternate 'this'
    html_name != info.name or            # renamed operation
    maybe_return_document):              # need to wrap value

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:927: info.type_name == 'Document') or
maybe_return_document:
Is info.typename == 'Doc' redundant with maybe_return_document?

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:946: return_type = '_EventTargetImpl'
This muck-with-the-names-and-fall-through logic is pretty confusing.
It might be clearer to make this an if-then-else and put both definitions in the
above 'emit'.

The emitters have a little-used feature: you can create an emitter that has
bindings.
This is shorter than a bunch of python variables and duplicated FOO=foo
bindings.

operation_emitter = self._members_emitter.Emit('$!SCOPE',
       RETURNTYPE=return_type, NAME=.... HTML_NAME=..., PARAMNAMES= ...,
       many more shared bindings )

if ...
  operation_emitter.Emit(
      ' $TYPE $HTML_NAME($PARAMS) => _Fix(_$HTML_NAME($PARAMS_AS_ARGS));\n'
      ' _EventTargetImpl _$HTML_NAME($PARAMS) native "...."\n',
      just a few bindings, perhaps shadowing one of the above)
else
  operation_emitter.Emit(' $TYPE ....')

The bindings do not have to be used,  i.e. Emit('', A=1) is fine) so the shared
bindings don't have to be shared.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:1114: def _FilePathForDartWrappingImpl(self,
interface_name):
Unused?

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:1145: shared: functionaly shared across all
Html generators.
Document 'template'.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:1339:
TARGETCLASS=self._NarrowOutputType(self._interface.id),
TARGETCLASS appears to be unused.

[Jacob, 2012-03-01 00:40:44]

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

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartdomgenerator.py:176: print 'Cannot generate html and dom
bindings at the same time'
On 2012/02/29 23:11:55, sra1 wrote:
> Just a question: could we generate both by partitioning the systems and
calling
> GenerateDOM twice?

Sounds reasonable but as it is unrelated to this CL lets do that as a separate
CL as this CL is already far too large.

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

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:284: TemplateLoader(self._template_dir,
['html/frog', 'html/impl', 'html',
On 2012/02/29 23:11:55, sra1 wrote:
> I'd rather break the line between the arguments than inside the list.

Done.

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

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:13: from systeminterface import *
On 2012/02/29 23:11:55, sra1 wrote:
> Are you using this import?
> 
> You could comment it out to see :-)

Done.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:14: from systemwrapping import *
On 2012/02/29 23:11:55, sra1 wrote:
> Are you using this import?

Done.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:439: DartType(interface_name) + '.' +
member_prefix + member in candidates):
On 2012/02/29 23:11:55, sra1 wrote:
> These two tests do the same when member_prefix='', so you can remove the
first.
Clarified offline. The design is the table of matching names may only contain
Document.bgColor instead of Document.get:bgColor
Added a comment indicating this and cleaned up the table of removed members to
take full advantage

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:452: # and they all appear to be same. 
Consider the alternate strategy of
On 2012/02/29 23:11:55, sra1 wrote:
> same = sane?
same ==> safe :)
done

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:852: if getter:
On 2012/02/29 23:11:55, sra1 wrote:
> _AddAttributeUsingProperties?
> Otherwise you might as well not have _AddAttributeUsingProperties since it is
> called once.

Done.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:891: NAME=attr.id, TYPE=return_type,
On 2012/02/29 23:11:55, sra1 wrote:
> I think it is easier to find TYPE if it is on its own line.  Ditto below.

Done.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:892: THIS='this.parentNode' if
self._interface.id == 'Document' else 'this'
On 2012/02/29 23:11:55, sra1 wrote:
> THIS is not used

Done.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:926: if (self._interface.id == 'Document' or
html_name != info.name or
Good comments. Done.
On 2012/02/29 23:11:55, sra1 wrote:
> Why self._interface.id == 'Document' ok I see - 'this'.
> 
> Perhaps break this down with comments?
> 
> # Do we need a native body?
> if (self._interface.id == 'Document' or  # Need alternate 'this'
>     html_name != info.name or            # renamed operation
>     maybe_return_document):              # need to wrap value

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:927: info.type_name == 'Document') or
maybe_return_document:
On 2012/02/29 23:11:55, sra1 wrote:
> Is info.typename == 'Doc' redundant with maybe_return_document?
fixed. That was legacy.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:946: return_type = '_EventTargetImpl'
On 2012/02/29 23:11:55, sra1 wrote:
> This muck-with-the-names-and-fall-through logic is pretty confusing.
> It might be clearer to make this an if-then-else and put both definitions in
the
> above 'emit'.
> 
> The emitters have a little-used feature: you can create an emitter that has
> bindings.
> This is shorter than a bunch of python variables and duplicated FOO=foo
> bindings.
> 
> operation_emitter = self._members_emitter.Emit('$!SCOPE',
>        RETURNTYPE=return_type, NAME=.... HTML_NAME=..., PARAMNAMES= ...,
>        many more shared bindings )
> 
> if ...
>   operation_emitter.Emit(
>       ' $TYPE $HTML_NAME($PARAMS) => _Fix(_$HTML_NAME($PARAMS_AS_ARGS));\n'
>       ' _EventTargetImpl _$HTML_NAME($PARAMS) native "...."\n',
>       just a few bindings, perhaps shadowing one of the above)
> else
>   operation_emitter.Emit(' $TYPE ....')
> 
> The bindings do not have to be used,  i.e. Emit('', A=1) is fine) so the
shared
> bindings don't have to be shared.
Nice. That makes the code better.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:1114: def _FilePathForDartWrappingImpl(self,
interface_name):
On 2012/02/29 23:11:55, sra1 wrote:
> Unused?

Done.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:1145: shared: functionaly shared across all
Html generators.
On 2012/02/29 23:11:55, sra1 wrote:
> Document 'template'.

Done.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:1339:
TARGETCLASS=self._NarrowOutputType(self._interface.id),
On 2012/02/29 23:11:55, sra1 wrote:
> TARGETCLASS appears to be unused.

Done.

[sra1, 2012-03-01 02:22:27]

lgtm

https://chromiumcodereview.appspot.com/9537001/diff/27055/client/dom/scripts/...
File client/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/9537001/diff/27055/client/dom/scripts/...
client/dom/scripts/systemhtml.py:84: # Syntax is:
ClassName.(get\.|set\.)?MemberName
get: not get.

[Siggi Cherem (dart-lang), 2012-03-01 16:21:24]

lgtm wrt unittest changes, but we should consider if we want to revert
unittest_html.dart as unittest.dart. I'm ok either way.

https://chromiumcodereview.appspot.com/9537001/diff/45128/samples/tests/sampl...
File samples/tests/samples/src/dartcombat/DartCombatTest.dart (right):

https://chromiumcodereview.appspot.com/9537001/diff/45128/samples/tests/sampl...
samples/tests/samples/src/dartcombat/DartCombatTest.dart:10:
#import('../../../../../client/testing/unittest/unittest_html.dart');
I thought you were going to keep unittest.dart as a default that uses dart:html
...

https://chromiumcodereview.appspot.com/9537001/diff/45128/tools/testing/dart/...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/9537001/diff/45128/tools/testing/dart/...
tools/testing/dart/test_suite.dart:455: String domLibraryImport = 'dart:dom';
this is just used to replace it in a template. feel free to remove this
altogether and change the template to take one parameter less. (see
browser_test.dart)

[nweiz, 2012-03-01 21:18:02]

lgtm

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
File client/dom/templates/html/dartium/html_dartium.darttemplate (right):

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
client/dom/templates/html/dartium/html_dartium.darttemplate:69: // Warning: does
not attempt wrap event listeners.
Nit: "does not attempt to wrap"

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
client/dom/templates/html/dartium/html_dartium.darttemplate:74: return
domObject.dartObjectLocalStorage;
Nit: {}

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
File client/dom/templates/html/dartium/impl_Document.darttemplate (right):

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
client/dom/templates/html/dartium/impl_Document.darttemplate:12:
_DocumentImpl._wrap(ptr) :
Nit: indentation

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
client/dom/templates/html/dartium/impl_Document.darttemplate:34: // adding
checks to all methods that could an HTMLDocument.  We believe that
"that could return an HTMLDocument"

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
File client/dom/templates/html/dartium/impl_EventTarget.darttemplate (right):

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
client/dom/templates/html/dartium/impl_EventTarget.darttemplate:17:
EventListenerList _get(String type) {
Why is this a separate method? Same with _add/_remove below.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
client/dom/templates/html/dartium/impl_EventTarget.darttemplate:36: //
TODO(jacobr): switch to <_EventListenerWrapper>[] when the VM allow it.
Issue number?

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
client/dom/templates/html/dartium/impl_EventTarget.darttemplate:50: //
TODO(jacobr): what is the correct behavior here. We could alternately
I like asserting. If the type doesn't match it's almost certainly a bug.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
client/dom/templates/html/dartium/impl_EventTarget.darttemplate:70: if
(_wrappers === null) {
When is _wrappers ever null? Same question for _findOrAddWrapper.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
client/dom/templates/html/dartium/impl_EventTarget.darttemplate:73: for (int i =
0; i < _wrappers.length; i++) {
Maybe it's my Ruby bias, but I hate mixing raw for loops with code. I'd make a
more generic _removeFirst(List, bool criterion(val)) function and call that
here.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
client/dom/templates/html/dartium/impl_EventTarget.darttemplate:99: final
wrapped = (e) { listener(_wrap(e)); };
Style nit: wrapped = listener . _wrap

Oh wait, wrong language.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
File client/dom/templates/html/frog/html_frog.darttemplate (right):

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
client/dom/templates/html/frog/html_frog.darttemplate:27: _DocumentImpl
_cachedDocument;
These should probably have the same name as they do in html_dartium

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
File client/dom/templates/html/impl/impl_NodeList.darttemplate (right):

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
client/dom/templates/html/impl/impl_NodeList.darttemplate:155: // FIXME:
implement thesee.
I don't think anything needs to be implemented here.

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
File client/dom/templates/html/interface/interface_Element.darttemplate (right):

https://chromiumcodereview.appspot.com/9537001/diff/1/client/dom/templates/ht...
client/dom/templates/html/interface/interface_Element.darttemplate:253: //
interface $ID$EXTENDS default _ElementImpl {
Why is the interface extending a private class?

https://chromiumcodereview.appspot.com/9537001/diff/45128/client/dom/template...
File client/dom/templates/html/dartium/html_dartium.darttemplate (right):

https://chromiumcodereview.appspot.com/9537001/diff/45128/client/dom/template...
client/dom/templates/html/dartium/html_dartium.darttemplate:6: // Auto-generated
dart:html library.
This is a template, so it's not actually generated, right?

https://chromiumcodereview.appspot.com/9537001/diff/45128/client/html/src/_DO...
File client/html/src/_DOMTypeBase.dart (right):

https://chromiumcodereview.appspot.com/9537001/diff/45128/client/html/src/_DO...
client/html/src/_DOMTypeBase.dart:8: _DOMTypeBase._wrap(this._ptr) {
Should this be in a dartium-specific directory or named dartium_DOMTypeBase?

https://chromiumcodereview.appspot.com/9537001/diff/45128/client/html/src/sha...
File client/html/src/shared_FactoryProviders.dart (right):

https://chromiumcodereview.appspot.com/9537001/diff/45128/client/html/src/sha...
client/html/src/shared_FactoryProviders.dart:79: // TODO(jacobr): make type
dom.HTMLElement when dartium allows it.
This is no longer necessary

https://chromiumcodereview.appspot.com/9537001/diff/45128/client/html/src/sha...
client/html/src/shared_FactoryProviders.dart:80: _ElementImpl temp =
document._createElement(parentTag);
Should this be Element rather than _ElementImpl?

https://chromiumcodereview.appspot.com/9537001/diff/45128/client/testing/unit...
File client/testing/unittest/unittest_dom.dart (right):

https://chromiumcodereview.appspot.com/9537001/diff/45128/client/testing/unit...
client/testing/unittest/unittest_dom.dart:3: // BSD-style license that can be
found in the LICENSE file.
Is there a way to share more code between this and unittest_html? There's a lot
of duplication as is.

https://chromiumcodereview.appspot.com/9537001/diff/45128/client/tests/client...
File client/tests/client/client.status (right):

https://chromiumcodereview.appspot.com/9537001/diff/45128/client/tests/client...
client/tests/client/client.status:48: [ $component == frogium || ($component ==
webdriver && ($browser == safari || $browser == ie))]
This should be disabled whenever $component == webdriver for now, since
otherwise it'll break with webdriver + Firefox.