855240 - Regression: Video trailer playback on imdb.com is broken

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

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

Comment 1

[Aaron Train [:aaronmt]]

Dynamic toolbar regression actually. This is fixed with the dynamic toolbar disabled.

Comment 2

[Chris Lord [:cwiiis]]

This appears to be some kind of infinite loop because of the viewport auto-sizing, looking into it now.

Comment 3

[Aaron Train [:aaronmt]]

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

Comment 4

[Chris Lord [:cwiiis]]

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

Comment 5

[Chris Lord [:cwiiis]]

Retitling to be more accurate.

Comment 6

[Chris Lord [:cwiiis]]

In fact, let's just dupe this.

Comment 7

[Chris Lord [:cwiiis]]

Sorry, this isn't the same as bug 853986 and is specific to imdb (and other sites with similar behaviour).

Comment 8

[Chris Lord [:cwiiis]]

Attachment: Delay and throttle viewport size changes based on page size

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.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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)

Comment 10

[Chris Lord [:cwiiis]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5a92cfb2d1c3

Comment 12

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 23.0a1(2013-04-22)
Device:LG Nexus 4
OS: Android 4.2.2

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 14

[Aaron Train [:aaronmt]]

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

^

Comment 15

[Chris Lord [:cwiiis]]

Now that the dynamic toolbar is disabled in Firefox 22, does this still need uplifting?

Comment 16

[Aaron Train [:aaronmt]]

Nope. We're good.

Comment 17

[Chris Lord [:cwiiis]]

Marking unaffected on 22 as per comment #16.