Make the MediaQueryList listener an EventListener

Make the MediaQueryList listener an EventListener

The spec changed from a callback to using DOM events, update
our code accordingly. See the note at
http://dev.w3.org/csswg/cssom-view/#dom-mediaquerylist-removelistener
for how this is compatible with existing content.

BUG=375658
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181124

[cbiesinger, 2014-07-16 22:15:23]

Hmm, do I need to make this an ActiveDOMObject now?

[haraken, 2014-07-17 01:47:26]

The bindings/ looks good.

Can we split the change to implement MediaQueryListEvent out of this CL?

https://codereview.chromium.org/396283004/diff/40001/Source/core/css/MediaQue...
File Source/core/css/MediaQueryList.idl (right):

https://codereview.chromium.org/396283004/diff/40001/Source/core/css/MediaQue...
Source/core/css/MediaQueryList.idl:26: void removeListener([Default=Undefined]
optional EventListener listener);

Nit: This is mis-speced... this should have been named
addEventListener/removeEventListener.

[cbiesinger, 2014-07-17 03:22:41]

On 2014/07/17 01:47:26, haraken wrote:
> The bindings/ looks good.
> 
> Can we split the change to implement MediaQueryListEvent out of this CL?

Hm, yeah, I really should split this into more CLs. Will do that tomorrow.

>
https://codereview.chromium.org/396283004/diff/40001/Source/core/css/MediaQue...
> File Source/core/css/MediaQueryList.idl (right):
> 
>
https://codereview.chromium.org/396283004/diff/40001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryList.idl:26: void removeListener([Default=Undefined]
> optional EventListener listener);
> 
> Nit: This is mis-speced... this should have been named
> addEventListener/removeEventListener.

This is only for historical reasons :( The spec used to have a special callback
function but then got changed to use DOM events. This should all be compatible
with existing web content, except that we need an addListener function that just
forwards to addEventListener :/

But I should add a comment explaining that!

[cbiesinger, 2014-07-17 03:27:57]

(see the note at
http://dev.w3.org/csswg/cssom-view/#dom-mediaquerylist-removelistener for some
more details; I've updated the CL description a bit too)

[cbiesinger, 2014-07-17 20:41:20]

I've now split this up into more CLs, and rebased this CL so that it depends on
https://codereview.chromium.org/404563003/ and
https://codereview.chromium.org/400043002/

[cbiesinger, 2014-07-18 00:01:33]

On 2014/07/16 22:15:23, cbiesinger wrote:
> Hmm, do I need to make this an ActiveDOMObject now?

I did do that in https://codereview.chromium.org/397403002/. Once that lands,
I'll have to update this CL a bit.

[haraken, 2014-07-18 01:15:38]

Thanks for the split, but I'm not really happy about landing code that is not
used and thus not tested.

Would it be possible to split the CL into pieces so that each piece is actually
used and has a test case?

[cbiesinger, 2014-07-19 01:19:08]

On 2014/07/18 01:15:38, haraken wrote:
> Thanks for the split, but I'm not really happy about landing code that is not
> used and thus not tested.
> 
> Would it be possible to split the CL into pieces so that each piece is
actually
> used and has a test case?

Hmm, I don't think there's really a good way to do that. I mean, after
https://codereview.chromium.org/397403002/ the code will be a little simpler,
but beyond that I can't think of a good way to split this up...

[cbiesinger, 2014-07-19 01:23:09]

On 2014/07/19 01:19:08, cbiesinger wrote:
> On 2014/07/18 01:15:38, haraken wrote:
> > Thanks for the split, but I'm not really happy about landing code that is
not
> > used and thus not tested.
> > 
> > Would it be possible to split the CL into pieces so that each piece is
> actually
> > used and has a test case?
> 
> Hmm, I don't think there's really a good way to do that. I mean, after
> https://codereview.chromium.org/397403002/ the code will be a little simpler,
> but beyond that I can't think of a good way to split this up...

So now I uploaded a self-contained version again.

[haraken, 2014-07-20 05:14:49]

I understand it's hard to split this CL... I'm sorry about taking your time on
the split.

https://codereview.chromium.org/396283004/diff/180001/Source/bindings/templat...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/396283004/diff/180001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:290: {% if interface_name ==
'MediaQueryList' and method.name in ('addListener',

Why does the MediaQueryList need separate code from other EventTargets? The only
difference between MediaQueryList and other EventTargets are the line 287 and
the line 294. Can't we get rid of the difference and share code between them?

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.cpp (right):

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:72: void
MediaQueryList::addMediaQueryListListener(PassRefPtrWillBeRawPtr<MediaQueryListListener>
listener)

Just help me understand: Why do we want to use m_listeners for
addMediaQueryListListener/removeMediaQueryListListener whereas we redirect
addListener/removeListener to EventTarget::addEventListener/removeEventListener?

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:107: removeAllEventListeners();

This will remove all event listeners other than onchange listeners. Is it
expected?

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:139:
EventTargetWithInlineData::trace(visitor);

Nit: We normally make this a tail call of the trace() method.

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.h (right):

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.h:46: REFCOUNTED_EVENT_TARGET(MediaQueryList);

You can use DEFINE_EVENT_TARGET_REFCOUNTING_WILL_BE_REMOVED. Then you don't need
the #if flag.

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.h:61: void
removeListener(PassRefPtr<EventListener>);

Shall we rename these methods to addDeprecatedListener/removeDeprecatedListener?
In an IDL file, you can write:

  [ImplementedAs=addDeprecatedListener] void addListener();

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryMatcher.cpp (right):

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
Source/core/css/MediaQueryMatcher.cpp:114:
m_document->enqueueUniqueAnimationFrameEvent(event);

I'm not quite sure of this. I wonder why MediaqueryListEvent wants to use
per-animation-frame events whereas other events are using
not-per-animation-frame events. Elliott would be the best reviewer on this.

[cbiesinger, 2014-07-21 22:07:19]

Changes made. I've also simplified MediaQueryListListener now that it does not
have to handle JavaScript.

https://codereview.chromium.org/396283004/diff/180001/Source/bindings/templat...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/396283004/diff/180001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:290: {% if interface_name ==
'MediaQueryList' and method.name in ('addListener',
On 2014/07/20 05:14:49, haraken wrote:
> 
> Why does the MediaQueryList need separate code from other EventTargets? The
only
> difference between MediaQueryList and other EventTargets are the line 287 and
> the line 294. Can't we get rid of the difference and share code between them?

Done.

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.cpp (right):

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:72: void
MediaQueryList::addMediaQueryListListener(PassRefPtrWillBeRawPtr<MediaQueryListListener>
listener)
On 2014/07/20 05:14:49, haraken wrote:
> 
> Just help me understand: Why do we want to use m_listeners for
> addMediaQueryListListener/removeMediaQueryListListener whereas we redirect
> addListener/removeListener to
EventTarget::addEventListener/removeEventListener?

I've been told that using DOM events for C++ listeners is discouraged;
consequently, I'm keeping a C++-specific way to add listeners. But I'm happy to
change that if it's fine to have C++ DOM event listeners.

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:107: removeAllEventListeners();
On 2014/07/20 05:14:49, haraken wrote:
> 
> This will remove all event listeners other than onchange listeners. Is it
> expected?

By my understanding of ActiveDOMEvent::stop that is expected, but I admit that I
don't understand that interface very well.

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:139:
EventTargetWithInlineData::trace(visitor);
On 2014/07/20 05:14:49, haraken wrote:
> 
> Nit: We normally make this a tail call of the trace() method.

Done.

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.h (right):

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.h:46: REFCOUNTED_EVENT_TARGET(MediaQueryList);
On 2014/07/20 05:14:49, haraken wrote:
> 
> You can use DEFINE_EVENT_TARGET_REFCOUNTING_WILL_BE_REMOVED. Then you don't
need
> the #if flag.

Hm... then I'd have to duplicate the superclass name here. I think I prefer the
#if over the other ugliness.

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.h:61: void
removeListener(PassRefPtr<EventListener>);
On 2014/07/20 05:14:49, haraken wrote:
> 
> Shall we rename these methods to
addDeprecatedListener/removeDeprecatedListener?
> In an IDL file, you can write:
> 
>   [ImplementedAs=addDeprecatedListener] void addListener();

That's a great suggestion! Thanks, done.

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryMatcher.cpp (right):

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
Source/core/css/MediaQueryMatcher.cpp:114:
m_document->enqueueUniqueAnimationFrameEvent(event);
On 2014/07/20 05:14:49, haraken wrote:
> 
> I'm not quite sure of this. I wonder why MediaqueryListEvent wants to use
> per-animation-frame events whereas other events are using
> not-per-animation-frame events. Elliott would be the best reviewer on this.

Yeah, hmm, I'm not sure... come to think of it, with the way I do this, you only
get the information (.matches) of the first event per a frame, so maybe this
should be changed. Elliott, any thoughts?

[haraken, 2014-07-22 01:30:14]

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.cpp (right):

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:72: void
MediaQueryList::addMediaQueryListListener(PassRefPtrWillBeRawPtr<MediaQueryListListener>
listener)
On 2014/07/21 22:07:19, cbiesinger wrote:
> On 2014/07/20 05:14:49, haraken wrote:
> > 
> > Just help me understand: Why do we want to use m_listeners for
> > addMediaQueryListListener/removeMediaQueryListListener whereas we redirect
> > addListener/removeListener to
> EventTarget::addEventListener/removeEventListener?
> 
> I've been told that using DOM events for C++ listeners is discouraged;
> consequently, I'm keeping a C++-specific way to add listeners. But I'm happy
to
> change that if it's fine to have C++ DOM event listeners.

I think Elliott would know better.

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.h (right):

https://codereview.chromium.org/396283004/diff/180001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.h:46: REFCOUNTED_EVENT_TARGET(MediaQueryList);
On 2014/07/21 22:07:19, cbiesinger wrote:
> On 2014/07/20 05:14:49, haraken wrote:
> > 
> > You can use DEFINE_EVENT_TARGET_REFCOUNTING_WILL_BE_REMOVED. Then you don't
> need
> > the #if flag.
> 
> Hm... then I'd have to duplicate the superclass name here. I think I prefer
the
> #if over the other ugliness.

We normally duplicate the superclass name... probably we should invent a better
macro for this (in a follow-up CL).

https://code.google.com/p/chromium/codesearch#search/&q=DEFINE_EVENT_TARGET_R...

[esprehn, 2014-08-01 14:15:10]

Is this ready for review? I'm not sure the state of all the broken up patches.

[haraken, 2014-08-01 14:38:53]

This is ready for review.

[cbiesinger, 2014-08-06 23:31:13]

Maybe I could move the bindings cleanup to a separate CL, if that would help?

[haraken, 2014-08-06 23:53:29]

On 2014/08/06 23:31:13, cbiesinger wrote:
> Maybe I could move the bindings cleanup to a separate CL, if that would help?

I just have a question to Elliott (mentioned in #14). Other parts mostly look
good.

[esprehn, 2014-08-06 23:59:56]

We should use a separate interface for C++ listeners. DOM events are dangerous
because the string type has no relation to the event type. Specifically if C++
has a listener on a MediaList object, and the change event fired a
MediaChangeEvent object, someone could do list.dispatchEvent(new
CustomEvent("change")) and the C++ could then do a bad cast from CustomEvent to
MediaChangeEvent.

Code that wants to listen for random DOM events should use blink-in-js. In this
case I think we just need a separate interface for C++ media listeners.

[cbiesinger, 2014-08-07 01:12:07]

On 2014/08/06 23:59:56, esprehn wrote:
> We should use a separate interface for C++ listeners. DOM events are dangerous
> because the string type has no relation to the event type. Specifically if C++
> has a listener on a MediaList object, and the change event fired a
> MediaChangeEvent object, someone could do list.dispatchEvent(new
> CustomEvent("change")) and the C++ could then do a bad cast from CustomEvent
to
> MediaChangeEvent.
> 
> Code that wants to listen for random DOM events should use blink-in-js. In
this
> case I think we just need a separate interface for C++ media listeners.

That makes sense. This patch does implement a C++-specific interface.

[esprehn, 2014-08-13 22:48:36]

lgtm

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.cpp (right):

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:64: addEventListener("change", listener,
false);
EventTypeNames::change

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:69: removeEventListener("change",
listener.get(), false);
listener.release()

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryListEvent.h (right):

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
Source/core/css/MediaQueryListEvent.h:37: bool matches() const { return
m_matches; }
return m_mediaList->matches; and make them live.

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
Source/core/css/MediaQueryListEvent.h:45: {
m_matches(false)

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryListListener.h (right):

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
Source/core/css/MediaQueryListListener.h:39: virtual void call() = 0;
notify() ? You can rename in a new patch.

[cbiesinger, 2014-08-16 00:00:47]

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.cpp (right):

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:64: addEventListener("change", listener,
false);
On 2014/08/13 22:48:36, esprehn wrote:
> EventTypeNames::change

Done.

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:69: removeEventListener("change",
listener.get(), false);
On 2014/08/13 22:48:36, esprehn wrote:
> listener.release()
.
../../third_party/WebKit/Source/core/css/MediaQueryList.cpp:69:58: error: no
member named 'release' in 'WTF::PassRefPtr<blink::EventListener>'

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryListEvent.h (right):

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
Source/core/css/MediaQueryListEvent.h:37: bool matches() const { return
m_matches; }
On 2014/08/13 22:48:36, esprehn wrote:
> return m_mediaList->matches; and make them live.

Done.

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
Source/core/css/MediaQueryListEvent.h:45: {
On 2014/08/13 22:48:36, esprehn wrote:
> m_matches(false)

Done.

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryListListener.h (right):

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
Source/core/css/MediaQueryListListener.h:39: virtual void call() = 0;
On 2014/08/13 22:48:36, esprehn wrote:
> notify() ? You can rename in a new patch.

Will do

[cbiesinger, 2014-08-16 00:01:15]

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.cpp (right):

https://codereview.chromium.org/396283004/diff/280001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:69: removeEventListener("change",
listener.get(), false);
On 2014/08/16 00:00:47, cbiesinger wrote:
> On 2014/08/13 22:48:36, esprehn wrote:
> > listener.release()
> .
> ../../third_party/WebKit/Source/core/css/MediaQueryList.cpp:69:58: error: no
> member named 'release' in 'WTF::PassRefPtr<blink::EventListener>'

Er, fixed this differently (removed the .get())

[cbiesinger, 2014-08-16 00:01:22]

The CQ bit was checked by cbiesinger@chromium.org

[commit-bot: I haz the power, 2014-08-16 00:02:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cbiesinger@chromium.org/396283004/300001

[haraken, 2014-08-16 00:46:02]

LGTM

[commit-bot: I haz the power, 2014-08-16 01:08:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/20039)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/23022)

[commit-bot: I haz the power, 2014-08-16 01:38:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-16 01:38:13]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[Jens Widell, 2014-08-16 06:56:08]

https://codereview.chromium.org/396283004/diff/300001/Source/bindings/templat...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/396283004/diff/300001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:292: if (info.Length() >= 2 && listener &&
!impl->toNode())
Should we adjust the "2" here to "argument_index+1"?

[cbiesinger, 2014-08-29 22:54:13]

https://codereview.chromium.org/396283004/diff/300001/Source/bindings/templat...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/396283004/diff/300001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:292: if (info.Length() >= 2 && listener &&
!impl->toNode())
On 2014/08/16 06:56:08, Jens Widell wrote:
> Should we adjust the "2" here to "argument_index+1"?

Done.

[cbiesinger, 2014-08-29 23:18:08]

The CQ bit was checked by cbiesinger@chromium.org

[commit-bot: I haz the power, 2014-08-29 23:18:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cbiesinger@chromium.org/396283004/360001

[commit-bot: I haz the power, 2014-08-30 00:23:17]

Committed patchset #19 (id:360001) as 181124