Bug 109584 – [Chromium] Add support for emulating legacy Android WebView 'LoadWithOverviewMode' setting

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mikhail Naganov]

The 'LoadWithOverviewMode' setting controls, whether the page needs to be scaled to fit contents into the viewport. Chrome on Android always adjusts page scale to fit contents. We need to make this behavior controllable.

[Mikhail Naganov]

Created attachment 187864 [details]
Patch

Alexandre, Adam, please take a look!

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

[Eric Seidel (no email)]

Can we test this?  I worry if it's not tested its going to be broken.

[Mikhail Naganov]

I have tests in Chromium: https://codereview.chromium.org/12217134/

Do you think we need a separate test in WebKit?

[Alexandre Elias]

Could you rename this setting to "initializeAtMinimumPageScale"?

WebFrameTest is a good place to test this, take a look at the test csae "FixedLayoutInitializeAtMinimumPageScale".

[Mikhail Naganov]

Created attachment 188046 [details]
Alexandre's comments addressed

[Adam Barth]

Comment on attachment 188046 [details]
Alexandre's comments addressed

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

> Source/WebKit/chromium/public/WebSettings.h:57
> +    virtual bool initializeAtMinimumPageScale() const = 0;

Why do you need a getter in the public API?  Most settings don't need one.

[Alexandre Elias]

LGTM modulo Adam's last comment.

[Mikhail Naganov]

(In reply to comment #7)
> (From update of attachment 188046 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188046&action=review
> 
> > Source/WebKit/chromium/public/WebSettings.h:57
> > +    virtual bool initializeAtMinimumPageScale() const = 0;
> 
> Why do you need a getter in the public API?  Most settings don't need one.

Thanks for the review!

I'm using it in Chromium glue layer to find out whether the setting was actually changed, see https://codereview.chromium.org/12217134/diff/1/webkit/glue/webpreferences.cc. The legacy implementation allows the setting to be applied on the next page reload. But WebViewImpl only allows to reset page scale on a new navigation, not on a reload. WebSettingsImpl, on the other hand, doesn't have an access to WebView, so it can't call resetScrollAndScaleState when the setting has changed. So I've ended up calling it from the Chromium glue layer.

Do you think there is any better way for solving this other than exposing a getter?

[Mikhail Naganov]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 188046 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=188046&action=review
> > 
> > > Source/WebKit/chromium/public/WebSettings.h:57
> > > +    virtual bool initializeAtMinimumPageScale() const = 0;
> > 
> > Why do you need a getter in the public API?  Most settings don't need one.
> 
> Thanks for the review!
> 
> I'm using it in Chromium glue layer to find out whether the setting was actually changed, see https://codereview.chromium.org/12217134/diff/1/webkit/glue/webpreferences.cc. The legacy implementation allows the setting to be applied on the next page reload. But WebViewImpl only allows to reset page scale on a new navigation, not on a reload. WebSettingsImpl, on the other hand, doesn't have an access to WebView, so it can't call resetScrollAndScaleState when the setting has changed. So I've ended up calling it from the Chromium glue layer.
> 
> Do you think there is any better way for solving this other than exposing a getter?

OK, I've found a workaround--I'll be using cached WebPreferences in RenderViewImpl in order to catch the moment the flag is being flipped. I'll submit the patch having the private getter removed from WebView.h as you have advised.

[Mikhail Naganov]

Created attachment 188331 [details]
Removed the getter from the public API

[Mikhail Naganov]

Committed r142875: <http://trac.webkit.org/changeset/142875>

[Adam Barth]

(Having a getter isn't such a horrible thing.  I just wanted to make sure you weren't just copy/pasting it.)