Bug 104767 – WebViewImpl::resetScrollAndScaleState() causes the page to render incorrectly

• Component: WebKit Misc.
• Status: RESOLVED FIXED

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

The patch set applied in the above bug is meant to avoid issues where the page would be zoomed in and scrolled incorrectly after a page has been reloaded with an override URL.  It does so by resetting the page's scale factor to 0.0f (which is a special value for HistoryController::restoreScrollPositionAndViewState()) and then saving it to the HistoryItem.

While this was working on Android, it was not working on ChromeOS when it landed (http://crbug.com/153907).  Recently, this stopped working on Android, as well, resulting in black page loads if called (http://crbug.com/165436).

There are two workarounds that get this working again:
* The original version of the patch in the original WebKit bug avoids this issue by directly setting the HistoryItem values instead of indirectly setting them through setPageScaleFactor(0.0).  Later versions of the patch flipped this around, apparently breaking things.

* Moving the resetScrollAndScaleState() call to RenderViewImpl::didFirstVisuallyNonEmptyLayout() from RenderViewImpl::didCommitProvisionalLoad() in chromium also appears to work, but this might cause a secondary reload.

Created attachment 178969 [details]
Patch

The attached patch goes with option 1 and changes the way the WebKit function works.

[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.

[Alexandre Elias]

Comment on attachment 178969 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178969&action=review

Note that I recommended this change because it's unsafe to set pageScaleFactor to 0: code elsewhere doesn't know what to do with that and will just shrink the document to a point.  The 0 value is only valid within a historyItem.

> Source/WebKit/chromium/src/WebViewImpl.cpp:-3093
> -    page()->setPageScaleFactor(0, IntPoint());

Let's keep this line but change it to "page()->setPageScaleFactor(1, IntPoint());" so that non-history-item-related callers still get the reset behavior.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3093
> +    // Clear out the values for the current history item.

Add extra comment: "This will prevent the history item from clobbering the value determined during page scale initialization, which may be less than 1."

> Source/WebKit/chromium/src/WebViewImpl.cpp:3095
> +    RefPtr<HistoryItem> currentItem = m_page->mainFrame()->loader()->history()->currentItem();

I'm thinking we should just move these lines to a new method "HistoryController::clearDocumentAndScrollState()".  That would be a reasonable method for HistoryController to have.

Created attachment 179098 [details]
Patch

[Alexandre Elias]

LGTM, thanks.

(In reply to comment #0)
> * Moving the resetScrollAndScaleState() call to RenderViewImpl::didFirstVisuallyNonEmptyLayout() from RenderViewImpl::didCommitProvisionalLoad() in chromium also appears to work, but this might cause a secondary reload.

Meant re-layout instead of reload.

[WebKit Review Bot]

Comment on attachment 179098 [details]
Patch

Clearing flags on attachment: 179098

Committed r137792: <http://trac.webkit.org/changeset/137792>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.