Closed Bug 630736 Opened 10 years ago Closed 10 years ago
Changing orientation causes checkerboarding - liliputing
74.85 KB, image/png
96.02 KB, image/png
855 bytes, patch
|Details | Diff | Splinter Review|
1.94 KB, patch
|Details | Diff | Splinter Review|
Load liliputing.com in a landscape view Zoom in on the page Change orientation to vertical
Mozilla/5.0 (Android; Linux armv7l; rv:2.0b11pre) Gecko/20110126 Firefox/4.0b11pre Fennec/4.0b4 ID:20110126160730 HTC G2 Android 2.2
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.
I can reproduce it too, thanks for checking and the STR.
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+
Status: NEW → RESOLVED
Closed: 10 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.
Comment on attachment 512393 [details] [diff] [review] fix restoreViewportPosition regressions oops, wrong bug and wrong patch. embarrassing!
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.