950523 - Move nsDOMMediaQueryList to WebIDL bindings

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[(no longer active)]

Attachment: Part 1: Rename nsDOMMediaQueryList to mozilla::dom::MediaQueryList; r=bzbarsky

Comment 2

[(no longer active)]

Attachment: Part 2: Move MediaQueryList to WebIDL bindings; r=bzbarsky

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8347845 [details] [diff] [review]
Part 1: Rename nsDOMMediaQueryList to mozilla::dom::MediaQueryList; r=bzbarsky

r=me

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8347846 [details] [diff] [review]
Part 2: Move MediaQueryList to WebIDL bindings; r=bzbarsky

>+  [Throws,NewObject] MediaQueryList? matchMedia(DOMString query);

Can you raise a spec issue about adding [NewObject] in the spec there, please?

>+++ b/layout/style/MediaQueryList.cpp
>+    if (aListener.Callable() == mCallbacks[i]->Callable()) {

That actually doesn't do the right thing when cross-compartment wrappers are involved.

Ther right test is what CallbackObjectHolder::operator== does.  And the simplest way to use it is to use a CallbackObjectHolder to hold your stuff.  Though maybe we should add an operator== on WebIDL callbacks that would do the right thing, since you don't actually want the unified CallbackObjectHolder behavior here...

Do we need to keep the XPCOM AddListener?  Are there any actual consumers of it?  If not, we should simplify the code by removing it.

Similar comments apply to RemoveListener.

> MediaQueryList::MediumFeaturesChanged(NotifyList &aListenersToNotify)
>+  if (mListeners.Length() || mCallbacks.Length()) {

HasListeners()?

>+++ b/layout/style/MediaQueryList.h
>+  typedef FallibleTArray< nsCOMPtr<mozilla::dom::MediaQueryListListener> > CallbackList;

Should really use nsRefPtr there, not nsCOMPtr.

r=me with the above fixed.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8347846 [details] [diff] [review]
Part 2: Move MediaQueryList to WebIDL bindings; r=bzbarsky

Review of attachment 8347846 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/test_media_query_list.html
@@ +268,5 @@
>    (function() {
>      iframe.style.width = "200px";
>      subroot.offsetWidth; // flush layout
>  
> +    function expectException(f) {

Look an awful lot like SimpleTest.doesThrow

Comment 6

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Comment on attachment 8347846 [details] [diff] [review]
> Part 2: Move MediaQueryList to WebIDL bindings; r=bzbarsky
> 
> >+  [Throws,NewObject] MediaQueryList? matchMedia(DOMString query);
> 
> Can you raise a spec issue about adding [NewObject] in the spec there,
> please?

https://www.w3.org/Bugs/Public/show_bug.cgi?id=24112

> Though maybe we should add an operator== on WebIDL callbacks that would do
> the right thing, since you don't actually want the unified
> CallbackObjectHolder behavior here...

Bug 950657.

> Do we need to keep the XPCOM AddListener?  Are there any actual consumers of
> it?  If not, we should simplify the code by removing it.
> 
> Similar comments apply to RemoveListener.

Hmm, yeah I actually think we can remove them.  I'll do that as a follow-up (bug 950659).

Comment 7

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb12cbb6c96
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f48392a6250

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3fb12cbb6c96
https://hg.mozilla.org/mozilla-central/rev/3f48392a6250

Comment 9

[Gregor Wagner [:gwagner]]

This caused a permanent travis failure for gaia tests:
  b2g-desktop stderr JavaScript error: app://gallery.gaiamobile.org/shared/js/screen_layout.js, line 95: Argument 1 of MediaQueryList.addListener is not callable.

Comment 10

[Carsten Book [:Tomcat]]

(In reply to Gregor Wagner [:gwagner] from comment #9)
> This caused a permanent travis failure for gaia tests:
>   b2g-desktop stderr JavaScript error:
> app://gallery.gaiamobile.org/shared/js/screen_layout.js, line 95: Argument 1
> of MediaQueryList.addListener is not callable.

Note that i tried to backout this change from m-c for this regression but run into conflicts, also seems Ms2ger and other looking into this to fix the regression

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Carsten Book [:Tomcat] from comment #10)
> Note that i tried to backout this change from m-c for this regression but
> run into conflicts, also seems Ms2ger and other looking into this to fix the
> regression

bz landed a patch shortly after these landed which depended on them.

Comment 12

[Gregor Wagner [:gwagner]]

We pushed a fix to gaia. No need for backout any more.

Comment 13

[(no longer active)]

(In reply to comment #12)
> We pushed a fix to gaia. No need for backout any more.

What was the fix?  Is is related to bug 951088?

Comment 14

[Boris Zbarsky [:bzbarsky]]

The fix is to stop using a random object with a handleChange property, which xpidl allowed here but the spec doesn't, and instead to use an actual function as the listener.  Like so:

  this.boundHandleChange = this.handleChange.bind(this);

and then:

-    this.queries[name].addListener(this);
+    this.queries[name].addListener(this.boundHandleChange);

-      this.queries[name].removeListener(this);
+      this.queries[name].removeListener(this.boundHandleChange);

Gregor, thank you for doing that!

> Is is related to bug 951088?

I would assume bug 951088 is fixed by the Gaia fix here.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Gregor, are the tests in comment 9 somewhere on tbpl?  Or at least heading that way?

Comment 16

[Gregor Wagner [:gwagner]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> Gregor, are the tests in comment 9 somewhere on tbpl?  Or at least heading
> that way?

Currently we only run those tests on travis but people are working on running them on tbpl as well.
I know that the current situation is unacceptable and with every gaia-tree closure caused by a gecko regression I push harder for it.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> but people are working on running them on tbpl as well.

Excellent.  If I can help somehow, please let me know!

Comment 18

[Natalya Kot [:nkot]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #13)
> (In reply to comment #12)
> > We pushed a fix to gaia. No need for backout any more.
> 
> What was the fix?  Is is related to bug 951088?

Is it related to bug 951280?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Natalya: hard to tell, since bug 951280 doesn't include any information about errors or anything...