Closed Bug 1086996 Opened 6 years ago Closed 6 years ago
Regression: touch video controls are busted
I can't bring up the video controls at all while a video is playing. Recent regression I think.
Looks like it first started in the 10-21-2014 Nightly.
Last good revision: 33c0181c4a25 First bad revision: fff578fc740a Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=33c0181c4a25&tochange=fff578fc740a
Ok that regression range seems....unlikely
6 years ago
Ok, this range looks more reasonable: Last good revision: acce1a807efb First bad revision: 2681f9b134c2 Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=acce1a807efb&tochange=2681f9b134c2
There are two places in videocontrols.xml where we dispatch an event to chrome, like this: this.dispatchEvent(new CustomEvent("VideoBindingAttached")); http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#1711 http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#1779 I assume the <constructor> line is failing, breaking the binding. When I added this code, I modeled it after the "PluginBindingAttached" event here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/plugins/content/pluginProblem.xml#78
Assignee: nobody → bobbyholley
Summary: Video controls busted → Touch video controls busted
Comment on attachment 8509401 [details] [diff] [review] Stop wrapping XBL handlers into the content compartment. v1 The basic problem with this patch is that nsXBLPrototypeHandler.cpp creates the bound handlers in the XBL scope, but then wraps them into the content scope before invoking them (there doesn't seem to be any reason for this anymore, AFAICT, so my patch just removes it). When the handler is executed from the XBL scope, we run into problems trying to pass |this|, because |this| is anonymous content that lives in the XBL scope, and thus isn't something that content should be allowed to touch.
Attachment #8509401 - Flags: review?(bugs)
(In reply to Bobby Holley (:bholley) from comment #7) > Created attachment 8509401 [details] [diff] [review] > Stop wrapping XBL handlers into the content compartment. v1 This fixes the issue, thanks! Tested on b2g desktop.
Attachment #8509401 - Flags: feedback?(martijn.martijn) → feedback+
I think you could use something like SpecialPowers.registerConsoleListener to write a mochitest for videocontrols: http://mxr.mozilla.org/mozilla-central/source/layout/mathml/tests/test_bug553917.html?force=1#116 Mochitests are run on Android and b2g emulator, so that way it would have been caught earlier.
(In reply to Martijn Wargers [:mwargers] (QA) from comment #10) > I think you could use something like SpecialPowers.registerConsoleListener > to write a mochitest for videocontrols: That would only be to catch this specific issue, yes? There are various ways to do that, but the bigger problem is that we don't appear to have any coverage on CI that checks whether the touch controls work at all.
Comment on attachment 8509401 [details] [diff] [review] Stop wrapping XBL handlers into the content compartment. v1 smaug wants bz to review this instead.
Attachment #8509401 - Flags: review?(bugs) → review?(bzbarsky)
(In reply to Bobby Holley (:bholley) from comment #12) > smaug wants bz to review this instead. Because the code to be removed was added in Bug 821850.
Comment on attachment 8509401 [details] [diff] [review] Stop wrapping XBL handlers into the content compartment. v1 r=me
Attachment #8509401 - Flags: review?(bzbarsky) → review+
Pushed with a test for the specific XBL issue (though nothing for the broader problem of not having touchControls coverage): remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/00f96ab44761 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1448d42a32c6
We do some testing of video controls in a gaia-ui-test, that's why duped bug 1086265 was filed.
(In reply to Martijn Wargers [:mwargers] (QA) from comment #16) > We do some testing of video controls in a gaia-ui-test, that's why duped bug > 1086265 was filed. Any way we can get those tests showing up in treeherder?
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Hardware: x86 → ARM
Summary: Touch video controls busted → Regression: touch video controls are busted
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.