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

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

This also adds some code to enforce the generation of an events class for
DocumentFragment so that it can provide access to all the element events.

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

[Jacob, 2012-03-07 23:24:04]

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

https://chromiumcodereview.appspot.com/9623017/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:508: # Hack to generate no-op Element events
for DocumentFragment.
this comment can now be removed.

https://chromiumcodereview.appspot.com/9623017/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:645: if events != None:
self.AddEventAttributes(events)
I believe we're using the style of placing the if clause on the next line.

https://chromiumcodereview.appspot.com/9623017/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:778: if events != None:
self.AddEventAttributes(events)
why not just
if events:
  self.AddEventAttributes(events)

https://chromiumcodereview.appspot.com/9623017/diff/1/client/html/dartium/htm...
File client/html/dartium/html_dartium.dart (right):

https://chromiumcodereview.appspot.com/9623017/diff/1/client/html/dartium/htm...
client/html/dartium/html_dartium.dart:6957: +class _DocumentFragmentEventsImpl
extends _EventsImpl implements DocumentFragmentEvents {
This DocumentFragmentEvents class should not be generated.

[nweiz, 2012-03-08 01:03:50]

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

https://chromiumcodereview.appspot.com/9623017/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:508: # Hack to generate no-op Element events
for DocumentFragment.
On 2012/03/07 23:24:05, Jacob wrote:
> this comment can now be removed.

Done.

https://chromiumcodereview.appspot.com/9623017/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:645: if events != None:
self.AddEventAttributes(events)
On 2012/03/07 23:24:05, Jacob wrote:
> I believe we're using the style of placing the if clause on the next line.

Done.

https://chromiumcodereview.appspot.com/9623017/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:778: if events != None:
self.AddEventAttributes(events)
On 2012/03/07 23:24:05, Jacob wrote:
> why not just
> if events:
>   self.AddEventAttributes(events)

Because we want to run AddEventAttributes even if events is empty.

https://chromiumcodereview.appspot.com/9623017/diff/1/client/html/dartium/htm...
File client/html/dartium/html_dartium.dart (right):

https://chromiumcodereview.appspot.com/9623017/diff/1/client/html/dartium/htm...
client/html/dartium/html_dartium.dart:6957: +class _DocumentFragmentEventsImpl
extends _EventsImpl implements DocumentFragmentEvents {
On 2012/03/07 23:24:05, Jacob wrote:
> This DocumentFragmentEvents class should not be generated.

Done.

[Jacob, 2012-03-08 18:38:09]

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

https://chromiumcodereview.appspot.com/9623017/diff/1/client/dom/scripts/syst...
client/dom/scripts/systemhtml.py:778: if events != None:
self.AddEventAttributes(events)
On 2012/03/08 01:03:51, nweiz wrote:
> On 2012/03/07 23:24:05, Jacob wrote:
> > why not just
> > if events:
> >   self.AddEventAttributes(events)
> 
> Because we want to run AddEventAttributes even if events is empty.

same comment as above for all cases where == set() is used

https://chromiumcodereview.appspot.com/9623017/diff/4001/client/dom/scripts/s...
File client/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/9623017/diff/4001/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:644: if events == set():
writing 
if events == set(): 
looks really odd
can you write
if len(events) > 0: 
  self.AddEventAttributes(events)
else:
  self.AddParentEvent()

instead?

https://chromiumcodereview.appspot.com/9623017/diff/4001/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:1020: def AddParentEvent(self):
seems like there is duplicated code in AddParentEvent and AddEventAttributes.
Can you unify?

https://chromiumcodereview.appspot.com/9623017/diff/4001/client/html/generate...
File client/html/generated/html/dartium/DocumentFragment.dart (right):

https://chromiumcodereview.appspot.com/9623017/diff/4001/client/html/generate...
client/html/generated/html/dartium/DocumentFragment.dart:5: Events get on() {
This isn't right.
It should
be
_ElementEventsImpl get on() {
  ... 
}
  
}

[nweiz, 2012-03-08 20:47:15]

https://chromiumcodereview.appspot.com/9623017/diff/4001/client/dom/scripts/s...
File client/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/9623017/diff/4001/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:644: if events == set():
On 2012/03/08 18:38:09, Jacob wrote:
> writing 
> if events == set(): 
> looks really odd
> can you write
> if len(events) > 0: 
>   self.AddEventAttributes(events)
> else:
>   self.AddParentEvent()
> 
> instead?

len(None) doesn't work, and I don't want to make the if statement have an extra
branch unnecessarily by checking for None early. "== set()" best expresses the
semantics here.

https://chromiumcodereview.appspot.com/9623017/diff/4001/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:1020: def AddParentEvent(self):
On 2012/03/08 18:38:09, Jacob wrote:
> seems like there is duplicated code in AddParentEvent and AddEventAttributes.
> Can you unify?

Done.

https://chromiumcodereview.appspot.com/9623017/diff/4001/client/html/generate...
File client/html/generated/html/dartium/DocumentFragment.dart (right):

https://chromiumcodereview.appspot.com/9623017/diff/4001/client/html/generate...
client/html/generated/html/dartium/DocumentFragment.dart:5: Events get on() {
On 2012/03/08 18:38:09, Jacob wrote:
> This isn't right.
> It should
> be
> _ElementEventsImpl get on() {
>   ... 
> }
>   
> }

Done.

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

lgtm

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
File client/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:501: def GetOneParentEventsClass(self,
interface):
Nit: remove the word "One" from this method name.

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

I'll let Jacob comment on the generated code.
I have a few comments on the python code - lgtm if you agree

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
File client/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:510: def GetEventAttributes(self, interface):
Since this returns two distinct falsy values, please add comment.

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:515: return events
Add 'return None'
Implicit return of None looks like a mistake.

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:650: if events == set():
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]

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
File client/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:501: def GetOneParentEventsClass(self,
interface):
On 2012/03/08 21:45:11, Jacob wrote:
> Nit: remove the word "One" from this method name.

Done.

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:510: def GetEventAttributes(self, interface):
On 2012/03/08 21:58:59, sra1 wrote:
> Since this returns two distinct falsy values, please add comment.

Done.

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:515: return events
On 2012/03/08 21:58:59, sra1 wrote:
> Add 'return None'
> Implicit return of None looks like a mistake.

Done.

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:650: if events == set():
On 2012/03/08 21:58:59, sra1 wrote:
> This is slightly confusing, since pythonic style is "if events:"
> 
> Perhaps:
> 
>   if events is not None:
>     if events:
>     else:

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

[Jacob, 2012-03-08 22:33:37]

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
File client/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:650: if events == set():
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:
> On 2012/03/08 21:58:59, sra1 wrote:
> > This is slightly confusing, since pythonic style is "if events:"
> > 
> > Perhaps:
> > 
> >   if events is not None:
> >     if events:
> >     else:
> 
> Why is explicitly comparing to None less confusing than explicitly comparing
to
> set()?

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

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
File client/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:650: if events == set():
On 2012/03/08 22:33:38, Jacob wrote:
> 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?

Good idea. Done.

[Jacob, 2012-03-08 22:54:18]

lgtm

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

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
File client/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:650: if events == set():
On 2012/03/08 22:14:45, nweiz wrote:
> On 2012/03/08 21:58:59, sra1 wrote:
> > This is slightly confusing, since pythonic style is "if events:"
> > 
> > Perhaps:
> > 
> >   if events is not None:
> >     if events:
> >     else:
> 
> Why is explicitly comparing to None less confusing than explicitly comparing
to
> set()?

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.

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:725: def _EmitEventGetter(self, interface):
can we call this events_interface for uniformity with above and to avoid
potential confusion that self._interface might be the same?

https://chromiumcodereview.appspot.com/9623017/diff/9002/client/dom/scripts/s...
client/dom/scripts/systemhtml.py:1051: def _EmitEventGetter(self, interface):
interface -> events_class ?