Move html events generation to domimpl.

Move html events generation to domimpl.


R=antonm@chromium.org

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

[sra1, 2012-05-14 17:29:15]

https://chromiumcodereview.appspot.com/10332144/diff/1/lib/dom/scripts/system...
File lib/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/10332144/diff/1/lib/dom/scripts/system...
lib/dom/scripts/systemhtml.py:460: assert event_name in _html_event_names, 'No
known html even name for event: ' + event_name
Line length.

https://chromiumcodereview.appspot.com/10332144/diff/1/lib/dom/scripts/system...
File lib/dom/scripts/systemnative.py (right):

https://chromiumcodereview.appspot.com/10332144/diff/1/lib/dom/scripts/system...
lib/dom/scripts/systemnative.py:13: from systemhtml import *
Please make new imports more specific.

https://chromiumcodereview.appspot.com/10332144/diff/1/tests/html/documentfra...
File tests/html/documentfragment_test.dart (right):

https://chromiumcodereview.appspot.com/10332144/diff/1/tests/html/documentfra...
tests/html/documentfragment_test.dart:283: fragment.on['click'].add((e) =>
null);
Why does on.click. no longer work?

[podivilov, 2012-05-15 10:24:26]

https://chromiumcodereview.appspot.com/10332144/diff/1/lib/dom/scripts/system...
File lib/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/10332144/diff/1/lib/dom/scripts/system...
lib/dom/scripts/systemhtml.py:460: assert event_name in _html_event_names, 'No
known html even name for event: ' + event_name
On 2012/05/14 17:29:15, sra1 wrote:
> Line length.

Done.

https://chromiumcodereview.appspot.com/10332144/diff/1/lib/dom/scripts/system...
File lib/dom/scripts/systemnative.py (right):

https://chromiumcodereview.appspot.com/10332144/diff/1/lib/dom/scripts/system...
lib/dom/scripts/systemnative.py:13: from systemhtml import *
On 2012/05/14 17:29:15, sra1 wrote:
> Please make new imports more specific.

Done.

https://chromiumcodereview.appspot.com/10332144/diff/1/tests/html/documentfra...
File tests/html/documentfragment_test.dart (right):

https://chromiumcodereview.appspot.com/10332144/diff/1/tests/html/documentfra...
tests/html/documentfragment_test.dart:283: fragment.on['click'].add((e) =>
null);
On 2012/05/14 17:29:15, sra1 wrote:
> Why does on.click. no longer work?
It looks like DocumentFragment shouldn't declare on.* listeners since there's no
DocumentFragment.onfoo attributes in DOM API.
While technically it is an event target, there's probably no real life use case
when you need to add onclick listener on DocumentFragment, so I think we should
remove it from the API.
It's outside of current cl scope, so I reverted the test for now. Let's discuss
on today's meeting.

[Anton Muhin, 2012-05-15 14:54:57]

1st round

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/scripts/dar...
File lib/dom/scripts/dartgenerator.py (right):

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/scripts/dar...
lib/dom/scripts/dartgenerator.py:315: super_map = dict((v,k) for k, v in
webkit_renames.iteritems())
nit: do not we need a space after comma?  (v, k)?

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/scripts/sys...
File lib/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/scripts/sys...
lib/dom/scripts/systemhtml.py:461: 'No known html even name for event: ' +
event_name
nit: even<t>

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/scripts/sys...
File lib/dom/scripts/systemnative.py (right):

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/scripts/sys...
lib/dom/scripts/systemnative.py:324: if self._interface.id ==
'DocumentFragment':
nit: move to the very beginning of the method?

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/scripts/sys...
lib/dom/scripts/systemnative.py:343: parent =
_ParentWithEventAttributes(self._system._database, self._interface) or ''
can it be ever a null?

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/scripts/sys...
lib/dom/scripts/systemnative.py:344: html_inteface =
self._system._html_renames.get(self._interface.id, self._interface.id)
do not we have a helper for this operation?

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/scripts/sys...
lib/dom/scripts/systemnative.py:358: "  EventListenerList get $HTML_NAME() =>
_get('$DOM_NAME');\n",
nit: shouldn't you use '' for outer and "" for inner strings?

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/scripts/sys...
lib/dom/scripts/systemnative.py:366: '    if (_on == null) _on = new
$EVENTS_CLASS(this);\n'
nit: === null?

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/scripts/sys...
lib/dom/scripts/systemnative.py:366: '    if (_on == null) _on = new
$EVENTS_CLASS(this);\n'
btw, maybe not for this change, but I believe lazy initialization values have
been added to the spec.

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/scripts/sys...
lib/dom/scripts/systemnative.py:1208: def _HasEventTargetParent(database,
interface):
nit: I believe in generator database argument is usually put second, but it
might have been me who spoiled it all

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/scripts/sys...
lib/dom/scripts/systemnative.py:1211: if parent_name == 'EventTarget' or not
database.HasInterface(parent_name):
it's time to abstract away tree traversal, wdyt?

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/src/native_...
File lib/dom/src/native_EventsImplementation.dart (right):

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/src/native_...
lib/dom/src/native_EventsImplementation.dart:13: html.EventListenerList operator
[](String type) {
nit: => syntax?

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/src/native_...
lib/dom/src/native_EventsImplementation.dart:17: html.EventListenerList
_get(String type) {
why auxiliary _get?

https://chromiumcodereview.appspot.com/10332144/diff/5002/lib/dom/src/native_...
lib/dom/src/native_EventsImplementation.dart:36: // TODO(jacobr): switch to
<EventListenerWrapper>[] when the VM allow it.
nit: at least in the rest of Dart code : is usually on the next line.  And <T>[]
should be supported now, I think

[podivilov, 2012-05-16 12:32:24]

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/dartgenerat...
File lib/dom/scripts/dartgenerator.py (right):

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/dartgenerat...
lib/dom/scripts/dartgenerator.py:315: super_map = dict((v,k) for k, v in
webkit_renames.iteritems())
On 2012/05/15 14:54:57, antonmuhin wrote:
> nit: do not we need a space after comma?  (v, k)?

Done.

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/systemhtml.py
File lib/dom/scripts/systemhtml.py (right):

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/systemhtml....
lib/dom/scripts/systemhtml.py:461: 'No known html even name for event: ' +
event_name
On 2012/05/15 14:54:57, antonmuhin wrote:
> nit: even<t>

Done.

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/systemnativ...
File lib/dom/scripts/systemnative.py (right):

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:324: if self._interface.id ==
'DocumentFragment':
On 2012/05/15 14:54:57, antonmuhin wrote:
> nit: move to the very beginning of the method?

Done.

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:343: parent =
_ParentWithEventAttributes(self._system._database, self._interface) or ''
On 2012/05/15 14:54:57, antonmuhin wrote:
> can it be ever a null?

Yes, for example Node doesn't have event target parents.

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:344: html_inteface =
self._system._html_renames.get(self._interface.id, self._interface.id)
On 2012/05/15 14:54:57, antonmuhin wrote:
> do not we have a helper for this operation?

Do we?

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:358: "  EventListenerList get $HTML_NAME() =>
_get('$DOM_NAME');\n",
On 2012/05/15 14:54:57, antonmuhin wrote:
> nit: shouldn't you use '' for outer and "" for inner strings?

Done.

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:366: '    if (_on == null) _on = new
$EVENTS_CLASS(this);\n'
On 2012/05/15 14:54:57, antonmuhin wrote:
> nit: === null?

Done.

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:1208: def _HasEventTargetParent(database,
interface):
On 2012/05/15 14:54:57, antonmuhin wrote:
> nit: I believe in generator database argument is usually put second, but it
> might have been me who spoiled it all

Done.

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:1211: if parent_name == 'EventTarget' or not
database.HasInterface(parent_name):
On 2012/05/15 14:54:57, antonmuhin wrote:
> it's time to abstract away tree traversal, wdyt?

Done.

http://codereview.chromium.org/10332144/diff/5002/lib/dom/src/native_EventsIm...
File lib/dom/src/native_EventsImplementation.dart (right):

http://codereview.chromium.org/10332144/diff/5002/lib/dom/src/native_EventsIm...
lib/dom/src/native_EventsImplementation.dart:13: html.EventListenerList operator
[](String type) {
On 2012/05/15 14:54:57, antonmuhin wrote:
> nit: => syntax?

Done.

http://codereview.chromium.org/10332144/diff/5002/lib/dom/src/native_EventsIm...
lib/dom/src/native_EventsImplementation.dart:17: html.EventListenerList
_get(String type) {
On 2012/05/15 14:54:57, antonmuhin wrote:
> why auxiliary _get?

It is used by subclasses.

http://codereview.chromium.org/10332144/diff/5002/lib/dom/src/native_EventsIm...
lib/dom/src/native_EventsImplementation.dart:36: // TODO(jacobr): switch to
<EventListenerWrapper>[] when the VM allow it.
On 2012/05/15 14:54:57, antonmuhin wrote:
> nit: at least in the rest of Dart code : is usually on the next line.
Done.
> And <T>[] should be supported now, I think
Could you please point a revision where it was added?

[Anton Muhin, 2012-05-16 13:17:56]

LGTM w/ comments addressed

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/systemnativ...
File lib/dom/scripts/systemnative.py (right):

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:343: parent =
_ParentWithEventAttributes(self._system._database, self._interface) or ''
In this case, where are the guards?

On 2012/05/16 12:32:24, podivilov wrote:
> On 2012/05/15 14:54:57, antonmuhin wrote:
> > can it be ever a null?
> 
> Yes, for example Node doesn't have event target parents.

http://codereview.chromium.org/10332144/diff/5002/lib/dom/src/native_EventsIm...
File lib/dom/src/native_EventsImplementation.dart (right):

http://codereview.chromium.org/10332144/diff/5002/lib/dom/src/native_EventsIm...
lib/dom/src/native_EventsImplementation.dart:17: html.EventListenerList
_get(String type) {
Cannot you use this[name] or super[name] to achieve the same semantics?

On 2012/05/16 12:32:24, podivilov wrote:
> On 2012/05/15 14:54:57, antonmuhin wrote:
> > why auxiliary _get?
> 
> It is used by subclasses.

http://codereview.chromium.org/10332144/diff/5002/lib/dom/src/native_EventsIm...
lib/dom/src/native_EventsImplementation.dart:36: // TODO(jacobr): switch to
<EventListenerWrapper>[] when the VM allow it.
On 2012/05/16 12:32:24, podivilov wrote:
> On 2012/05/15 14:54:57, antonmuhin wrote:
> > nit: at least in the rest of Dart code : is usually on the next line.
> Done.
> > And <T>[] should be supported now, I think
> Could you please point a revision where it was added?
> 

Sorry, no, but I believe we used it a lot previous year.

http://codereview.chromium.org/10332144/diff/5003/lib/dom/scripts/systemhtml.py
File lib/dom/scripts/systemhtml.py (right):

http://codereview.chromium.org/10332144/diff/5003/lib/dom/scripts/systemhtml....
lib/dom/scripts/systemhtml.py:461: 'No known html event name for event: ' +
event_name
nit: something like 'No html event with name %s' % event_name might be a better
variant.  up to you though.

http://codereview.chromium.org/10332144/diff/5003/lib/dom/scripts/systemnativ...
File lib/dom/scripts/systemnative.py (right):

http://codereview.chromium.org/10332144/diff/5003/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:322: if self._interface.id ==
'DocumentFragment':
may you add a comment why this is special-cased?

http://codereview.chromium.org/10332144/diff/5003/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:330: 
nit: intentional two blanks?

http://codereview.chromium.org/10332144/diff/5003/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:332: def is_event_target(interface):
q: is it a correct style for nest functions?  just asking

http://codereview.chromium.org/10332144/diff/5003/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:348: for attribute in interface.attributes:
as a variant: return any((attr.type.id == 'EventListener' for attr in
interface.attributes))

[podivilov, 2012-05-16 15:19:44]

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/systemnativ...
File lib/dom/scripts/systemnative.py (right):

http://codereview.chromium.org/10332144/diff/5002/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:343: parent =
_ParentWithEventAttributes(self._system._database, self._interface) or ''
On 2012/05/16 13:17:56, antonmuhin wrote:
> In this case, where are the guards?
> 
> On 2012/05/16 12:32:24, podivilov wrote:
> > On 2012/05/15 14:54:57, antonmuhin wrote:
> > > can it be ever a null?
> > 
> > Yes, for example Node doesn't have event target parents.
> 

Note or ''. And see new version.

http://codereview.chromium.org/10332144/diff/5002/lib/dom/src/native_EventsIm...
File lib/dom/src/native_EventsImplementation.dart (right):

http://codereview.chromium.org/10332144/diff/5002/lib/dom/src/native_EventsIm...
lib/dom/src/native_EventsImplementation.dart:17: html.EventListenerList
_get(String type) {
On 2012/05/16 13:17:56, antonmuhin wrote:
> Cannot you use this[name] or super[name] to achieve the same semantics?
> 
> On 2012/05/16 12:32:24, podivilov wrote:
> > On 2012/05/15 14:54:57, antonmuhin wrote:
> > > why auxiliary _get?
> > 
> > It is used by subclasses.
> 

Done.

http://codereview.chromium.org/10332144/diff/5002/lib/dom/src/native_EventsIm...
lib/dom/src/native_EventsImplementation.dart:36: // TODO(jacobr): switch to
<EventListenerWrapper>[] when the VM allow it.
On 2012/05/16 13:17:56, antonmuhin wrote:
> On 2012/05/16 12:32:24, podivilov wrote:
> > On 2012/05/15 14:54:57, antonmuhin wrote:
> > > nit: at least in the rest of Dart code : is usually on the next line.
> > Done.
> > > And <T>[] should be supported now, I think
> > Could you please point a revision where it was added?
> > 
> 
> Sorry, no, but I believe we used it a lot previous year.

Done.

http://codereview.chromium.org/10332144/diff/5003/lib/dom/scripts/systemnativ...
File lib/dom/scripts/systemnative.py (right):

http://codereview.chromium.org/10332144/diff/5003/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:322: if self._interface.id ==
'DocumentFragment':
On 2012/05/16 13:17:57, antonmuhin wrote:
> may you add a comment why this is special-cased?

Done.

http://codereview.chromium.org/10332144/diff/5003/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:330: 
On 2012/05/16 13:17:57, antonmuhin wrote:
> nit: intentional two blanks?

Done.

http://codereview.chromium.org/10332144/diff/5003/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:332: def is_event_target(interface):
On 2012/05/16 13:17:57, antonmuhin wrote:
> q: is it a correct style for nest functions?  just asking

Done.

http://codereview.chromium.org/10332144/diff/5003/lib/dom/scripts/systemnativ...
lib/dom/scripts/systemnative.py:348: for attribute in interface.attributes:
On 2012/05/16 13:17:57, antonmuhin wrote:
> as a variant: return any((attr.type.id == 'EventListener' for attr in
> interface.attributes))

Done.