[Android WebView] Implement WebSettings.{get|set}LoadWithOverviewMode

[Android WebView] Implement WebSettings.{get|set}LoadWithOverviewMode

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.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184631

[mnaganov (inactive), 2013-02-12 17:27:26]

Depends on a webkit patch: https://bugs.webkit.org/show_bug.cgi?id=109584

[mnaganov (inactive), 2013-02-14 14:22:01]

The webkit patch is now committed. Ben, please start reviewing the change.

[benm (inactive), 2013-02-14 18:29:38]

lgtm

https://codereview.chromium.org/12217134/diff/7001/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(right):

https://codereview.chromium.org/12217134/diff/7001/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2242:
// set in the viewport tag,
Is this documented?

nit: s/,/./ ... I had to find something :)

[mnaganov (inactive), 2013-02-14 21:37:29]

OWNERS review needed (calling all Js :)

jcivelli@: content/browser/android and content/public/android
joi@: content/public/common
jamesr@: content/renderer and webkit/glue

[Jói, 2013-02-15 10:24:48]

LGTM for content/public.

[mnaganov (inactive), 2013-02-15 10:30:19]

https://codereview.chromium.org/12217134/diff/7001/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(right):

https://codereview.chromium.org/12217134/diff/7001/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2242:
// set in the viewport tag,
On 2013/02/14 18:29:38, benm wrote:
> Is this documented?
> 

No, figured out experimentally. The JavaDoc is barely minimal: "Sets whether the
WebView loads pages in overview mode". We should improve it.

> nit: s/,/./ ... I had to find something :)

Done, thanks!

[mnaganov (inactive), 2013-02-19 20:56:37]

Jay, James, can you please take a look?

[Jay Civelli, 2013-02-19 21:17:33]

lgtm

[jamesr, 2013-02-19 23:26:21]

I'm not sure the OnUpdate...() code will work - in some cases (for instance
RenderViewImpl::Initialize()) we just call webkit_preferences_.Apply() directly
and skip the newly added hook.

This feels more like it belongs on content::RendererPreferences or something
else slightly removed from webkit_glue since it isn't just mapped directly on to
settings.  Things like the zoom level are already on
content::RendererPreferences and have similar logic in place already to make
sure the proper change notifications are generated when the value is different.

https://codereview.chromium.org/12217134/diff/13001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12217134/diff/13001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2368: void
RenderViewImpl::WebPreferencesUpdated(const WebPreferences& old_prefs) {
I'd rather just do this inline in OnUpdateWebPreferences(), the function
indirection doesn't seem to buy much. Alternately, if this doesn't behave like
other WebPreferences in that it has extra state perhaps it should be managed
separately.

https://codereview.chromium.org/12217134/diff/13001/webkit/glue/webpreference...
File webkit/glue/webpreferences.cc (right):

https://codereview.chromium.org/12217134/diff/13001/webkit/glue/webpreference...
webkit/glue/webpreferences.cc:146: initialize_at_minimum_page_scale(true),
this defaults to true, but in java it looks like ContentSettings defaults to
false.  Is the difference intentional? Is it significant?

[mnaganov (inactive), 2013-02-20 10:36:49]

James, thanks for your comments!

> I'm not sure the OnUpdate...() code will work - in some cases (for instance
> RenderViewImpl::Initialize()) we just call webkit_preferences_.Apply()
directly
> and skip the newly added hook.

The constructor case seems to be the only one, and this is fine, as we don't
need to do anything on the setting initialization.

> This feels more like it belongs on content::RendererPreferences or something
> else slightly removed from webkit_glue since it isn't just mapped directly on
to
> settings.  Things like the zoom level are already on
> content::RendererPreferences and have similar logic in place already to make
> sure the proper change notifications are generated when the value is
different.

I disagree it doesn't mapped directly on settings--it does, it's a member of
WebSettings. As we can only apply settings in a single batch, reapplying them
from OnSetRendererPrefs doesn't make much sense. Please also see my comments
below.

https://codereview.chromium.org/12217134/diff/13001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12217134/diff/13001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2368: void
RenderViewImpl::WebPreferencesUpdated(const WebPreferences& old_prefs) {
On 2013/02/19 23:26:21, jamesr wrote:
> I'd rather just do this inline in OnUpdateWebPreferences(), the function
> indirection doesn't seem to buy much. Alternately, if this doesn't behave like
> other WebPreferences in that it has extra state perhaps it should be managed
> separately.

This setting resides in WebPreferences because it's used by ChromeClientImpl,
not by WebView itself. The reason we need to make a call to WebView is because
the latter doesn't reinitialize scale when reloading the same page.

In fact, a lot of WebKit settings require some actions to be done on changing
them, but this is hidden inside Settings.cpp in WebCore. However, this
particular setting isn't from WebCore, but rather from WebKit Chromium client,
where WebSettingsImpl doesn't have access to WebViewImpl and thus can't mimic
WebCore Settings' behaviour.

So I don't see a reason for making this a Renderer Preference, but I'm following
your advice on inlining the check inside WebPreferencesUpdated.

https://codereview.chromium.org/12217134/diff/13001/webkit/glue/webpreference...
File webkit/glue/webpreferences.cc (right):

https://codereview.chromium.org/12217134/diff/13001/webkit/glue/webpreference...
webkit/glue/webpreferences.cc:146: initialize_at_minimum_page_scale(true),
On 2013/02/19 23:26:21, jamesr wrote:
> this defaults to true, but in java it looks like ContentSettings defaults to
> false.  Is the difference intentional? Is it significant?

ContentSettings defaults are used for Android WebView only. WebView observer
RenderView creation and applies them. Defaults specified here are for Chrome and
Chrome for Android.

[mnaganov (inactive), 2013-02-21 21:25:48]

James, can you please take another look?

Those trans-Atlantic reviews have unpleasantly long turnaround time :(

[jamesr, 2013-02-21 22:35:13]

On 2013/02/20 10:36:49, Mikhail Naganov (Chromium) wrote:
> James, thanks for your comments!
> 
> > I'm not sure the OnUpdate...() code will work - in some cases (for instance
> > RenderViewImpl::Initialize()) we just call webkit_preferences_.Apply()
> directly
> > and skip the newly added hook.
> 
> The constructor case seems to be the only one, and this is fine, as we don't
> need to do anything on the setting initialization.
> 
> > This feels more like it belongs on content::RendererPreferences or something
> > else slightly removed from webkit_glue since it isn't just mapped directly
on
> to
> > settings.  Things like the zoom level are already on
> > content::RendererPreferences and have similar logic in place already to make
> > sure the proper change notifications are generated when the value is
> different.
> 
> I disagree it doesn't mapped directly on settings--it does, it's a member of
> WebSettings.

It doesn't map 1:1 - there are side effects of changing the value outside of
simply propagating the value.  webkit_glue::WebPreferences is relatively easy to
reason about in chromium since all it is is a simple pass-through struct that
maps straight into WebSettings.  Adding stateful side effects makes it more
difficult to reason about what it's doing.

> As we can only apply settings in a single batch, reapplying them
> from OnSetRendererPrefs doesn't make much sense. Please also see my comments
> below.
> 
>
https://codereview.chromium.org/12217134/diff/13001/content/renderer/render_v...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/12217134/diff/13001/content/renderer/render_v...
> content/renderer/render_view_impl.cc:2368: void
> RenderViewImpl::WebPreferencesUpdated(const WebPreferences& old_prefs) {
> On 2013/02/19 23:26:21, jamesr wrote:
> > I'd rather just do this inline in OnUpdateWebPreferences(), the function
> > indirection doesn't seem to buy much. Alternately, if this doesn't behave
like
> > other WebPreferences in that it has extra state perhaps it should be managed
> > separately.
> 
> This setting resides in WebPreferences because it's used by ChromeClientImpl,
> not by WebView itself. The reason we need to make a call to WebView is because
> the latter doesn't reinitialize scale when reloading the same page.

The ChromeClientImpl / WebView distinction shouldn't matter at all to code in
chromium.  Currently, WebPreferences just map into WebSettings and what happens
after that is no concern of the chromium code.

> 
> In fact, a lot of WebKit settings require some actions to be done on changing
> them, but this is hidden inside Settings.cpp in WebCore. However, this
> particular setting isn't from WebCore, but rather from WebKit Chromium client,
> where WebSettingsImpl doesn't have access to WebViewImpl and thus can't mimic
> WebCore Settings' behaviour.

That means going through the WebSettings API is not a great fit for this setting
- or at least, the auto-reset behavior isn't.

[jamesr, 2013-02-21 22:36:25]

On 2013/02/20 10:36:49, Mikhail Naganov (Chromium) wrote:
> This setting resides in WebPreferences because it's used by ChromeClientImpl,
> not by WebView itself. The reason we need to make a call to WebView is because
> the latter doesn't reinitialize scale when reloading the same page.

Is this the core behavior change?  You want different behavior on page reloads? 
How does this work with this patch - do you re-apply the WebPreferences on every
page load with a different value?

[jamesr, 2013-02-21 22:38:32]

https://codereview.chromium.org/12217134/diff/20001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12217134/diff/20001/content/renderer/render_v...
content/renderer/render_view_impl.cc:3330:
ViewMsg_Navigate_Type::RELOAD_ORIGINAL_REQUEST_URL);
if you just want to modify the behavior on page reload, it seems like perhaps
this is the line of code you want to be patching

[mnaganov (inactive), 2013-02-22 15:55:16]

James, thanks for explaining your point of view!

As this change of behaviour is only needed for emulating legacy Android WebView,
I decided to make the required action from WebView-specific code, to avoid
polluting the common code.

https://codereview.chromium.org/12217134/diff/20001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12217134/diff/20001/content/renderer/render_v...
content/renderer/render_view_impl.cc:3330:
ViewMsg_Navigate_Type::RELOAD_ORIGINAL_REQUEST_URL);
On 2013/02/21 22:38:32, jamesr wrote:
> if you just want to modify the behavior on page reload, it seems like perhaps
> this is the line of code you want to be patching

Thanks for pointing this out! But at this point I don't have information,
whether the setting value has been flipped. And introducing a special flag for
that seems to be of a doubtful usefulness for a feature that is only used for
emulating legacy behaviour of Android WebView.

[mnaganov (inactive), 2013-02-22 16:00:55]

Ben, can you please re-review the change?

James, can you please still look at the webkit/glue change?

[jamesr, 2013-02-22 20:47:06]

lgtm and thank you

[benm (inactive), 2013-02-25 12:37:04]

https://codereview.chromium.org/12217134/diff/28010/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/12217134/diff/28010/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:678:
public void resetScrollAndScaleState() {
does this need to be public?

https://codereview.chromium.org/12217134/diff/28010/android_webview/javatests...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(right):

https://codereview.chromium.org/12217134/diff/28010/android_webview/javatests...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1239:
mAwSettings.resetScrollAndScaleState();
Could we call this on awContents and avoid the method on AwSettings?

[mnaganov (inactive), 2013-02-25 12:59:23]

https://codereview.chromium.org/12217134/diff/28010/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/12217134/diff/28010/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:678:
public void resetScrollAndScaleState() {
On 2013/02/25 12:37:04, benm wrote:
> does this need to be public?

After addressing your second comment, now it really needs to be public.

https://codereview.chromium.org/12217134/diff/28010/android_webview/javatests...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(right):

https://codereview.chromium.org/12217134/diff/28010/android_webview/javatests...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1239:
mAwSettings.resetScrollAndScaleState();
On 2013/02/25 12:37:04, benm wrote:
> Could we call this on awContents and avoid the method on AwSettings?

OK. My concern was that ContentSettingsAdapter doesn't have access to
AwContents, but I think it can be easily fixed, and indeed this makes code
simpler.

[benm (inactive), 2013-02-25 17:53:07]

lgtm

[mnaganov (inactive), 2013-02-25 18:02:34]

Thanks, Ben!

Chris, can you please look at android_webview/common/render_view_messages.h?

[palmer, 2013-02-26 01:45:43]

LGTM from IPC security perspective.

[commit-bot: I haz the power, 2013-02-26 10:20:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/12217134/23019

[commit-bot: I haz the power, 2013-02-26 12:31:51]

Change committed as 184631