[Android WebView] Parametrize targetDensityDpi support

[Android WebView] Parametrize targetDensityDpi support

It is now possible to control targetDensityDpi and other quirks support when constructing AwContents

BUG=277157
R=benm@chromium.org

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

[mnaganov (inactive), 2013-08-22 13:47:27]

This is how targetSdkVersion check can be done for controlling bugwards
compatibility of WebView apps.

[benm (inactive), 2013-08-22 15:10:08]

https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContents.java:456:
isAccessFromFileURLsGrantedByDefault, layoutSizer, true);
I think the param should be false by default; embedders should decide whether to
enable quirks mode or not.

[mnaganov (inactive), 2013-08-22 15:12:43]

https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContents.java:456:
isAccessFromFileURLsGrantedByDefault, layoutSizer, true);
On 2013/08/22 15:10:08, benm wrote:
> I think the param should be false by default; embedders should decide whether
to
> enable quirks mode or not.

...but AwSettings.mSupportDeprecatedTargetDensityDPI has always been true prior
to this change!

[mkosiba (inactive), 2013-08-22 15:15:40]

https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContents.java:484:
boolean supportsLegacyQuirks) {
instead of a boolean how about making it an int? I think that'd be more
future-proof if we ever need to add more quirks 'levels' (hopefully not, but you
know, better safe than sorry!)

[benm (inactive), 2013-08-22 15:16:22]

https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContents.java:456:
isAccessFromFileURLsGrantedByDefault, layoutSizer, true);
And the problem is that we'll start to fail tests? Or something else?

In that case, I guess the test shell should enable quirks mode...?! Or, we need
tests for both in and out of quirks mode... :-/

[mnaganov (inactive), 2013-08-22 15:36:20]

https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContents.java:456:
isAccessFromFileURLsGrantedByDefault, layoutSizer, true);
On 2013/08/22 15:16:23, benm wrote:
> And the problem is that we'll start to fail tests? Or something else?
> 

Right. Because we have a test for target-densitydpi support.

> In that case, I guess the test shell should enable quirks mode...?! Or, we
need
> tests for both in and out of quirks mode... :-/

We can have different test shells targeted at different SDKs and containing
different tests.

https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContents.java:484:
boolean supportsLegacyQuirks) {
On 2013/08/22 15:15:40, mkosiba wrote:
> instead of a boolean how about making it an int? I think that'd be more
> future-proof if we ever need to add more quirks 'levels' (hopefully not, but
you
> know, better safe than sorry!)

If this will be an int, then we should just pass targetSdkVersion over here and
call it so. But in this case, we could just retrieve it from the app's context.

[mkosiba (inactive), 2013-08-22 15:54:17]

On 2013/08/22 15:36:20, Mikhail Naganov (Chromium) wrote:
>
https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
> File android_webview/java/src/org/chromium/android_webview/AwContents.java
> (right):
> 
>
https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
> android_webview/java/src/org/chromium/android_webview/AwContents.java:456:
> isAccessFromFileURLsGrantedByDefault, layoutSizer, true);
> On 2013/08/22 15:16:23, benm wrote:
> > And the problem is that we'll start to fail tests? Or something else?
> > 
> 
> Right. Because we have a test for target-densitydpi support.
> 
> > In that case, I guess the test shell should enable quirks mode...?! Or, we
> need
> > tests for both in and out of quirks mode... :-/
> 
> We can have different test shells targeted at different SDKs and containing
> different tests.

I'm not sure whether that's worth the overhead. I think it's simpler to write
two
tests - one that verifies the proper behavior in quirks mode and one that
verifies
the proper behavior in 'reqular' mode.
The downside is that we won't be verifying that enabling a quirk doesn't break
other
tests, but it's a hard problem - in order to be 100% sure that quirks don't
impact one
another you'd have to test 1 << (number of quirks) configurations.

AwTestBase already has the TestDependencyFactory that could be extended with a
quirks mode method the result value of which could be passed to the AwContents
constructor
in createDetachedAwTestContainerView - I suggest we use that (we already do it
for the
layoutSizer)

[benm (inactive), 2013-08-22 16:47:19]

On 2013/08/22 15:54:17, mkosiba wrote:
> On 2013/08/22 15:36:20, Mikhail Naganov (Chromium) wrote:
> >
>
https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
> > File android_webview/java/src/org/chromium/android_webview/AwContents.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/22909031/diff/4001/android_webview/java/src/o...
> > android_webview/java/src/org/chromium/android_webview/AwContents.java:456:
> > isAccessFromFileURLsGrantedByDefault, layoutSizer, true);
> > On 2013/08/22 15:16:23, benm wrote:
> > > And the problem is that we'll start to fail tests? Or something else?
> > > 
> > 
> > Right. Because we have a test for target-densitydpi support.
> > 
> > > In that case, I guess the test shell should enable quirks mode...?! Or, we
> > need
> > > tests for both in and out of quirks mode... :-/
> > 
> > We can have different test shells targeted at different SDKs and containing
> > different tests.
> 
> I'm not sure whether that's worth the overhead. I think it's simpler to write
> two
> tests - one that verifies the proper behavior in quirks mode and one that
> verifies
> the proper behavior in 'reqular' mode.
> The downside is that we won't be verifying that enabling a quirk doesn't break
> other
> tests, but it's a hard problem - in order to be 100% sure that quirks don't
> impact one
> another you'd have to test 1 << (number of quirks) configurations.
> 
> AwTestBase already has the TestDependencyFactory that could be extended with a
> quirks mode method the result value of which could be passed to the AwContents
> constructor
> in createDetachedAwTestContainerView - I suggest we use that (we already do it
> for the
> layoutSizer)

Right, we can just force the AwTestBase to create an AwContents with quirksMode
set to true in the ctor so that we don't break tests with the CL as is.

[mnaganov (inactive), 2013-08-23 09:39:42]

Ben, PTAL. I did what you have suggested. There is one more AwContents
construction in AwShellActivity. I'm now passing 'true' there as well.

[benm (inactive), 2013-08-23 09:58:17]

LGTM

Thanks Mikhail! Only suggestion would be to file a bug (and add it in a comment
above the tests where we are passing true for quirks) to track adding tests for
both quirky and correct behavior.

[mnaganov (inactive), 2013-08-23 10:16:54]

Thanks, Ben! Added a meta bug (http://crbug.com/278106) and a comment

[benm (inactive), 2013-08-23 10:32:49]

lgtm

[commit-bot: I haz the power, 2013-08-23 11:19:29]

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

[mkosiba (inactive), 2013-08-23 13:24:36]

https://chromiumcodereview.appspot.com/22909031/diff/4001/android_webview/jav...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://chromiumcodereview.appspot.com/22909031/diff/4001/android_webview/jav...
android_webview/java/src/org/chromium/android_webview/AwContents.java:484:
boolean supportsLegacyQuirks) {
On 2013/08/22 15:36:20, Mikhail Naganov (Chromium) wrote:
> On 2013/08/22 15:15:40, mkosiba wrote:
> > instead of a boolean how about making it an int? I think that'd be more
> > future-proof if we ever need to add more quirks 'levels' (hopefully not, but
> you
> > know, better safe than sorry!)
> 
> If this will be an int, then we should just pass targetSdkVersion over here
and
> call it so. But in this case, we could just retrieve it from the app's
context.

Agreed, we should just pass targetSdkVersion. We should not get it from the
app's context because doing that makes it harder to test.
Anyway, the boolean should be fine for now, we can always change it to an int
later.

[commit-bot: I haz the power, 2013-08-23 13:32:42]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[mnaganov (inactive), 2013-08-23 13:43:59]

Committed patchset #4 manually as r219274 (presubmit successful).