[Android WebView] Wire up Blink's UseLegacyBackgroundSizeShorthandBehavior setting

[Android WebView] Wire up Blink's UseLegacyBackgroundSizeShorthandBehavior setting

R=benm@chromium.org
BUG=277157
TEST=AwSettingsTest#testUseLegacyBackgroundSizeShorthandBehavior

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

[benm (inactive), 2013-08-28 16:51:46]

Thanks Mikhail. lgtm modulo question about adding booleans.

https://codereview.chromium.org/23483005/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/23483005/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:86:
private final boolean mUseLegacyBackgroundSizeShorthandBehavior;
Should we be adding a new boolean for each quirk? Or could we just set a
mUseQuirks boolean and use that in each of the getters for quirky behaviors?

+joth as I think he had similar thoughts in the crbug.

[mnaganov (inactive), 2013-08-28 16:59:19]

https://codereview.chromium.org/23483005/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/23483005/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:86:
private final boolean mUseLegacyBackgroundSizeShorthandBehavior;
On 2013/08/28 16:51:46, benm wrote:
> Should we be adding a new boolean for each quirk? Or could we just set a
> mUseQuirks boolean and use that in each of the getters for quirky behaviors?
> 
> +joth as I think he had similar thoughts in the crbug.

Apparently, adding an argument for each quirk to the AwContents constructor
doesn't scale :) Perhaps, we should have a struct-like class for configuring
these if we plan to manage each quirk individually. But I'm for disabling all of
them at once for apps targeted to K, as otherwise it will be a really heavy
burden to specify how WebView works in each API version.

[mnaganov (inactive), 2013-08-28 17:24:57]

On 2013/08/28 16:59:19, Mikhail Naganov (Chromium) wrote:
>
https://codereview.chromium.org/23483005/diff/1/android_webview/java/src/org/...
> File android_webview/java/src/org/chromium/android_webview/AwSettings.java
> (right):
> 
>
https://codereview.chromium.org/23483005/diff/1/android_webview/java/src/org/...
> android_webview/java/src/org/chromium/android_webview/AwSettings.java:86:
> private final boolean mUseLegacyBackgroundSizeShorthandBehavior;
> On 2013/08/28 16:51:46, benm wrote:
> > Should we be adding a new boolean for each quirk? Or could we just set a
> > mUseQuirks boolean and use that in each of the getters for quirky behaviors?
> > 
> > +joth as I think he had similar thoughts in the crbug.
> 
> Apparently, adding an argument for each quirk to the AwContents constructor
> doesn't scale :) Perhaps, we should have a struct-like class for configuring
> these if we plan to manage each quirk individually. But I'm for disabling all
of
> them at once for apps targeted to K, as otherwise it will be a really heavy
> burden to specify how WebView works in each API version.

Actually, Ben was asking about not having AwSettings' setting per quirk, I have
misunderstood the question. Also makes sense to me, will update the patch.

[mnaganov (inactive), 2013-08-28 17:30:45]

https://codereview.chromium.org/23483005/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/23483005/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:86:
private final boolean mUseLegacyBackgroundSizeShorthandBehavior;
On 2013/08/28 16:51:46, benm wrote:
> Should we be adding a new boolean for each quirk? Or could we just set a
> mUseQuirks boolean and use that in each of the getters for quirky behaviors?
> 
> +joth as I think he had similar thoughts in the crbug.

Done.

[benm (inactive), 2013-08-28 17:59:45]

lgtm

[joth, 2013-08-28 20:07:32]

lgtm too

thanks

https://codereview.chromium.org/23483005/diff/2/android_webview/native/aw_set...
File android_webview/native/aw_settings.cc (right):

https://codereview.chromium.org/23483005/diff/2/android_webview/native/aw_set...
android_webview/native/aw_settings.cc:208:
Java_AwSettings_getSupportLegacyQuirksLocked(env, obj);
nit: the unequal indent 206 & 207 (and if more are added)  makes this hard to
scan

either rely on the defaults being false; hence:-
if (Java_AwSettings_getSupportLegacyQuirksLocked(env, obj)) {
  a = true;
  b = true;
}

or perhaps format as

bool support_quirks = Java_AwSettings_getSupportLegacyQuirksLocked(env, obj);
a = support_quirks;
b = support_quirks;

[mnaganov (inactive), 2013-08-29 11:49:25]

https://codereview.chromium.org/23483005/diff/2/android_webview/native/aw_set...
File android_webview/native/aw_settings.cc (right):

https://codereview.chromium.org/23483005/diff/2/android_webview/native/aw_set...
android_webview/native/aw_settings.cc:208:
Java_AwSettings_getSupportLegacyQuirksLocked(env, obj);
On 2013/08/28 20:07:33, joth wrote:
> nit: the unequal indent 206 & 207 (and if more are added)  makes this hard to
> scan
> 
> either rely on the defaults being false; hence:-
> if (Java_AwSettings_getSupportLegacyQuirksLocked(env, obj)) {
>   a = true;
>   b = true;
> }
> 
> or perhaps format as
> 
> bool support_quirks = Java_AwSettings_getSupportLegacyQuirksLocked(env, obj);
> a = support_quirks;
> b = support_quirks;
> 
> 
> 

The latter pattern seems to be more compliant with the rest of the function, so
I've changed to it.

[mnaganov (inactive), 2013-08-29 11:53:22]

Need an OWNER's review:

jamesr@: content/renderer and webkit/
joi@: content/public

[Jói, 2013-08-29 11:56:50]

//content/public LGTM.

[mnaganov (inactive), 2013-08-30 19:52:48]

jamesr@: Ping! Please take a look, the change is trivially simple.

[jamesr, 2013-08-30 20:49:27]

lgtm

[commit-bot: I haz the power, 2013-08-30 21:07:44]

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

[commit-bot: I haz the power, 2013-08-31 01:10:02]

Change committed as 220710