90222 – [chromium, android] Reloading a page with a different user agent can cause the page to be zoomed in

RESOLVED FIXED 90222

[chromium, android] Reloading a page with a different user agent can cause the page to be zoomed in

https://bugs.webkit.org/show_bug.cgi?id=90222

Summary [chromium, android] Reloading a page with a different user agent can cause th...

dfalcantara

Reported 2012-06-28 16:12:55 PDT

Created attachment 150038 [details] Screenshots showing slashdot.org loads under different situations When reloading a page after changing from a mobile to a desktop user agent (e.g.), the page scale from the first time the page was loaded is re-used. This often results in desktop pages that are zoomed in instead of showing the whole page after reloading. This happens frequently on sites with different layouts for mobile and desktop browsers, like http://slashdot.org. Ideally, the page would appear as if it were loaded by navigating directly to the page with the right user agent to avoid situations like these.

Attachments
Screenshots showing slashdot.org loads under different situations (810.54 KB, application/octet-stream) 2012-06-28 16:12 PDT, dfalcantara	no flags	Details
Patch (6.92 KB, patch) 2012-07-03 17:00 PDT, dfalcantara	fishd: review-	Details Formatted Diff Diff
Patch (3.20 KB, patch) 2012-08-28 17:02 PDT, dfalcantara	no flags	Details Formatted Diff Diff
Patch (3.33 KB, patch) 2012-08-30 14:14 PDT, dfalcantara	no flags	Details Formatted Diff Diff
Patch (5.93 KB, patch) 2012-08-30 16:02 PDT, dfalcantara	no flags	Details Formatted Diff Diff
Patch (6.34 KB, patch) 2012-08-31 12:17 PDT, dfalcantara	no flags	Details Formatted Diff Diff
Patch (6.42 KB, patch) 2012-09-10 15:15 PDT, dfalcantara	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

dfalcantara

Comment 1 2012-07-03 17:00:46 PDT

Created attachment 150693 [details] Patch

dfalcantara

Comment 2 2012-07-03 17:01:46 PDT

I'm not sure if this is the right way to do it, but I can't figure out an alternative way to accomplish the same thing. Ideas?

WebKit Review Bot

Comment 3 2012-07-03 17:03:49 PDT

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Adam Barth

Comment 4 2012-07-04 08:15:49 PDT

Comment on attachment 150693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150693&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:3227 > + if (m_resetPageStateOnLoad && m_page && m_page->mainFrame()) { > + m_page->mainFrame()->loader()->history()->saveDocumentAndScrollState(); > + RefPtr<HistoryItem> currentItem = m_page->mainFrame()->loader()->history()->currentItem(); > + currentItem->clearScrollPoint(); > + currentItem->setPageScaleFactor(0); > + m_pageScaleFactorIsSet = false; > + m_resetPageStateOnLoad = false; > + } It seems a bit odd for us to be manipulating this state from the WebKit layer. I guess we might need to do that in order to keep m_pageScaleFactorIsSet updated properly...

Adam Barth

Comment 5 2012-07-04 08:16:18 PDT

I wonder if fishd has some thoughts here.

dfalcantara

Comment 6 2012-07-16 09:35:16 PDT

Ping?

Darin Fisher (:fishd, Google)

Comment 7 2012-07-16 16:11:50 PDT

Comment on attachment 150693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150693&action=review > Source/WebKit/chromium/public/WebFrame.h:324 > + virtual void reloadWithOverrideURL(const WebURL& overrideUrl, bool ignoreCache = false, bool resetState = false) = 0; there are too many boolean parameters. think how confusing the callsite will look when there is a string of bools? since resetState is implemented by calling setResetPageStateOnLoad, it seems like you could just expose that method on WebView / WebFrame. or, perhaps there is a WebViewClient / WebFrameClient method that you could hook to then call to change the page scale factor at the right time? > Source/WebKit/chromium/src/WebViewImpl.cpp:3223 > + currentItem->clearScrollPoint(); why do you want to clear the scroll position? normally, reloading a page preserves the scroll position.

dfalcantara

Comment 8 2012-07-16 16:25:34 PDT

Comment on attachment 150693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150693&action=review Will look into what you suggested. I'd tried looking for a WebFrameClient method to do this when I was looking for alternatives, but I could've missed something... >> Source/WebKit/chromium/src/WebViewImpl.cpp:3223 >> + currentItem->clearScrollPoint(); > > why do you want to clear the scroll position? normally, reloading a page preserves > the scroll position. The scrolling position often doesn't make sense when you've reloaded in this fashion, since you can end up with a completely different layout for a different URL.

Adam Barth

Comment 9 2012-07-30 09:37:50 PDT

> Will look into what you suggested. I'd tried looking for a WebFrameClient method to do this when I was looking for alternatives, but I could've missed something... @dfalcantara: Have you made any progress on the above?

dfalcantara

Comment 10 2012-07-30 10:20:17 PDT

(In reply to comment #9) > > Will look into what you suggested. I'd tried looking for a WebFrameClient method to do this when I was looking for alternatives, but I could've missed something... > > @dfalcantara: Have you made any progress on the above? No, unfortunately. I've been pulled into other random tasks for the last several weeks :/

Adam Barth

Comment 11 2012-08-20 14:07:28 PDT

> No, unfortunately. I've been pulled into other random tasks for the last several weeks :/ @dfalcantara: Do you plan to return to this patch or should we find someone else to take it over?

dfalcantara

Comment 12 2012-08-20 14:11:50 PDT

(In reply to comment #11) > @dfalcantara: Do you plan to return to this patch or should we find someone else to take it over? Returning, but I'm currently stuck trying to push through some other Chromium RDS CLs to unblock someone else. Coincidentally, that person will probably hit this bug too :/

dfalcantara

Comment 13 2012-08-28 17:02:29 PDT

Created attachment 161094 [details] Patch

dfalcantara

Comment 14 2012-08-28 17:04:07 PDT

Spoke with Darin and broke it apart so that the logic is split with Chromium. The patch just adds a way to reset the page scale and scroll position from Chromium during a reload; that half is located at https://chromiumcodereview.appspot.com/10889019

Adam Barth

Comment 15 2012-08-28 17:11:22 PDT

Comment on attachment 161094 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161094&action=review > Source/WebKit/chromium/public/WebView.h:265 > + virtual void resetScrollAndScaleState() = 0; Can you test that this plays nice with saveScrollAndScaleState and restoreScrollAndScaleState ?

Adam Barth

Comment 16 2012-08-28 17:12:10 PDT

Comment on attachment 161094 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161094&action=review >> Source/WebKit/chromium/public/WebView.h:265 >> + virtual void resetScrollAndScaleState() = 0; > > Can you test that this plays nice with saveScrollAndScaleState and restoreScrollAndScaleState ? Oh noes! Looks like I didn't add tests for restoreScrollAndScaleState in the first place. :(

dfalcantara

Comment 17 2012-08-28 17:34:19 PDT

I'll add a unit test for this guy once I'm sure that this pathway makes sense.

Darin Fisher (:fishd, Google)

Comment 18 2012-08-30 09:54:32 PDT

Comment on attachment 161094 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161094&action=review This approach looks good to me. > Source/WebKit/chromium/src/WebViewImpl.cpp:3406 > + m_page->mainFrame()->loader()->history()->saveDocumentAndScrollState(); previously, you explicitly reset the scroll offset. are you just relying on that happening as a side-effect of HistoryController::saveScrollPositionAndViewStateToItem copying the FrameView's scrollPosition field to HistoryItem's scrollPoint field? If so, how do you know that the FrameView's scrollPosition will be 0,0?

dfalcantara

Comment 19 2012-08-30 14:14:13 PDT

Created attachment 161554 [details] Patch

dfalcantara

Comment 20 2012-08-30 14:14:56 PDT

Comment on attachment 161094 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161094&action=review >> Source/WebKit/chromium/src/WebViewImpl.cpp:3406 >> + m_page->mainFrame()->loader()->history()->saveDocumentAndScrollState(); > > previously, you explicitly reset the scroll offset. are you just relying on that > happening as a side-effect of HistoryController::saveScrollPositionAndViewStateToItem > copying the FrameView's scrollPosition field to HistoryItem's scrollPoint field? > If so, how do you know that the FrameView's scrollPosition will be 0,0? Yeah, I'm relying on the HistoryItem copying over the scrollPoint field. The Page::setPageScaleFactor() call sets the ScrollView's scroll position field to (0,0) at the same time it resets the page scale. It seems to do so for all of the pathways as long as the page isn't already at (0,0), in which case we're fine. I did, however, just add a call to cache the scroll position in case the HistoryItem pulls the scroll position from there instead.

dfalcantara

Comment 21 2012-08-30 16:02:47 PDT

Created attachment 161576 [details] Patch

dfalcantara

Comment 22 2012-08-30 16:07:07 PDT

Adam: Added a unit test for the function. Is this along the lines of what you're looking for?

Adam Barth

Comment 23 2012-08-30 16:32:10 PDT

Comment on attachment 161576 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161576&action=review > Source/WebKit/chromium/public/WebView.h:265 > + // Reset the scroll and scale state. > + virtual void resetScrollAndScaleState() = 0; Can you add a comment explaining the interaction between reset and the save/restore mechanism above? > Source/WebKit/chromium/src/WebViewImpl.cpp:3403 > +void WebViewImpl::resetScrollAndScaleState() How does this clobber the saved state without calling resetSavedScrollAndScaleState() ? > Source/WebKit/chromium/tests/WebViewTest.cpp:448 > + // Confirm that resetting the page state resets both the scale and scroll position, as well > + // as overwrites the original parameters that were saved. Yes. I wasn't sure whether we'd want it to clobber the saved state or keep it, but it's good to test either way.

Adam Barth

Comment 24 2012-08-30 16:33:28 PDT

Comment on attachment 161576 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161576&action=review > Source/WebKit/chromium/src/WebViewImpl.h:230 > virtual void saveScrollAndScaleState(); > virtual void restoreScrollAndScaleState(); > + virtual void resetScrollAndScaleState(); Oh, actually I meant the interaction between these three functions. :)

dfalcantara

Comment 25 2012-08-30 16:36:40 PDT

Comment on attachment 161576 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161576&action=review >> Source/WebKit/chromium/src/WebViewImpl.h:230 >> + virtual void resetScrollAndScaleState(); > > Oh, actually I meant the interaction between these three functions. :) Hrm, all of the comments in the code refer to the interaction between the HistoryItem saving, not the WebView saving. I'm not entirely sure what the behavior should be in this case; should it just be wiped out?

Adam Barth

Comment 26 2012-08-30 16:51:48 PDT

Yeah, I think resetting to a fully known state is probably best.

dfalcantara

Comment 27 2012-08-31 12:17:33 PDT

Created attachment 161749 [details] Patch

dfalcantara

Comment 28 2012-08-31 12:24:25 PDT

Comment on attachment 161576 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161576&action=review >> Source/WebKit/chromium/src/WebViewImpl.cpp:3403 >> +void WebViewImpl::resetScrollAndScaleState() > > How does this clobber the saved state without calling resetSavedScrollAndScaleState() ? New version of the patch clobbers it. >> Source/WebKit/chromium/tests/WebViewTest.cpp:448 >> + // as overwrites the original parameters that were saved. > > Yes. I wasn't sure whether we'd want it to clobber the saved state or keep it, but it's good to test either way. Had some difficulties getting a unit test working to examine that, but it can probably be followed up on in https://bugs.webkit.org/show_bug.cgi?id=95267.

Adam Barth

Comment 29 2012-09-01 00:55:33 PDT

Comment on attachment 161749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161749&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:2895 > + m_page->mainFrame()->loader()->history()->saveDocumentAndScrollState(); I don't fully understand this line of the patch. It looks like fishd had questions about it too. Please check that he's happy with your answer before landing.

Darin Fisher (:fishd, Google)

Comment 30 2012-09-10 13:02:19 PDT

Comment on attachment 161749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161749&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:2888 > + FrameView* view = page()->mainFrame()->document()->view(); nit: move this code closer to where it is needed. in fact, probably want to do: if (FrameView* view = page()->...) view->cacheCurrentScrollPosition();

dfalcantara

Comment 31 2012-09-10 15:15:48 PDT

Created attachment 163225 [details] Patch

Adam Barth

Comment 32 2012-09-10 16:53:32 PDT

Comment on attachment 163225 [details] Patch I'm told that fishd gave a thumbs up in email. This patch looks good to me too.

WebKit Review Bot

Comment 33 2012-09-10 17:21:43 PDT

Comment on attachment 163225 [details] Patch Clearing flags on attachment: 163225 Committed r128133: <http://trac.webkit.org/changeset/128133>

WebKit Review Bot

Comment 34 2012-09-10 17:21:48 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebKit Misc.

Assignee

dfalcantara

Reported

2012-06-28 16:12 PDT

Modified

2012-09-10 17:21 PDT History

CC List

6 users Show

URL

Keywords

Depends on

Blocks

66687

Dependencies

tree graph