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

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 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

(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).

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

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

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.

(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

(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.

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

(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?

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.

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

(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.

> but people are working on running them on tbpl as well. Excellent. If I can help somehow, please let me know!

(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?

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