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

• Component: WebKit Misc.
• Status: RESOLVED FIXED

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.

Created attachment 150693 [details]
Patch

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]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

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]

I wonder if fishd has some thoughts here.

Ping?

[Darin Fisher (:fishd, Google)]

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.

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]

> 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?

(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]

> 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?

(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 :/

Created attachment 161094 [details]
Patch

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 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 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.  :(

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

[Darin Fisher (:fishd, Google)]

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?

Created attachment 161554 [details]
Patch

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.

Created attachment 161576 [details]
Patch

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

[Adam Barth]

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 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.  :)

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]

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

Created attachment 161749 [details]
Patch

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 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 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();

Created attachment 163225 [details]
Patch

[Adam Barth]

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 on attachment 163225 [details]
Patch

Clearing flags on attachment: 163225

Committed r128133: <http://trac.webkit.org/changeset/128133>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.