View scale changes are temporarily lost after restoring a page from the page cache

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

Steps to Reproduce (through the code!):

1. Set a non-1 viewScale.
2. Navigate.
3. Set a different non-1 viewScale.
4. Go back.

Expected:

WebCore's pageScaleFactor should be equal to the viewScale set in #3.
The transform on the tiles should be a scale equal to the viewScale set in #3.

Actual:

WebCore's pageScaleFactor is equal to the viewScale set in #1.
The transform on the tiles is a scale equal to the viewScale set in #1.

Notes:

This is happening because the pageScaleFactor stored on HistoryItem is WebCore's pageScaleFactor, which has
the viewScale multiplied into it. When we come back from the PageCache, HistoryController does a setPageScaleFactor() with
the stored scale (with the old viewScale multiplied into it), disregarding any viewScale changes since that item went into the PageCache.

Since WebKit2 keeps pageScale and viewScale separately, doing a small pinch zoom will push the correct pageScaleFactor to WebCore and recover from this situation.

To fix this, I think we're going to have to inform WebCore of the viewScale, something I had previously been trying to avoid -- but only for the purposes of the PageCache, so hopefully it will be OK.

[Tim Horton]

There's a second hilarity here where, once we fix HistoryController to apply the new viewScale when restoring the pageScale, since the Page's m_pageScaleFactor is already updated to the new value, setPageScaleFactor() will bail and fail to update the scale on the tile container layer.

This never mattered before because the scale that HistoryController handed to setPageScaleFactor() was always the same as the scale already applied to the layer that is coming back from the PageCache (which is no longer true).

I've worked around this by notifying the frames of a pageScaleFactor change if either of the scales is now different from what was stored in the history item -- I'm not sure this is the best fix. Alternatively, we could make setPageScaleFactor take a "no really, I mean it" parameter...

[Tim Horton]

... or, check with the compositor's notion of the current page scale factor and avoid bailing if it doesn't match m_pageScaleFactor. Or something.

[Tim Horton]

Created attachment 253027 [details]
Patch

[Tim Horton]

(In reply to comment #1)
> I've worked around this by notifying the frames of a pageScaleFactor change
> if either of the scales is now different from what was stored in the history
> item -- I'm not sure this is the best fix.

It turns out there was already a mechanism to do this that I just made use of for this patch (after reinventing it :D).

[Simon Fraser (smfr)]

Comment on attachment 253027 [details]
Patch

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

> Source/WebCore/page/Page.cpp:839
> +    m_viewScaleFactor = scale;
> +    PageCache::singleton().markPagesForDeviceOrPageScaleChanged(*this);
> +    PageCache::singleton().markPagesForFullStyleRecalc(*this);

Should this early-return if the scale hasn't changed?

[Said Abou-Hallawa]

Comment on attachment 253027 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1451
>  }

What does this function exactly do? Does it scale the view scale factor: new_scale_factor = current_scale_factor * scale? Or does it set the view scale factor: new_scale_factor = scale? I think it is the later and I think this function should be renamed setViewScaleFactor()?

[Tim Horton]

Comment on attachment 253027 [details]
Patch

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

>> Source/WebCore/page/Page.cpp:839
>> +    PageCache::singleton().markPagesForFullStyleRecalc(*this);
> 
> Should this early-return if the scale hasn't changed?

Probably! The WebKit2 one should too, probably.

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1451
>>  }
> 
> What does this function exactly do? Does it scale the view scale factor: new_scale_factor = current_scale_factor * scale? Or does it set the view scale factor: new_scale_factor = scale? I think it is the later and I think this function should be renamed setViewScaleFactor()?

You're talking about scaleView()? The naming of scaleView matches the longstanding scalePage() message name, which does the same thing. Not sure why, but I'm not going to change it now.

[Tim Horton]

http://trac.webkit.org/changeset/184290