630736 - Changing orientation causes checkerboarding - liliputing.com

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Kevin Brosnan]

Load liliputing.com in a landscape view
Zoom in on the page
Change orientation to vertical

Comment 1

[Kevin Brosnan]

Attachment: screenshot

Comment 2

[Kevin Brosnan]

Mozilla/5.0 (Android; Linux armv7l; rv:2.0b11pre) Gecko/20110126 Firefox/4.0b11pre Fennec/4.0b4 ID:20110126160730

HTC G2 Android 2.2

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I wonder if this one is a duplicate of bug 630946? We need to re-check once bug 630946 will land.

Comment 4

[Stuart Parmenter]

Does this still happen now that the other patches have landed?

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Kevin, 
do you mind retrying since I am unsure of your steps to reproduce?

The way I was reproducing it on my side was:
 * load liliputing.com in a landscape view
 * zoom in
 * scroll to bottom
 * change orientation

Comment 6

[Kevin Brosnan]

This is what I am doing.

http://dl.dropbox.com/u/686313/630736.webm

Comment 7

[Anna (Waverley)]

I can also reproduce this.

Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b12pre) Gecko/20110214 Firefox/4.0b12pre Fennec /4.0b5pre 

Devices: Motorola Droid 2(Android 2.2), HTC Desire (Android 2.2)

1. Load "www.mozilla.org" in portrait mode.
2. Perform a double tap on the screen to zoom in.
3. Rotate the device to landscape mode.

Actual results:
After step 3, the web page does not re-size to fit the orientation of the screen. Please see the attached screenshot.

Expected results:
After step 3, the browser and page should re-size to fit the orientation of the screen.

Comment 8

[Anna (Waverley)]

Attachment: checkerboard appears

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I can reproduce it too, thanks for checking and the STR.

Comment 10

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

Attachment: patch

When changing the window size we really need to reset the display port too.

Matt though we should call scrollBy(0, 0) which ends up calling _updateCacheViewport and manages the _pixelsPannedSinceRefresh, but I found it kinda confusing to do it and I'm not sure we need to manage _pixelsPannedSinceRefresh when resizing the window.

The current patch does as little as we can do in the parent process. We might be able to make a smaller patch by working entirely in the child process. If regress Ts / Tp, we might need to do that.

browser.setWindowSize is called from Tab.updateViewportSize, which is called from a few places, none of which seem performance hot spots except for maybe scrollAreaChanged handler here:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#2587
I'd like to kill that method anyway.

Comment 11

[Matt Brubeck (:mbrubeck)]

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

This looks good to me.  I'm concerned that this increases the number of displayport updates during an orientation change.  But we should worry about correctness first, and performance later.  I'll file a followup bug or patch to reduce the duplicate work.

Comment 12

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

pushed:
http://hg.mozilla.org/mobile-browser/rev/2985b4863961

Comment 13

[Matt Brubeck (:mbrubeck)]

Attachment: fix restoreViewportPosition regressions

One reason this regressed is that we incorrectly stopped calling restoreViewportPosition in most cases where we should.  This fixes the logic so that we call it whenever the layout is unchanged:

1) We were mistakenly comparing the contentWindowWidth to the chrome window width.  Instead, we should compare contentWindowWidth from before and after the viewport update.

2) Calling scrollBy(0,0) in the middle of restoreViewportPosition seems to have a race condition; it makes the getPosition call frequently return unexpected results.  The scrollBy call was added in bug 512369, but removing it does not seem to regress that bug or cause other ill effects.  Removing it also saves us a wasted updateCacheViewport call.

3) Move the call to restoreViewportPosition before the call to updateDefaultZoomLevel.  This will prevent another wasted updateViewportCache call (since updateDefaultZoomLevel is smart enough not to rescale the browser if restoreViewportPosition has already done it).

The other patch in attachment 512369 [details] [diff] [review] is still needed, because there are still some cases where neither restoreViewportPosition nor updateDefaultZoomLevel triggers a cache update.  This just fixes some behavior regressions that happen to be related to this bug.

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 512393 [details] [diff] [review]
fix restoreViewportPosition regressions

oops, wrong bug and wrong patch.  embarrassing!

Comment 15

[Matt Brubeck (:mbrubeck)]

Attachment: fix restoreViewportPosition regressions

Actually, this was the right bug... and here's the right patch.

Comment 16

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mobile-browser/rev/dc25fe7ee058

Comment 17

[Anna (Waverley)]

VERIFIED FIXED on

Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b12pre) Gecko/20110215 Firefox/4.0b12pre Fennec /4.0b5pre 

Devices: Motorola Droid 2 (Android 2.2), HTC Desire (Android 2.2)