Closed Bug 855240 Opened 7 years ago Closed 7 years ago

Regression: Video trailer playback on imdb.com is broken

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 23
Tracking Status
firefox20 --- unaffected
firefox21 --- unaffected
firefox22 + unaffected
firefox23 --- verified
fennec 22+ ---

People

(Reporter: AdrianT, Assigned: cwiiis)

References

()

Details

(Keywords: regression, reproducible, testcase, Whiteboard: [NavBar])

Attachments

(1 file)

Nightly 22.0a1 2013-03-26
LG Nexus 4 (Android 4.2.2)

Steps to reproduce:
1) Load imdb.com (mobile version) and open the trailer page for any movie (for e.g. "Oz the great and powerful")
2) Turn the device in Landscape orientation and tap on the play button

Expected results:
The video is correctly played

Actual results:
The video player shakes making the video un-vieweble

Notes:
This is not reproducible on Firefox Mobile 20 RC build 1 or Aurora 21.0a2 2013-03-26
Video capture: http://youtu.be/viNuppWkYGM
Component: General → Video/Audio
Product: Firefox for Android → Core
Version: Firefox 22 → Trunk
Dynamic toolbar regression actually. This is fixed with the dynamic toolbar disabled.
Component: Video/Audio → General
Product: Core → Firefox for Android
Summary: Video trailer playback on imdb.com is broken in Landscape orientation → Regression: Video trailer playback on imdb.com is broken in landscape orientation
Whiteboard: regression
Assignee: nobody → chrislord.net
tracking-fennec: ? → 22+
This appears to be some kind of infinite loop because of the viewport auto-sizing, looking into it now.
Status: NEW → ASSIGNED
(In reply to Chris Lord [:cwiiis] from comment #2)
> This appears to be some kind of infinite loop because of the viewport
> auto-sizing, looking into it now.

Is this the same issue as what I filed yesterday bug 859946? (DDG on the Nexus 4)
(In reply to Aaron Train [:aaronmt] from comment #3)
> (In reply to Chris Lord [:cwiiis] from comment #2)
> > This appears to be some kind of infinite loop because of the viewport
> > auto-sizing, looking into it now.
> 
> Is this the same issue as what I filed yesterday bug 859946? (DDG on the
> Nexus 4)

Not sure, possibly.

A further note, been trying to debug this - the bug occurs regardless of orientation, and when the dynamic toolbar is disabled too. It seems to oscillate between the window size and some larger size, for which I've not yet figured out the relation.
Retitling to be more accurate.
Summary: Regression: Video trailer playback on imdb.com is broken in landscape orientation → Regression: Video trailer playback on imdb.com is broken
In fact, let's just dupe this.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 853986
Sorry, this isn't the same as bug 853986 and is specific to imdb (and other sites with similar behaviour).
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
So changing the CSS viewport in direct response to MozScrollAreaChanged is not a good idea it turns out.

This patch delays the call, but I've also taken the opportunity to throttle it and fix a couple of bugs that would've caused more measurements to happen than is necessary.
Attachment #736319 - Flags: review?(bugmail.mozilla)
Comment on attachment 736319 [details] [diff] [review]
Delay and throttle viewport size changes based on page size

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

Ok, I think this makes sense. r=me with comments addressed.

::: mobile/android/chrome/content/browser.js
@@ +2973,5 @@
>               != this.viewportExcludesHorizontalMargins)) {
> +        if (!this.viewportMeasureCallback) {
> +          let self = this;
> +          this.viewportMeasureCallback = setTimeout(function() {
> +            self.viewportMeasureCallback = null;

I think doing a .bind(this) on the function is cleaner than using "self", but I'm not picky about it either way - your call.

@@ +2977,5 @@
> +            self.viewportMeasureCallback = null;
> +
> +            let viewport = self.getViewport();
> +            if (Math.abs((viewport.pageRight - viewport.pageLeft)
> +                           - self.lastPageSizeAfterViewportChange.width) >= 0.5 ||

There shouldn't be any significant rounding error here, right? lastPageSizeAfterViewportChange is calculated using viewport.pageRight - viewport.pageLeft as well. I would prefer replacing the 0.5 with 1e-6 which is what we use everywhere else for fuzzy matching. (We could also file a mentored bug for creating a fuzzyEquals method and refactoring the code to use it)
Attachment #736319 - Flags: review?(bugmail.mozilla) → review+
First change made, second change not made after discussion on IRC - I originally had it at 1e-6, but I was seeing rounding errors above that so thought it'd be sensible to just move it to a value that would eliminate rounding errors (the measurements at this point are screen pixels).

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/5a92cfb2d1c3

Opened bug for adding a fuzzyEquals function to browser.js: https://bugzilla.mozilla.org/show_bug.cgi?id=861205
https://hg.mozilla.org/mozilla-central/rev/5a92cfb2d1c3
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Whiteboard: [NavBar]
Verified fixed on:
Build: Firefox for Android 23.0a1(2013-04-22)
Device:LG Nexus 4
OS: Android 4.2.2
Status: RESOLVED → VERIFIED
Can we uplift this to Aurora? Bug 867084 is kind of bad (considering it's one of our own sites), and I bisected it to being fixed by this bug.
(In reply to Jim Chen [:jchen :nchen] from comment #13)
> Can we uplift this to Aurora? Bug 867084 is kind of bad (considering it's
> one of our own sites), and I bisected it to being fixed by this bug.

^
Flags: needinfo?(chrislord.net)
Now that the dynamic toolbar is disabled in Firefox 22, does this still need uplifting?
Flags: needinfo?(chrislord.net) → needinfo?(aaron.train)
Nope. We're good.
Flags: needinfo?(aaron.train)
Marking unaffected on 22 as per comment #16.
You need to log in before you can comment on or make changes to this bug.