Bug 90222

Summary: [chromium, android] Reloading a page with a different user agent can cause the page to be zoomed in
Product: WebKit Reporter: dfalcantara
Component: WebKit Misc.Assignee: dfalcantara
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fishd, jamesr, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 66687    
Attachments:
Description Flags
Screenshots showing slashdot.org loads under different situations
none
Patch
fishd: review-
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

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-
DetailsFormatted DiffDiff
Patch (3.20 KB, patch)
2012-08-28 17:02 PDT, dfalcantara 		no flags
DetailsFormatted DiffDiff
Patch (3.33 KB, patch)
2012-08-30 14:14 PDT, dfalcantara 		no flags
DetailsFormatted DiffDiff
Patch (5.93 KB, patch)
2012-08-30 16:02 PDT, dfalcantara 		no flags
DetailsFormatted DiffDiff
Patch (6.34 KB, patch)
2012-08-31 12:17 PDT, dfalcantara 		no flags
DetailsFormatted DiffDiff
Patch (6.42 KB, patch)
2012-09-10 15:15 PDT, dfalcantara 		no flags
DetailsFormatted DiffDiff
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.