480761 - resizing window while panned to the left doesn't redraw previously offscreen area

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

We don't call the viewportUpdateHandler and therefore don't redraw if there's no need to pan the content in response to the window resize (i.e. if we're panned to the left such that there is already content past the right window edge).

We can fix that easily enough by not relying on adjustViewingRect's call to panTo and just always calling the viewportUpdateHandler.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Oh, and this only manifests itself when the window starts off smaller than the viewport and is then made larger. If the window size goes over the viewport width, bug 480762 kicks in.

Comment 4

[(dormant account)]

Comment on attachment 364722 [details] [diff] [review]
patch

I think this is ok. Patch made me realize that viewportHandler ended up getting called 2x per zoom operation(i think resizes too). That is a probably a perf bug.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)
> Patch made me realize that viewportHandler ended up getting
> called 2x per zoom operation(i think resizes too).

Didn't my patch in bug 477105 fix that?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mobile-browser/rev/02384ea1b0d9

Comment 7

[(dormant account)]

Attachment: batch viewport updates on startup

Mark, I think it's time for that startup flag :)

Comment 8

[(dormant account)]

Gavin points out I forgot to set _isStartup to false in that if statement. I should clarify that I meant for this patch to be used as a starting point for me and mark to shove various init stuff into.

Comment 9

[(dormant account)]

Attachment: try #2

It doesn't make sense for the drawing code to do anything until the final resize fires, unfortunately it fires AFTER the firstrun page finishes loading.

This patch deals with that. It also ignores bogus initial resizes.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 365070 [details] [diff] [review]
try #2

I don't like using _isStartup in a countdown manner. 

I do think we need some kind of event to signal the app is loaded and ready.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 365070 [details] [diff] [review]
try #2


>@@ -116,18 +120,21 @@ var Browser = {
>       if (e.target != window)
>         return;
> 
>+      // resize our container...
>+      let w = window.innerWidth;
>+      let h = window.innerHeight;
>+      if (w > window.screen.width && h > window.screen.height)
>+        return

After doing some testing, I've found only window.screen.availWidth will match exactly to the final width. The height will end up less than screen.height and screen.availHeight, but never equal to either.

Also, we only want to do this check of sizemode="maximized"

I have a similar patch in bug 477653

Comment 12

[(dormant account)]

Attachment: batch viewport updates on startup

I'd like to land this or something close to this.

Comment 13

[(dormant account)]

Attachment: batch viewport updates on startup

no event-counting funnyness now.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 365282 [details] [diff] [review]
batch viewport updates on startup

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>-      if (maximize && window.innerWidth > screen.availWidth)
>+      if (maximize && window.innerWidth > screen.width) {
>         return;
>-
>+      }

Don't add the brackets?

I'm slightly worried that we'll end up not calling endUpdateBatch for whatever reason, not sure if we could add some extra protection there... perhaps have user-triggered pans force an endUpdateBatch?

Comment 15

[(dormant account)]

http://hg.mozilla.org/mobile-browser/rev/2193001441dd


(In reply to comment #14)
> 
> I'm slightly worried that we'll end up not calling endUpdateBatch for whatever
> reason, not sure if we could add some extra protection there... perhaps have
> user-triggered pans force an endUpdateBatch?

I think that's similar to some other startup code not running. I wouldn't worry about it. One thing we could do is make sure that during user-triggered-pans were aren't in a transaction and popup a big fat error(ie these aren't meant to be long-lasting)

Comment 16

[(dormant account)]

I just realized that calling endLoading from resizehandler doesn't make sense as for pages that take longer to render this could screw things up(mildly) by calling endLoading on the canvas before the page finishes loading.

Comment 17

[(dormant account)]

Attachment: fix

Comment 18

[(dormant account)]

pushed http://hg.mozilla.org/mobile-browser/rev/3e25d365b93f