This binding runs with no privileges and so we can't import AppConstants making removing the preprocessing more tricky. Dolske has a plan though!.

This removes one #ifdef entirely by just always using fastSeek(), and replaces the other with a navigator.platform check. Works in local testing. I did, however, notice that fastSeek() seems a little odd... In my testing on https://people.mozilla.org/~dolske/tmp/rickroll.ogg (sorry, it's my go-to test video!), the fastSeek() flavor will immediately play audio at the new position after a seek, but the video doesn't play at the new position for 1-2 seconds (ie, the last frame before the seek is noticeably frozen for a period of time). Also, in cpearce's test video (http://people.mozilla.org/~cpearce/fastSeek/) from bug 778077 comment 33, seeking will sometimes actually jump to a different spot a few seconds away. I think that might be expected due to how fastSeek() works, but it's kind of crappy UX. :( So unless these last two things are extremely unlikely to hit in real-world videos (or there's some other bug that can be fixed), I think I should go back to only using fastSeek() on FFOS. I'll probably just use a navigator.platform check there too. (Anyone know what it is on FFOS?)

I highly recommend that we don't use fastSeek on desktop. The UX is pretty terrible. Please, please, please(!) do not enable fastSeek on desktop! It's only really enabled on FxOS because of the low end devices we were targeting there. FastSeek is supposed to seek the video stream to the keyframe nearest the seek target, rather than seeking to keyframe nearest to the seek target and decoding the video stream forwards until the seek target and then terminating the seek. So it makes seeking faster on low end handsets where the "decode to seek target" step is very slow. On desktop and higher end android phones, this step is not noticeable. So please, don't enable this. We could even remove the call to faskSeek, since we're not targeting low end FxOS phones any more. The FxOS guys would need to comment on that however.

Comment on attachment 8699202 [details] [diff] [review] Patch v.1 Review of attachment 8699202 [details] [diff] [review]: ----------------------------------------------------------------- fastSeek is bad UX. We should not enable it on desktop for seeking, it'll make the default video controls suck.

Comment on attachment 8699202 [details] [diff] [review] Patch v.1 Review of attachment 8699202 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/videocontrols.xml @@ +1251,5 @@ > if (event.shiftKey) > keystroke += "shift-"; > + > + // We don't want to #ifdef XP_MACOSX and can't use AppConstants... > + if (window.navigator.platform == "MacIntel") { This will break on TenFourFox since window.navigator.platform can be "MacPPC" there. You could check if the platform string starts with "Mac". Alternatively, this code could be refactored to call a new getModifierKeys() function. The #videoControls binding's implementation of getModifierKeys() would have `if (event.metaKey) keystroke += "meta-"; if (event.ctrlKey) keystroke += "accel-";`. Then, the binding could be extended to create a #videoControlsMac binding which overrides getModifierKeys() and translates metaKey to "acccel-" and ctrlKey to "control-". This would be hooked up in html.css, which still uses the preprocessor.

(In reply to Chris Pearce (:cpearce) from comment #2) > I highly recommend that we don't use fastSeek on desktop. The UX is pretty > terrible. Please, please, please(!) do not enable fastSeek on desktop! Concur. :) > We could even remove the call to faskSeek, > since we're not targeting low end FxOS phones any more. The FxOS guys would > need to comment on that however. I think we should do this. Otherwise I'm not quite sure how to avoid the #ifdef, because I'm not quite sure how to reliably detect Firefox OS from unprivileged code. (Other than the ugly hack Jared mentions in comment 4, which I'd prefer to avoid.) This must be pretty awful to use on FFOS, especially if you _do_ have a device that's otherwise powerful enough to deal with media. Do you know who can r+ that for FFOS?

Blake Wu manages the Taipei/FirefoxOS Media team, he can either make this call or discuss with the stake holders in Taipei to get a decision. JW in Taipei may also have an opinion on this. Blake/JW: We're proposing to make the default video controls not use fastSeek on FirefoxOS but instead use the same code paths as on Desktop Firefox. That is, instead of the default video controls using fast-but-inaccurate seek on FirefoxOS, use a more resource intensive but accurate seek, just like in Firefox Desktop. I think this is a good idea because we've had lots of bugs filed about fastSeek giving unexpected behaviour in the FirefoxOS video controls, and since we've stopped targeting low end FirefoxOS devices the performance benefit of using fastSeek seems to be less critical.

I wonder if most of the users are aware of the idea of fastSeek. If not, the UI response would be cranky to them. I am also in favor of accurate seek by default. We can leave the discussion until there is a high demand for fastSeek from the users.

IMO, we should still use fastSeek, at least in video APP for better user experience, so we may still need to handle those unexpected behavior... Two major reasons are as below: 1. It is not only useful on low end device. Take Aries for example, it can record and play a 4k file, accurate seeking in a 4k file takes noticeable time. 2. Using accurate seek when users drag the progress bar may cause video frames rendering slow due to a series of seeking to key frames and dropping unwanted frames. Ni djf, video App owner, to have his opinion as well.

Review commit: https://reviewboard.mozilla.org/r/32323/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/32323/

Comment on attachment 8711817 [details] Bug 1233198 - Remove preprocessing from toolkit/content/widgets/videocontrols.xml. https://reviewboard.mozilla.org/r/32323/#review29617 ::: toolkit/content/widgets/videocontrols.xml:800 (Diff revision 1) > + if (!window.navigator.platform) { I started doing this too, after looking for something in the B2G UA to filter on, and finding bug 801614. I'm not super-thrilled with it, though. I'm a little worried about that people who have tweaked their UA (in the name of privacy) might trigger this, and they'd experience terrible seeking (fastSeek) as a result. It's so non-obviously related that it would be hard to figure out why Firefox seems broken. (This could also happen with the Mac code below, but a keyboard modifier seems much less likely to be noticed.)

If B2G thinks fastSeek() functionality is critical, it seems like this should really be moved into a back-end preference to control how setting .currentTime works. (Although that risks being a bit of a footgun, because inevitably some tweak-guide will suggest users enable this without understanding the impact, so I'd want this to never ever work on Desktop.) Do any other browsers actually even support fastSeek()? We added it in Firefox 31, and the WhatWG/Devmo compat tables don't show anyone else implementing it. As such, I'd expect that there's zero web content using this, which means B2G users are already having to deal with this issue. Finally: it's not the most polite option, but given that B2G is Tier 3, this is impeding Tier 1 improvements, and opinions vary on if it's helping or hurting B2G... Maybe the best solution is to just rip it out.

Well, the Firefox OS UA string says it's mobile but doesn't include Android or iPhone, so we could also use that. if (window.navigator.platform.includes("Mobile") && !window.navigator.platform.includes("Android") && !window.navigator.platform.includes("iPhone")) { // fastSeek(); } ...

I fear the same issue, though. UA hacks are inherently gross and fragile, and having that kind of brokenness-at-a-distance is something I'd like to avoid. To restate a bit of comment 11, does anyone want to argue this is an important feature for desktop/android? We could completely disable fastSeek() on not-B2G, and do feature detection in the video controls to decide to use it. [Although I'd come back to wondering how useful a web API that's _only_ used on B2G really is.]

Comment on attachment 8711817 [details] Bug 1233198 - Remove preprocessing from toolkit/content/widgets/videocontrols.xml. https://reviewboard.mozilla.org/r/32323/#review68966 ::: toolkit/content/widgets/videocontrols.xml:1152 (Diff revision 3) > var keystroke = ""; > if (event.altKey) > keystroke += "alt-"; > if (event.shiftKey) > keystroke += "shift-"; > -#ifdef XP_MACOSX > + if (window.navigator.platform.startsWith("Mac")) { Can drop the 'window' reference here, I think? ::: toolkit/content/widgets/videocontrols.xml:1352 (Diff revision 3) > - > + this.changeTextTrack(this.isClosedCaptionOn() ? 0 : lastTTIdx); > - return this.changeTextTrack(this.isClosedCaptionOn() ? 0 : lastTTIdx); Shouldn't this still early return to preserve behaviour? ::: toolkit/content/widgets/videocontrols.xml:1903 (Diff revision 3) > + newPosition /= 1000; // convert from ms > + this.Utils.log("+++ seeking to " + newPosition); > + // We use fastSeek() on B2G, and an accurate (but slower) > + // seek on other platforms (that are likely to be higher > + // perf). > + this.video.fastSeek(newPosition); So I get the idea, but I don't understand how this is working. There are no references to GonkUtils anywhere, and we don't overwrite the original Utils object, so I don't see how this version of `seekToPosition` ever gets called. Separately, it seems like it would be simpler to have the sub-binding just define, say, a field or property ("isGonk" set to true or whatever) and to have an if (this.isGonk) { fastSeek(newPosition) } else { other thing } in the original seekToPosition. Saves us another binding etc. Would that work? Also, how are we testing this, or are we flying blind and hoping we don't break b2g?

Comment on attachment 8711817 [details] Bug 1233198 - Remove preprocessing from toolkit/content/widgets/videocontrols.xml. https://reviewboard.mozilla.org/r/32323/#review68966 > Can drop the 'window' reference here, I think? I had used it here because we used window.getComputedStyle elsewhere in the file. I've now removed the window reference from the getComputedStyle call. > Shouldn't this still early return to preserve behaviour? Yikes, thanks for catching that.

Comment on attachment 8711817 [details] Bug 1233198 - Remove preprocessing from toolkit/content/widgets/videocontrols.xml. https://reviewboard.mozilla.org/r/32323/#review68970 ::: toolkit/content/widgets/videocontrols.xml:776 (Diff revision 4) > }, > > seekToPosition : function(newPosition) { > newPosition /= 1000; // convert from ms > this.log("+++ seeking to " + newPosition); > -#ifdef MOZ_WIDGET_GONK > + if (this.isGonk) { We define isGonk on the binding, so on the videocontrols element, and |this| here is presumably the Utils object, so I *think* this should be this.videocontrols.isGonk. This goes back to "how are we testing that this works correctly? :-)

Comment on attachment 8711817 [details] Bug 1233198 - Remove preprocessing from toolkit/content/widgets/videocontrols.xml. https://reviewboard.mozilla.org/r/32323/#review68972

I tested that this last version works. In layout/style/res/html.css, I changed the desktop videocontrols binding to use #touchControlsGonk and placed an alert in the two different code paths. When the #touchControlsGonk binding was used the fastSeek code path was followed.

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b20c524ca0c4 Remove preprocessing from toolkit/content/widgets/videocontrols.xml. r=Gijs