Reset page scale factor to 1.0 on navigation to a new page.

Reset page scale factor to 1.0 on navigation to a new page.

When navigating from a page that has page scale factor != 1 (is zoomed) to a new page, the existing code will display the new page at the current page scale factor. Instead, we wish to reset the page scale factor to 1 for the new page.


BUG=162482

[wjmaclean, 2013-04-30 19:35:09]

aelias@ - is this "resetting" behaviour OK for Clank, or should we preserve the
existing behaviour for Clank while implementing the new behaviour for
ChromeOS/Win/etc.?

[aelias_OOO_until_Jul13, 2013-04-30 19:44:20]

It's not OK for Android, nor do I think this is the right fix for desktop
either.  Setting "m_pageScaleFactorIsSet = false" is supposed to cause
computePageScaleFactorAndLimits to reset the scale on the next layout, we should
find out why that's failing to happen on desktop.

[wjmaclean, 2013-04-30 19:49:23]

On 2013/04/30 19:44:20, aelias wrote:
> It's not OK for Android, nor do I think this is the right fix for desktop
> either.  Setting "m_pageScaleFactorIsSet = false" is supposed to cause
> computePageScaleFactorAndLimits to reset the scale on the next layout, we
should
> find out why that's failing to happen on desktop.

Ahh, did not know that ... I'll trace it through then.

[wjmaclean, 2013-04-30 20:04:14]

On 2013/04/30 19:49:23, wjmaclean wrote:
> On 2013/04/30 19:44:20, aelias wrote:
> > It's not OK for Android, nor do I think this is the right fix for desktop
> > either.  Setting "m_pageScaleFactorIsSet = false" is supposed to cause
> > computePageScaleFactorAndLimits to reset the scale on the next layout, we
> should
> > find out why that's failing to happen on desktop.
> 
> Ahh, did not know that ... I'll trace it through then.

It seems computePageScaleFactorAndLimits is getting called from
WebViewImpl::layoutUpdated, but only if the viewport tag is enabled.

[aelias_OOO_until_Jul13, 2013-04-30 21:05:18]

OK, seems reasonable.  I had originally hid that code behind if
(enable_viewport) to be conservative and not affect desktop, but let's try to
unify the codepaths.

https://codereview.chromium.org/14643011/diff/6001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/14643011/diff/6001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebViewImpl.cpp:3003: // If we're in viewport tag
mode, we need to layout to obtain the latest
OK, in that case, can you also remove the conditional here and make this the
only path?

[wjmaclean, 2013-05-01 17:27:01]

Fixed and added a layout test. Adam, can you please take a look now?

[aelias_OOO_until_Jul13, 2013-05-01 19:22:49]

lgtm

[jamesr, 2013-05-03 17:55:58]

You seem to be moving slightly more code out of the viewportEnabled guard than I
would expect.

https://codereview.chromium.org/14643011/diff/10001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/14643011/diff/10001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.cpp:3667: // Contents size is an input to
the page scale limits, so a good time to
do you really need to do all this work on every layoutUpdated()? can you skip it
when the page scale factor limits are 1.0/1.0?

[wjmaclean, 2013-05-03 18:26:17]

> You seem to be moving slightly more code out of the viewportEnabled guard than
I would expect.

Not sure I follow. I'm assuming that computePageScaleFactorLimits() can set
needsLayout(), so both parts need to come out. Or am I missing something?

https://codereview.chromium.org/14643011/diff/10001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/14643011/diff/10001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.cpp:3667: // Contents size is an input to
the page scale limits, so a good time to
On 2013/05/03 17:55:58, jamesr wrote:
> do you really need to do all this work on every layoutUpdated()? can you skip
it
> when the page scale factor limits are 1.0/1.0?

I think so ... will upload new patch shortly.

[aelias_OOO_until_Jul13, 2013-05-03 21:00:50]

https://codereview.chromium.org/14643011/diff/10001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/14643011/diff/10001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebViewImpl.cpp:3667: // Contents size is an input to
the page scale limits, so a good time to
We shouldn't avoid calling this based on pageScaleFactorLimits being 1.0/1.0,
that is circular.  If we really wanted to avoid calling this we should have a
new setting along the lines of "pageScaleFactorLockedTo1".

But I don't understand your concern about "all this work".  It's some O(1)
arithmetic, it doesn't walk any data structures.  If nothing changes, it won't
relayout either.  I think we should just always call it.

[aelias_OOO_until_Jul13, 2013-05-13 10:36:20]

I just uploaded a big refactoring for this area of the code at
https://codereview.chromium.org/14813025/ that obsoletes this patch.  It
addresses James's concern above by hiding all the code behind a dirty flag. 
After that lands, you should be able to call
setInitialPageScaleFactorOverride(1.0f); from the Chromium side to get the reset
behavior you want.

[aelias_OOO_until_Jul13, 2013-06-07 19:08:40]

Closing this as my refactoring landed.  Please try calling
setInitialPageScaleFactorOverride(1.0f); from the Chromium side instead.