Closed Bug 630736 Opened 9 years ago Closed 9 years ago

Changing orientation causes checkerboarding - liliputing.com

Categories

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

ARM
Android
defect

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: kbrosnan, Assigned: mfinkle)

References

()

Details

Attachments

(4 files, 1 obsolete file)

Load liliputing.com in a landscape view
Zoom in on the page
Change orientation to vertical
Attached image screenshot
Mozilla/5.0 (Android; Linux armv7l; rv:2.0b11pre) Gecko/20110126 Firefox/4.0b11pre Fennec/4.0b4 ID:20110126160730

HTC G2 Android 2.2
tracking-fennec: --- → ?
Priority: -- → P2
I wonder if this one is a duplicate of bug 630946? We need to re-check once bug 630946 will land.
tracking-fennec: ? → 2.0+
Assignee: nobody → 21
Does this still happen now that the other patches have landed?
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
This is what I am doing.

http://dl.dropbox.com/u/686313/630736.webm
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.
Attached image checkerboard appears
I can reproduce it too, thanks for checking and the STR.
Attached patch patchSplinter Review
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.
Assignee: 21 → mark.finkle
Attachment #512369 - Flags: review?(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.
Attachment #512369 - Flags: review?(mbrubeck) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/2985b4863961
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.
Attachment #512393 - Flags: review?(mark.finkle)
Comment on attachment 512393 [details] [diff] [review]
fix restoreViewportPosition regressions

oops, wrong bug and wrong patch.  embarrassing!
Attachment #512393 - Attachment is obsolete: true
Attachment #512393 - Flags: review?(mark.finkle)
Actually, this was the right bug... and here's the right patch.
Attachment #512394 - Flags: review?(mark.finkle)
Attachment #512394 - Flags: review?(mark.finkle) → review+
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.