Bug 109946 – [Chromium] Add support for emulating legacy Android WebView 'setInitialScale' method

• Component: Media
• Status: RESOLVED FIXED

[Mikhail Naganov]

Android WebView has a method called 'setInitialScale' which allows to override any page scale that WebKit would apply based on the scaling logic or page's viewport meta tag. The setting is effective on next page load.

[Mikhail Naganov]

Created attachment 188578 [details]
Patch

Adam, Alexandre, another WebView blast, please take a look. I'm open to any proposals about naming.

[WebKit Review Bot]

Comment on attachment 188578 [details]
Patch

Attachment 188578 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16580885

New failing tests:
platform/chromium/fast/repaint/fixed-layout-360x240.html

[Mikhail Naganov]

Created attachment 188590 [details]
Fixed layout test failure

[Adam Barth]

Comment on attachment 188590 [details]
Fixed layout test failure

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

> Source/WebKit/chromium/public/WebView.h:234
> +    virtual void setInitialPageScaleFactorPermanently(float) = 0;

How does this function relate to setPageScaleFactorLimits?  It seems similar in the sense that it bounds what the viewport meta tag can do, but just in a different parameter.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3831
> +    } else if (isFixedLayoutModeEnabled()) {
> +        computePageScaleFactorLimits();
>      }

No { } in WebKit style.

> Source/WebKit/chromium/src/WebViewImpl.h:762
>      float m_minimumPageScaleFactor;
>      float m_maximumPageScaleFactor;
>      float m_initialPageScaleFactor;
> +    bool m_initialPageScaleFactorIsPermanent;
>      bool m_ignoreViewportTagMaximumScale;
>      bool m_pageScaleFactorIsSet;

Should we move all this page-scale related state into a sub object?  The relationship between all these floats and bools is getting pretty complex.

[Adam Barth]

This patch seems fine.  I suspect aelias has thoughts on naming.  This patch makes me think we should factor all this page-scale related state and logic into a separate class, but we don't necessarily need to do that in this patch.

[Alexandre Elias]

Comment on attachment 188590 [details]
Fixed layout test failure

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

How about:

virtual void setInitialPageScaleOverride(float) = 0;

and in WebViewImpl:
float m_initialPageScaleFactorOverride;

(instead of the bool; we can also continue to keep separately the viewport-tag value but just prioritize the other one in computePageScaleFactorLimits).

>> Source/WebKit/chromium/public/WebView.h:234
>> +    virtual void setInitialPageScaleFactorPermanently(float) = 0;
> 
> How does this function relate to setPageScaleFactorLimits?  It seems similar in the sense that it bounds what the viewport meta tag can do, but just in a different parameter.

We may want to call it "setInitialPageScaleOverride".

On a side topic, note that setPageScaleFactorLimits() as called from the embedder does *not* bound what the viewport tag can do.  It's currently only used when viewport tag is disabled.  I'm considering removing setPageScaleFactorLimits from WebView.h.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2959
> +    m_pageScaleFactorIsSet = false;

Are you sure you want to clear m_pageScaleFactorIsSet?  You say you want it to take effect on the next page load, but this may make it happen earlier than that.  I recommend not touching m_pageScaleFactorIsSet based on your description.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3829
> +    } else if (isFixedLayoutModeEnabled()) {

Please delete this clause and put || isFixedLayoutModeEnabled() in the earlier if() statement.

Also, one of your unit tests should have viewportEnabled == false since you care about this case.

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

[Mikhail Naganov]

Created attachment 188868 [details]
Comments addressed

[Mikhail Naganov]

Thanks for comments!

(In reply to comment #6)
> (From update of attachment 188590 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188590&action=review
> 
> How about:
> 
> virtual void setInitialPageScaleOverride(float) = 0;
> 
> and in WebViewImpl:
> float m_initialPageScaleFactorOverride;
> 
> (instead of the bool; we can also continue to keep separately the viewport-tag value but just prioritize the other one in computePageScaleFactorLimits).
>

Done. The code has become easier indeed, thanks for the suggestion!
 
> >> Source/WebKit/chromium/public/WebView.h:234
> >> +    virtual void setInitialPageScaleFactorPermanently(float) = 0;
> > 
> > How does this function relate to setPageScaleFactorLimits?  It seems similar in the sense that it bounds what the viewport meta tag can do, but just in a different parameter.
> 
> We may want to call it "setInitialPageScaleOverride".
> 
> On a side topic, note that setPageScaleFactorLimits() as called from the embedder does *not* bound what the viewport tag can do.  It's currently only used when viewport tag is disabled.  I'm considering removing setPageScaleFactorLimits from WebView.h.
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:2959
> > +    m_pageScaleFactorIsSet = false;
> 
> Are you sure you want to clear m_pageScaleFactorIsSet?  You say you want it to take effect on the next page load, but this may make it happen earlier than that.  I recommend not touching m_pageScaleFactorIsSet based on your description.
>

Not quite. I need the change to be effective at least on the next page load, if it will be applied sooner, that's not a problem. I don't see a better way of making the renderer to apply the changed override on the next reload of the same page other than resetting the `m_pageScaleFactorIsSet' flag.
 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:3829
> > +    } else if (isFixedLayoutModeEnabled()) {
> 
> Please delete this clause and put || isFixedLayoutModeEnabled() in the earlier if() statement.
> 

This is what I did in my first patch, and this broke a layout test, because during the call to `dispatchViewportPropertiesDidChange' fixed layout size was recalculated. Although, I changed the code, please take a look.

> Also, one of your unit tests should have viewportEnabled == false since you care about this case.

Yes, it is there--in `setInitialPageScaleFactorPermanently' I'm not setting viewportEnabled to `true'.

[Alexandre Elias]

This mixed "fixed layout on", "viewport off" mode is getting confusing, let's revisit that before going further with this.  I think we can avoid this mixed mode entirely now.

In https://bugs.webkit.org/show_bug.cgi?id=106021 you had to take the approach that you did because WebViewImpl::size() was in physical pixels, but it's now in DIP pixels.  So I think we can change dispatchViewportPropertiesDidChange() to early-return if !viewportEnabled() like it did before.  And if UseWideViewport is set, you can disable both enableViewport and enableFixedLayoutMode.  Then we can avoid all the if(fixedLayoutMode()).

If a WebFrameTest is failing because it specifies only fixed-layout mode, you may need to change it to enable viewport as well.

Can you give that a try and see if it works as you expect?

[Mikhail Naganov]

(In reply to comment #10)
> This mixed "fixed layout on", "viewport off" mode is getting confusing, let's revisit that before going further with this.  I think we can avoid this mixed mode entirely now.
> 
> In https://bugs.webkit.org/show_bug.cgi?id=106021 you had to take the approach that you did because WebViewImpl::size() was in physical pixels, but it's now in DIP pixels.  So I think we can change dispatchViewportPropertiesDidChange() to early-return if !viewportEnabled() like it did before.  And if UseWideViewport is set, you can disable both enableViewport and enableFixedLayoutMode.  Then we can avoid all the if(fixedLayoutMode()).
> 
> If a WebFrameTest is failing because it specifies only fixed-layout mode, you may need to change it to enable viewport as well.
> 
> Can you give that a try and see if it works as you expect?

Thanks for the suggestion! Yes, it looks like now we can turn on and off viewport and fixedlayout simultaneously and get rid of the "special" case when fixedlayout is on, but viewport is off.

I'm working on finalizing the patch.

[Mikhail Naganov]

Created attachment 189282 [details]
Eliminate "fixed layout enabled, viewport disabled" mode

Alexandre, please take a look.

I had a trouble calling computePageScaleFactorLimits when having viewport disabled, because computePageScaleFactorLimits expects m_pageDefined*ScaleFactor to be set. As you have mentioned, you are inclined to remove setPageScaleFactorLimits, so I went ahead and made computePageScaleFactorLimits to work without them, this doesn't seem to break anything.

[Mikhail Naganov]

BTW, is it true that "viewport enabled" settings has become redundant and the logic that is currently checking it should really check "isFixedLayoutModeEnabled"? Otherwise, it seems confusing to have two settings that must be kept in a synchronized state.

[Alexandre Elias]

Comment on attachment 189282 [details]
Eliminate "fixed layout enabled, viewport disabled" mode

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

General approach looks good.  I agree it should be safe to get rid of the m_pageDefinedMinimumScaleFactor != -1 early-return.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2964
> +void WebViewImpl::setInitialPageScaleOverride(float initialPageScaleFactor)

Call this variable initialPageScaleFactorOverride

> Source/WebKit/chromium/src/WebViewImpl.cpp:2965
> +{

Please start this method with:

if (m_initialPageScaleFactorOverride == initialPageScaleFactorOverride)
    return;

> Source/WebKit/chromium/src/WebViewImpl.cpp:2968
> +    if (m_initialPageScaleFactorOverride == -1 && !settings()->viewportEnabled())

This is messy, particularly the viewportEnabled() check I don't like.  And are you sure you can't just early-return after "m_initialPageScaleFactorOverride = initialPageScaleFactor;" in this case?  Do users actually expect anything to happen right away when they clear this value?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3180
> +        m_minimumPageScaleFactor = min(minPageScaleFactor, maxPageScaleFactor);

Just do:
m_minimumPageScaleFactor = minPageScaleFactor;
m_maximumPageScaleFactor = maxPageScaleFactor;

as those are constants.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3203
> +    if (!m_pageScaleFactorIsSet && (m_initialPageScaleFactorOverride != -1 || m_initialPageScaleFactor != -1)) {

This is a bit too verbose, I suggest reformulating:

float initialPageScaleFactor = (m_initialPageScaleFactorOverride != -1) ? m_initialPageScaleFactorOverride : m_initialPageScaleFactor;
if (!m_pageScaleFactorIsSet && initialPageScaleFactor != -1) {
    newPageScaleFactor = m_initialPageScaleFactor;
    m_pageScaleFactorIsSet = true;
}

[Alexandre Elias]

(In reply to comment #13)
> BTW, is it true that "viewport enabled" settings has become redundant and the logic that is currently checking it should really check "isFixedLayoutModeEnabled"? Otherwise, it seems confusing to have two settings that must be kept in a synchronized state.

They are indeed redundant, we should get rid of one or the other.  I was thinking of getting rid of isFixedLayoutModeEnabled because that is an implementation detail whereas viewport tag is the high-level feature.

[Mikhail Naganov]

Comment on attachment 189282 [details]
Eliminate "fixed layout enabled, viewport disabled" mode

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2968
>> +    if (m_initialPageScaleFactorOverride == -1 && !settings()->viewportEnabled())
> 
> This is messy, particularly the viewportEnabled() check I don't like.  And are you sure you can't just early-return after "m_initialPageScaleFactorOverride = initialPageScaleFactor;" in this case?  Do users actually expect anything to happen right away when they clear this value?

The current version of WebView jumps back to computed page scale once the user sets the override to the "use default" value. Without this code, our version does that only when viewport is enabled. This happens, because if viewport is disabled, ChromeClientImpl::dispatchViewportPropertiesDidChange bails out and doesn't update initialPageScaleFactor to the computed value. Thus, m_initialPageScaleFactor is never get updated from the default value, and having that m_initialPageScaleFactorOverride is also being set to -1, newPageScaleFactor in computePageScaleFactorLimits only receives the current page scale value.

Using page scale factor of 1 in the viewport disabled case is a good choice for now, because page width can only be set to viewport width in CSS pixels in this case.

[Mikhail Naganov]

Comment on attachment 189282 [details]
Eliminate "fixed layout enabled, viewport disabled" mode

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2968
>> +    if (m_initialPageScaleFactorOverride == -1 && !settings()->viewportEnabled())
> 
> This is messy, particularly the viewportEnabled() check I don't like.  And are you sure you can't just early-return after "m_initialPageScaleFactorOverride = initialPageScaleFactor;" in this case?  Do users actually expect anything to happen right away when they clear this value?

The current version of WebView jumps back to computed page scale once the user sets the override to the "use default" value. Without this code, our version does that only when viewport is enabled. This happens, because if viewport is disabled, ChromeClientImpl::dispatchViewportPropertiesDidChange bails out and doesn't update initialPageScaleFactor to the computed value. Thus, m_initialPageScaleFactor is never get updated from the default value, and having that m_initialPageScaleFactorOverride is also being set to -1, newPageScaleFactor in computePageScaleFactorLimits only receives the current page scale value.

Using page scale factor of 1 in the viewport disabled case is a good choice for now, because page width can only be set to viewport width in CSS pixels in this case.

[Alexandre Elias]

OK.  m_initialPageScale needs to be 1 when viewport is disabled in general, I think.  Let's do that in a more general manner.  I think this would actually fix a bug in desktop Chrome pinch zoom (http://crbug.com/162482 ).

We could make the block in computePageScaleFactorLimits look like:

float initialPageScaleFactor = m_initialPageScaleFactor;
if (!settings()->viewportEnabled())
   initialPageScaleFactor = 1;
if (m_initialPageScaleFactorOverride != -1)
   initialPageScaleFactor = m_initialPageScaleFactorOverride;
if (!m_pageScaleFactorIsSet && initialPageScaleFactor != -1) {
    newPageScaleFactor = m_initialPageScaleFactor;
    m_pageScaleFactorIsSet = true;
}

[Alexandre Elias]

Adding wjmaclean@ as an FYI that we expect to fix http://crbug.com/162482 with this.

Please also add a unit test for the viewport tag disabled case.

[Mikhail Naganov]

Created attachment 189486 [details]
Alexandre's comments addressed (again)

[Mikhail Naganov]

(In reply to comment #14)
> (From update of attachment 189282 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189282&action=review
> 
> General approach looks good.  I agree it should be safe to get rid of the m_pageDefinedMinimumScaleFactor != -1 early-return.
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:2964
> > +void WebViewImpl::setInitialPageScaleOverride(float initialPageScaleFactor)
> 
> Call this variable initialPageScaleFactorOverride
> 

Done.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:2965
> > +{
> 
> Please start this method with:
> 
> if (m_initialPageScaleFactorOverride == initialPageScaleFactorOverride)
>     return;
>

Done.
 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:2968
> > +    if (m_initialPageScaleFactorOverride == -1 && !settings()->viewportEnabled())
> 
> This is messy, particularly the viewportEnabled() check I don't like.  And are you sure you can't just early-return after "m_initialPageScaleFactorOverride = initialPageScaleFactor;" in this case?  Do users actually expect anything to happen right away when they clear this value?
>

Fixed as proposed in the comment #18
 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:3180
> > +        m_minimumPageScaleFactor = min(minPageScaleFactor, maxPageScaleFactor);
> 
> Just do:
> m_minimumPageScaleFactor = minPageScaleFactor;
> m_maximumPageScaleFactor = maxPageScaleFactor;
> 
> as those are constants.
>

Right, stupid me. Fixed.
 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:3203
> > +    if (!m_pageScaleFactorIsSet && (m_initialPageScaleFactorOverride != -1 || m_initialPageScaleFactor != -1)) {
> 
> This is a bit too verbose, I suggest reformulating:
> 
> float initialPageScaleFactor = (m_initialPageScaleFactorOverride != -1) ? m_initialPageScaleFactorOverride : m_initialPageScaleFactor;
> if (!m_pageScaleFactorIsSet && initialPageScaleFactor != -1) {
>     newPageScaleFactor = m_initialPageScaleFactor;
>     m_pageScaleFactorIsSet = true;
> }

Fixed as proposed in the comment #18

[Mikhail Naganov]

(In reply to comment #19)
> Adding wjmaclean@ as an FYI that we expect to fix http://crbug.com/162482 with this.
> 

I'm not sure how to verify this. I can enable pinch zoom via the flag, but not sure how to emulate pinch zoom using a single mouse.

> Please also add a unit test for the viewport tag disabled case.

In WebFrameTest.setInitialPageScaleFactorPermanently, I'm flipping the viewport state back and forth. Do you have any other scenario in mind?

[Alexandre Elias]

LGTM, looks much cleaner now.  Adam?


(In reply to comment #22)
> (In reply to comment #19)
> > Adding wjmaclean@ as an FYI that we expect to fix http://crbug.com/162482 with this.
> > 
> 
> I'm not sure how to verify this. I can enable pinch zoom via the flag, but not sure how to emulate pinch zoom using a single mouse.

Here are all the magic flags.  With this you can pinch zoom by holding right-click and dragging vertically:

out/Release/chrome --force-compositing-mode --enable-threaded-compositing --simulate-touch-screen-with-mouse --enable-pinch --enable-viewport --enable-fixed-layout

> 
> > Please also add a unit test for the viewport tag disabled case.
> 
> In WebFrameTest.setInitialPageScaleFactorPermanently, I'm flipping the viewport state back and forth. Do you have any other scenario in mind?

Looks good actually, thanks.

[Adam Barth]

Comment on attachment 189486 [details]
Alexandre's comments addressed (again)

ok

[WebKit Review Bot]

Comment on attachment 189486 [details]
Alexandre's comments addressed (again)

Clearing flags on attachment: 189486

Committed r143735: <http://trac.webkit.org/changeset/143735>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.