Bug 109584 - [Chromium] Add support for emulating legacy Android WebView 'LoadWithOverviewMode' setting
Summary: [Chromium] Add support for emulating legacy Android WebView 'LoadWithOverview...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Normal
Assignee: Mikhail Naganov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-12 07:20 PST by Mikhail Naganov
Modified: 2013-02-14 09:36 PST (History)
9 users (show)

See Also:


Attachments
Patch (5.30 KB, patch)
2013-02-12 07:24 PST, Mikhail Naganov
no flags Details | Formatted Diff | Diff
Alexandre's comments addressed (8.76 KB, patch)
2013-02-13 03:02 PST, Mikhail Naganov
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
Removed the getter from the public API (8.52 KB, patch)
2013-02-14 05:51 PST, Mikhail Naganov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Naganov 2013-02-12 07:20:10 PST
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.
Comment 1 Mikhail Naganov 2013-02-12 07:24:44 PST
Created attachment 187864 [details]
Patch

Alexandre, Adam, please take a look!
Comment 2 WebKit Review Bot 2013-02-12 07:35:25 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Eric Seidel (no email) 2013-02-12 09:17:12 PST
Can we test this?  I worry if it's not tested its going to be broken.
Comment 4 Mikhail Naganov 2013-02-12 09:25:05 PST
I have tests in Chromium: https://codereview.chromium.org/12217134/

Do you think we need a separate test in WebKit?
Comment 5 Alexandre Elias 2013-02-12 12:47:22 PST
Could you rename this setting to "initializeAtMinimumPageScale"?

WebFrameTest is a good place to test this, take a look at the test csae "FixedLayoutInitializeAtMinimumPageScale".
Comment 6 Mikhail Naganov 2013-02-13 03:02:15 PST
Created attachment 188046 [details]
Alexandre's comments addressed
Comment 7 Adam Barth 2013-02-13 11:22:56 PST
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.
Comment 8 Alexandre Elias 2013-02-13 11:24:39 PST
LGTM modulo Adam's last comment.
Comment 9 Mikhail Naganov 2013-02-13 14:06:12 PST
(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?
Comment 10 Mikhail Naganov 2013-02-14 05:43:53 PST
(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.
Comment 11 Mikhail Naganov 2013-02-14 05:51:14 PST
Created attachment 188331 [details]
Removed the getter from the public API
Comment 12 Mikhail Naganov 2013-02-14 05:55:29 PST
Committed r142875: <http://trac.webkit.org/changeset/142875>
Comment 13 Adam Barth 2013-02-14 09:36:41 PST
(Having a getter isn't such a horrible thing.  I just wanted to make sure you weren't just copy/pasting it.)