Refactor the event-generation code to locate it in systemhtml.py.

client/dom/scripts/systemhtml.py

 #!/usr/bin/python
 # Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 # for details. All rights reserved. Use of this source code is governed by a
 # BSD-style license that can be found in the LICENSE file.
 
 """This module provides shared functionality for the system to generate
 Dart:html APIs from the IDL database."""
 
 from systemfrog import *
 from systeminterface import *
@@ skipping 365 matching lines @@
   'waiting': 'waiting',
   'webkitAnimationEnd': 'animationEnd',
   'webkitAnimationIteration': 'animationIteration',
   'webkitAnimationStart': 'animationStart',
   'webkitfullscreenchange': 'fullscreenChange',
   'webkitfullscreenerror': 'fullscreenError',
   'webkitSpeechChange': 'speechChange',
   'webkitTransitionEnd': 'transitionEnd'
 }
 
+# These classes require an explicit declaration for the "on" method even though
+# they don't declare any unique events, because the concrete class hierarchy
+# doesn't match the interface hierarchy.
+_html_explicit_event_classes = set(['DocumentFragment'])
+
 def _OnAttributeToEventName(on_method):
   event_name = on_method.id[2:]
   if event_name in _on_attribute_to_event_name_mapping:
     return _on_attribute_to_event_name_mapping[event_name]
   else:
     return event_name
 
 def _DomToHtmlEvents(interface_id, events):
   event_names = set(map(_OnAttributeToEventName, events)) 
   if interface_id in _html_manual_events:
@@ skipping 71 matching lines @@
   def _TraverseParents(self, interface, callback):
     for parent in interface.parents:
       parent_id = parent.type.id
       if self._database.HasInterface(parent_id):
         parent_interface = self._database.GetInterface(parent_id)
         callback(parent_interface)
         self._TraverseParents(parent_interface, callback)
 
   # TODO(jacobr): this isn't quite right.... 
   def GetParentsEventsClasses(self, interface):
-    # Ugly hack as we don't specify that Document inherits from Element
-    # in our IDL.
-    if interface.id == 'Document':
+    # Ugly hack as we don't specify that Document and DocumentFragment inherit
+    # from Element in our IDL.
+    if interface.id == 'Document' or interface.id == 'DocumentFragment':
       return ['ElementEvents']
 
     interfaces_with_events = set()
     def visit(parent):
       if parent.id in self._event_classes:
         interfaces_with_events.add(parent)
 
     self._TraverseParents(interface, visit)
     if len(interfaces_with_events) == 0:
       return ['Events']
     else:
       names = []
       for interface in interfaces_with_events:
         names.append(interface.id + 'Events')
       return names
 
+  def GetOneParentEventsClass(self, interface):

[Jacob, 2012/03/08 21:45:11]

Nit: remove the word "One" from this method name.

[nweiz, 2012/03/08 22:14:45]

Done.

+    parent_event_classes = self.GetParentsEventsClasses(interface)
+    if len(parent_event_classes) != 1:
+      raise Exception('Only one parent event class allowed ' + interface.id)
+    return parent_event_classes[0]
+
   def _ImplClassName(self, type_name):
     return '_' + type_name + 'Impl'
 
+  def GetEventAttributes(self, interface):

[sra1, 2012/03/08 21:58:59]

Since this returns two distinct falsy values, please add comment.

[nweiz, 2012/03/08 22:14:45]

Done.

+    events =  set([attr for attr in interface.attributes
+                   if self._generator._IsEventAttribute(interface, attr)])
+
+    if events or interface.id in _html_explicit_event_classes:
+      return events

[sra1, 2012/03/08 21:58:59]

Add 'return None'
Implicit return of None looks like a mistake.

[nweiz, 2012/03/08 22:14:45]

Done.

+
 class HtmlSystem(System):
 
   def __init__(self, templates, database, emitters, output_dir, generator):
     super(HtmlSystem, self).__init__(
         templates, database, emitters, output_dir)
     self._shared = HtmlSystemShared(database, generator)
 
 class HtmlInterfacesSystem(HtmlSystem):
 
   def __init__(self, templates, database, emitters, output_dir, generator):
@@ skipping 112 matching lines @@
       self._members_emitter.Emit(
           '\n'
           '  $CTOR(int length);\n'
           '\n'
           '  $CTOR.fromList(List<$TYPE> list);\n'
           '\n'
           '  $CTOR.fromBuffer(ArrayBuffer buffer);\n',
         CTOR=self._interface.id,
         TYPE=DartType(element_type))
 
+    events = self._shared.GetEventAttributes(self._interface)
+    if events == set():

[sra1, 2012/03/08 21:58:59]

This is slightly confusing, since pythonic style is "if events:"

Perhaps:

  if events is not None:
    if events:
    else:

[nweiz, 2012/03/08 22:14:45]

Why is explicitly comparing to None less confusing than explicitly comparing to
set()?

[Jacob, 2012/03/08 22:33:38]

I think the issue is that making code behave differently for an empty set() and
None is inherently confusing.  Perhaps it would be better to just add a second
variable that tracks whether events should be generated?

On 2012/03/08 22:14:45, nweiz wrote:

[nweiz, 2012/03/08 22:52:45]

Good idea. Done.

[sra1, 2012/03/08 23:07:32]

Idea 1:
Do we need to emit the no-added-methods on-getter at all?

Idea 2:
Push the login into AddEventAttributes.  It calls GetOneParentEventsClass
anyway.

events = self._shared...
if events is not None:
  self.AddEventAttributes(events)


If stay like this:

There are three actions here, one is to do nothing.  'do-nothing' is at a
coarser level of distinction, so it might be clearer when expressed as
if-without-{else,elif}.  Your code is:

detail1
detail2
[implicit !feature]

I'm suggesting:

feature
  detail1
  detail2

This is not a *huge* issue, but I stumbled when reading the original; it just
looked odd.  I thought 'huh?' and had to go and read GetEventAttributes to see
that the value is not always a set.
There is clearly something not quite 100% perfect because Jacob was confused
too.

Perhaps the answer is to make both falsy values explicit, so it is clear we are
dealing with a nullable possibly empty collection.

if events is not None:
  if events == set():
  else:

if events is not None:
  if events:
  else: # empty set of events means we can inherit the events.

+      self._EmitEventGetter(
+          self._shared.GetOneParentEventsClass(self._interface))
+    elif events:
+      self.AddEventAttributes(events)
+
   def AddAttribute(self, getter, setter):
     html_getter_name = self._shared.RenameInHtmlLibrary(
       self._interface, getter.id, 'get:')
     html_setter_name = self._shared.RenameInHtmlLibrary(
       self._interface, getter.id, 'set:')
 
     if not html_getter_name:
       getter = None
     if not html_setter_name:
       setter = None
@@ skipping 34 matching lines @@
   def FinishInterface(self):
     pass
 
   def AddConstant(self, constant):
     self._EmitConstant(self._members_emitter, constant)
 
   def AddEventAttributes(self, event_attrs):
     event_attrs = _DomToHtmlEvents(self._interface.id, event_attrs)
     self._shared._event_classes.add(self._interface.id)
     events_interface = self._interface.id + 'Events'
-    self._members_emitter.Emit('\n  $TYPE get on();\n',
-                               TYPE=events_interface)
+    self._EmitEventGetter(events_interface)
+
     events_members = self._emitter.Emit(
         '\ninterface $INTERFACE extends $PARENTS {\n$!MEMBERS}\n',
         INTERFACE=events_interface,
         PARENTS=', '.join(
             self._shared.GetParentsEventsClasses(self._interface)))
 
     for event_name in event_attrs:
       if event_name in _html_event_names:
         events_members.Emit('\n  EventListenerList get $NAME();\n',
           NAME=_html_event_names[event_name])
       else:
         raise Exception('No known html even name for event: ' + event_name)
 
+  def _EmitEventGetter(self, interface):

[sra1, 2012/03/08 23:07:32]

can we call this events_interface for uniformity with above and to avoid
potential confusion that self._interface might be the same?

+    self._members_emitter.Emit('\n  $TYPE get on();\n',
+                               TYPE=interface)
+
 # ------------------------------------------------------------------------------
 
 # TODO(jmesserly): inheritance is probably not the right way to factor this long
 # term, but it makes merging better for now.
 class HtmlFrogClassGenerator(FrogInterfaceGenerator):
   """Generates a Frog class for the dart:html library from a DOM IDL
   interface.
   """
 
   def __init__(self, system, interface, template, super_interface, dart_code,
@@ skipping 41 matching lines @@
         NATIVESPEC=' native "' + native_spec + '"')
 
     if element_type:
       self.AddTypedArrayConstructors(element_type)
 
     # Emit a factory provider class for the constructor.
     constructor_info = AnalyzeConstructor(interface)
     if constructor_info:
       self._EmitFactoryProvider(interface_name, constructor_info)
 
+    events = self._shared.GetEventAttributes(self._interface)
+    if events == set():
+      self._EmitEventGetter(
+          self._shared.GetOneParentEventsClass(self._interface))
+    elif events:
+      self.AddEventAttributes(events)
+
   def _EmitFactoryProvider(self, interface_name, constructor_info):
     template_file = 'factoryprovider_%s.darttemplate' % interface_name
     template = self._system._templates.TryLoad(template_file)
     if not template:
       template = self._system._templates.Load('factoryprovider.darttemplate')
 
     factory_provider = '_' + interface_name + 'FactoryProvider'
     emitter = self._system._ImplFileEmitter(factory_provider)
     emitter.Emit(
         template,
@@ skipping 208 matching lines @@
           '  $TYPE $NAME($PARAMS) native;\n',
           TYPE=self._NarrowOutputType(info.type_name),
           NAME=info.name,
           PARAMS=info.ParametersImplementationDeclaration(
               lambda type_name: self._NarrowInputType(type_name)))
 
   def AddEventAttributes(self, event_attrs):
     event_attrs = _DomToHtmlEvents(self._interface.id, event_attrs)
     events_class = '_' + self._interface.id + 'EventsImpl'
     events_interface = self._interface.id + 'Events'
-    self._members_emitter.Emit(
-        '\n  $TYPE get on() =>\n    new $TYPE($EVENTTARGET);\n',
-        TYPE=events_class,
-        EVENTTARGET='_jsDocument' if self._interface.id == 'Document'
-            else 'this')
+    self._EmitEventGetter(events_class)
 
     self._shared._event_classes.add(self._interface.id)
 
-    parent_event_classes = self._shared.GetParentsEventsClasses(
-        self._interface)
-    if len(parent_event_classes) != 1:
-      raise Exception('Only one parent event class allowed '
-          + self._interface.id)
+    parent_event_class = self._shared.GetOneParentEventsClass(self._interface)
 
     # TODO(jacobr): specify the type of _ptr as EventTarget
     events_members = self._dart_code.Emit(
         '\n'
         'class $CLASSNAME extends $SUPER implements $INTERFACE {\n'
         '  $CLASSNAME(_ptr) : super(_ptr);\n'
         '$!MEMBERS}\n',
         CLASSNAME=events_class,
         INTERFACE=events_interface,
-        SUPER='_' + parent_event_classes[0] + 'Impl')
+        SUPER='_' + parent_event_class + 'Impl')
 
     for event_name in event_attrs:
       if event_name in _html_event_names:
         events_members.Emit(
             "\n"
             "  EventListenerList get $NAME() => _get('$RAWNAME');\n",
             RAWNAME=event_name,
             NAME=_html_event_names[event_name])
       else:
         raise Exception('No known html even name for event: ' + event_name)
 
+  def _EmitEventGetter(self, interface):

[sra1, 2012/03/08 23:07:32]

interface -> events_class ?

+    self._members_emitter.Emit(
+        '\n  $TYPE get on() =>\n    new $TYPE($EVENTTARGET);\n',
+        TYPE=interface,
+        EVENTTARGET='_jsDocument' if self._interface.id == 'Document'
+            else 'this')
+
 # ------------------------------------------------------------------------------
 
 class HtmlFrogSystem(HtmlSystem):
 
   def __init__(self, templates, database, emitters, output_dir, generator):
     super(HtmlFrogSystem, self).__init__(
         templates, database, emitters, output_dir, generator)
     self._dart_frog_file_paths = []
 
 
@@ skipping 197 matching lines @@
     if dom_name != 'Document':
       self._members_emitter.Emit(
           '  $(CLASSNAME)._wrap(ptr) : super._wrap(ptr);\n',
           CLASSNAME=self._class_name)
 
     # Emit a factory provider class for the constructor.
     constructor_info = AnalyzeConstructor(interface)
     if constructor_info:
       self._EmitFactoryProvider(interface_name, constructor_info)
 
+    events = self._shared.GetEventAttributes(self._interface)
+    if events == set():
+      self._EmitEventGetter(
+          self._shared.GetOneParentEventsClass(self._interface))
+    elif events:
+      self.AddEventAttributes(events)
+
   def _EmitFactoryProvider(self, interface_name, constructor_info):
     template_file = 'factoryprovider_%s.darttemplate' % interface_name
     template = self._system._templates.TryLoad(template_file)
     if not template:
       template = self._system._templates.Load('factoryprovider.darttemplate')
 
     factory_provider = '_' + interface_name + 'FactoryProvider'
     emitter = self._system._ImplFileEmitter(factory_provider)
     emitter.Emit(
         template,
@@ skipping 96 matching lines @@
     self.AddAttribute(getter, setter)
 
   def AddSecondaryOperation(self, interface, info):
     self._SecondaryContext(interface)
     self.AddOperation(info)
 
   def AddEventAttributes(self, event_attrs):
     event_attrs = _DomToHtmlEvents(self._interface.id, event_attrs)
     events_class = '_' + self._interface.id + 'EventsImpl'
     events_interface = self._interface.id + 'Events'
-    self._members_emitter.Emit(
-        '\n'
-        '  $TYPE get on() {\n'
-        '    if (_on == null) _on = new $TYPE($EVENTTARGET);\n'
-        '    return _on;\n'
-        '  }\n',
-        TYPE=events_class,
-        EVENTTARGET='_wrappedDocumentPtr' if self._interface.id == 'Document'
-            else 'this')
+    self._EmitEventGetter(events_class)
 
     self._shared._event_classes.add(self._interface.id)
 
-    parent_event_classes = self._shared.GetParentsEventsClasses(
-        self._interface)
-    if len(parent_event_classes) != 1:
-      raise Exception('Only one parent event class allowed '
-          + self._interface.id)
+    parent_event_class = self._shared.GetOneParentEventsClass(self._interface)
 
     # TODO(jacobr): specify the type of _ptr as EventTarget
     events_members = self._dart_code.Emit(
         '\n'
         'class $CLASSNAME extends $SUPER implements $INTERFACE {\n'
         '  $CLASSNAME(_ptr) : super(_ptr);\n'
         '$!MEMBERS}\n',
         CLASSNAME=events_class,
         INTERFACE=events_interface,
-        SUPER='_' + parent_event_classes[0] + 'Impl')
+        SUPER='_' + parent_event_class + 'Impl')
 
     for event_name in event_attrs:
       if event_name in _html_event_names:
         events_members.Emit(
             "\n"
             "  EventListenerList get $NAME() => _get('$RAWNAME');\n",
             RAWNAME=event_name,
             NAME=_html_event_names[event_name])
       else:
         raise Exception('No known html even name for event: ' + event_name)
 
+  def _EmitEventGetter(self, events_class):
+    self._members_emitter.Emit(
+        '\n'
+        '  $TYPE get on() {\n'
+        '    if (_on == null) _on = new $TYPE($EVENTTARGET);\n'
+        '    return _on;\n'
+        '  }\n',
+        TYPE=events_class,
+        EVENTTARGET='_wrappedDocumentPtr' if self._interface.id == 'Document'
+            else 'this')
+
   def _SecondaryContext(self, interface):
     if interface is not self._current_secondary_parent:
       self._current_secondary_parent = interface
       self._members_emitter.Emit('\n  // From $WHERE\n', WHERE=interface.id)
 
   # TODO(jacobr): change this to more directly match the frog version.
   def AddIndexer(self, element_type):
     """Adds all the methods required to complete implementation of List."""
     # We would like to simply inherit the implementation of everything except
     # get length(), [], and maybe []=.  It is possible to extend from a base
@@ skipping 311 matching lines @@
     # dispatch has removed f(X), leaving only f(Y), but there is no guarantee
     # that Y = Z-X, so we need to check for Y.
     true_code = emitter.Emit(
         '$(INDENT)if ($COND) {\n'
         '$!TRUE'
         '$(INDENT)}\n',
         COND=test, INDENT=indent)
     self.GenerateDispatch(
         true_code, info, indent + '  ', position + 1, positive)
     return True