Change how events are handled in Effen

Change how events are handled in Effen

This patch removes the mutable API to event handling (Nodes no longer have a events object with which to add listeners).

Instead, a new (non-Render) Node is introduced: EventTarget. This node represents a location in the Effen tree which can handle events as they bubble.

Note that this also changes the implementation to use event delegation (one set of listeners at the sky.document level) rather than direct listeners on leaf nodes.

R=abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/c7db4c71024f3df21475944da8752ec29484131c

[rafaelw, 2015-03-19 05:34:29]

rafaelw@chromium.org changed reviewers:
+ abarth@chromium.org

[rafaelw, 2015-03-19 06:11:26]

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/component...
File sky/framework/components/button_base.dart (right):

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/component...
sky/framework/components/button_base.dart:12: Node buildContent();
I'm tempted to feel like this is the wrong pattern, that we should be using
componentization rather than inheritance for composition. This way is more
compact, though. We can continue to see how it places out.

[abarth-chromium, 2015-03-19 14:19:59]

lgtm

https://codereview.chromium.org/1019633004/diff/20001/sky/examples/stocks-fn/...
File sky/examples/stocks-fn/lib/stock_app.dart (right):

https://codereview.chromium.org/1019633004/diff/20001/sky/examples/stocks-fn/...
sky/examples/stocks-fn/lib/stock_app.dart:61: _menuController = new
PopupMenuController();
Interesting.  I need to study this more.  My intent was that different popup
menus could attach to the same PopupMenuController at different times.  I
probably messed something up somewhere.  Certainly not an issue for this CL.

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/component...
File sky/framework/components/button_base.dart (right):

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/component...
sky/framework/components/button_base.dart:12: Node buildContent();
On 2015/03/19 at 06:11:26, rafaelw wrote:
> I'm tempted to feel like this is the wrong pattern, that we should be using
componentization rather than inheritance for composition. This way is more
compact, though. We can continue to see how it places out.

Yeah.

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart
File sky/framework/fn.dart (right):

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:127: static Map<sky.Node, RenderNode> _nodeMap =
final

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:168: bool _docListenersRegistered = false;
style nit: _docListenersRegistered -> _haveRegisteredListenersOnDocument ?

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:189: listeners['wheel'] = onWheel;
This if-cascade is kind of ugly, but I think you're right that it's valuable to
have the shorthands for the common events.

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:207: listeners['pointerup'] = onPointerUp;
Do you actually want to mutate the |custom| argument?  You might want to make a
copy of the map before mutating it.

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:224: }) : this.content = content,
You can just write |this.content| in the argument list to do this automatically.

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:260: static void _ensureDocListener(String eventType) {
style nit: s/Doc/Document/

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:264: }
Something is off with your indentation here.

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:272: _docListenersRegistered = true;
Do we ever sync the same Node twice?  It seems like we always get the "new"
version in the sync...  Will this flag ever cut off work?

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:276: content = _syncChild(content, oldContent , host,
insertBefore);
s/ ,/,/

[abarth-chromium, 2015-03-19 14:38:41]

I guess the EventTarget can get re-used if its handed into a component and used
in another build.

[rafaelw, 2015-03-19 15:03:51]

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart
File sky/framework/fn.dart (right):

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:127: static Map<sky.Node, RenderNode> _nodeMap =
On 2015/03/19 14:19:59, abarth wrote:
> final

Done.

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:168: bool _docListenersRegistered = false;
On 2015/03/19 14:19:59, abarth wrote:
> style nit: _docListenersRegistered -> _haveRegisteredListenersOnDocument ?

Removed this.

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:189: listeners['wheel'] = onWheel;
On 2015/03/19 14:19:59, abarth wrote:
> This if-cascade is kind of ugly, but I think you're right that it's valuable
to
> have the shorthands for the common events.

Yeah. We could also do this with reflection (i.e. create an annotation with the
event names on the handlers), but I'm not sure that's much better.

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:207: listeners['pointerup'] = onPointerUp;
On 2015/03/19 14:19:59, abarth wrote:
> Do you actually want to mutate the |custom| argument?  You might want to make
a
> copy of the map before mutating it.

Done.

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:224: }) : this.content = content,
On 2015/03/19 14:19:59, abarth wrote:
> You can just write |this.content| in the argument list to do this
automatically.

Not if you want to use content in the initializer (passed into super:key below).
You'll get an error that you can't access |this| in the initializer.

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:260: static void _ensureDocListener(String eventType) {
On 2015/03/19 14:19:59, abarth wrote:
> style nit: s/Doc/Document/

Done.

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:264: }
On 2015/03/19 14:19:59, abarth wrote:
> Something is off with your indentation here.

Done.

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:272: _docListenersRegistered = true;
On 2015/03/19 14:19:59, abarth wrote:
> Do we ever sync the same Node twice?  It seems like we always get the "new"
> version in the sync...  Will this flag ever cut off work?

No, I think you're right. This buys us very little. I'm just going to remove it.
Presumably this work will show up in some future profile and we'll just create a
way to register for all events.

https://codereview.chromium.org/1019633004/diff/20001/sky/framework/fn.dart#n...
sky/framework/fn.dart:276: content = _syncChild(content, oldContent , host,
insertBefore);
On 2015/03/19 14:19:59, abarth wrote:
> s/ ,/,/

Done.

[rafaelw, 2015-03-19 15:05:00]

Committed patchset #3 (id:40001) manually as
c7db4c71024f3df21475944da8752ec29484131c (presubmit successful).