android webview: setShouldReuseGlobalForUnownedMainFrame

android webview: setShouldReuseGlobalForUnownedMainFrame

For apps targeting M and below. Break this behavior for apps targetting
N and above.

BUG=593984
Committed: https://crrev.com/1a00f26b150c8bcd160a53694e003c48c33f6d89
Cr-Commit-Position: refs/heads/master@{#385517}

[boliu, 2016-04-05 17:53:49]

boliu@chromium.org changed reviewers:
+ sgurun@chromium.org, torne@chromium.org

[boliu, 2016-04-05 17:53:49]

Depends on https://codereview.chromium.org/1816983002.

And probably should write a test for this too

https://codereview.chromium.org/1859163002/diff/1/android_webview/glue/java/s...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/1859163002/diff/1/android_webview/glue/java/s...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:237:
final boolean allowEmptyDocumentPersistence = mAppTargetSdkVersion <=
Build.VERSION_CODES.M;
is this safe?

[sgurun-gerrit only, 2016-04-05 18:24:05]

On 2016/04/05 17:53:49, boliu wrote:
> Depends on https://codereview.chromium.org/1816983002.
> 
> And probably should write a test for this too
> 
>
https://codereview.chromium.org/1859163002/diff/1/android_webview/glue/java/s...
> File
>
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
> (right):
> 
>
https://codereview.chromium.org/1859163002/diff/1/android_webview/glue/java/s...
>
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:237:
> final boolean allowEmptyDocumentPersistence = mAppTargetSdkVersion <=
> Build.VERSION_CODES.M;
> is this safe?

yes needs a test to catch regressions.

[sgurun-gerrit only, 2016-04-05 18:24:15]

https://codereview.chromium.org/1859163002/diff/1/android_webview/glue/java/s...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/1859163002/diff/1/android_webview/glue/java/s...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:237:
final boolean allowEmptyDocumentPersistence = mAppTargetSdkVersion <=
Build.VERSION_CODES.M;
On 2016/04/05 17:53:49, boliu wrote:
> is this safe?

I think that is the best we can do at the moment.Another alternative is always
setting allowEmptyDocumentPersistence to true here, and in the internal glue
layer, test against Build.VERSION_CODES.N.

I still prefer this one.

https://codereview.chromium.org/1859163002/diff/1/android_webview/javatests/s...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java
(right):

https://codereview.chromium.org/1859163002/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:385:
supportsLegacyQuirks, false /* allowEmptyDocumentPersistence */);
does any aw test expect this to be true? we will see I think.

[boliu, 2016-04-05 21:42:05]

https://codereview.chromium.org/1859163002/diff/1/android_webview/javatests/s...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java
(right):

https://codereview.chromium.org/1859163002/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:385:
supportsLegacyQuirks, false /* allowEmptyDocumentPersistence */);
On 2016/04/05 18:24:15, sgurun wrote:
> does any aw test expect this to be true? we will see I think.

Obivously not, since tests are green without this CL

[sgurun-gerrit only, 2016-04-05 21:52:49]

On 2016/04/05 21:42:05, boliu wrote:
>
https://codereview.chromium.org/1859163002/diff/1/android_webview/javatests/s...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java
> (right):
> 
>
https://codereview.chromium.org/1859163002/diff/1/android_webview/javatests/s...
>
android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:385:
> supportsLegacyQuirks, false /* allowEmptyDocumentPersistence */);
> On 2016/04/05 18:24:15, sgurun wrote:
> > does any aw test expect this to be true? we will see I think.
> 
> Obivously not, since tests are green without this CL

:) yeah

[boliu, 2016-04-05 22:40:11]

boliu@chromium.org changed reviewers:
+ dcheng@chromium.org, sievers@chromium.org

[boliu, 2016-04-05 22:40:12]

added tests

dcheng for common_param_traits_macros.h
sievers for rest of content/

[dcheng, 2016-04-05 22:41:59]

lgtm for ipc changes

[Torne, 2016-04-06 09:21:56]

On 2016/04/05 18:24:15, sgurun wrote:
>
https://codereview.chromium.org/1859163002/diff/1/android_webview/glue/java/s...
> File
>
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
> (right):
> 
>
https://codereview.chromium.org/1859163002/diff/1/android_webview/glue/java/s...
>
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:237:
> final boolean allowEmptyDocumentPersistence = mAppTargetSdkVersion <=
> Build.VERSION_CODES.M;
> On 2016/04/05 17:53:49, boliu wrote:
> > is this safe?
> 
> I think that is the best we can do at the moment.Another alternative is always
> setting allowEmptyDocumentPersistence to true here, and in the internal glue
> layer, test against Build.VERSION_CODES.N.
> 
> I still prefer this one.

Normally we shouldn't assume <=N is enough, but at this point it's probably
okay.

>
https://codereview.chromium.org/1859163002/diff/1/android_webview/javatests/s...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java
> (right):
> 
>
https://codereview.chromium.org/1859163002/diff/1/android_webview/javatests/s...
>
android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:385:
> supportsLegacyQuirks, false /* allowEmptyDocumentPersistence */);
> does any aw test expect this to be true? we will see I think.

[Torne, 2016-04-06 09:23:30]

We should try to find somewhere in the API docs to mention this, but I'm not
sure where makes sense. We should also make sure it goes into the release notes.

Code LGTM.

[sgurun-gerrit only, 2016-04-06 16:19:26]

On 2016/04/06 09:23:30, Torne wrote:
> We should try to find somewhere in the API docs to mention this, but I'm not
> sure where makes sense. We should also make sure it goes into the release
notes.
> 
> Code LGTM.

lgtm

[no sievers, 2016-04-06 17:46:53]

content lgtm

https://codereview.chromium.org/1859163002/diff/20001/content/public/common/w...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/1859163002/diff/20001/content/public/common/w...
content/public/common/web_preferences.h:221: bool
resue_global_for_unowned_main_frame;
Can you add a comment that this is for WebView and backwards-compatibility of
certain apps only (you can probably summarize the comment from 'Settings.in')?

[boliu, 2016-04-06 17:55:40]

I'm gonna keep the <= M part. torne/sgurun, holler if you don't agree

https://codereview.chromium.org/1859163002/diff/20001/content/public/common/w...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/1859163002/diff/20001/content/public/common/w...
content/public/common/web_preferences.h:221: bool
resue_global_for_unowned_main_frame;
On 2016/04/06 17:46:53, sievers wrote:
> Can you add a comment that this is for WebView and backwards-compatibility of
> certain apps only (you can probably summarize the comment from 'Settings.in')?

Done.

[Torne, 2016-04-06 17:58:39]

On 2016/04/06 17:55:40, boliu wrote:
> I'm gonna keep the <= M part. torne/sgurun, holler if you don't agree

Yes, I think it's okay to keep this for now. If we really need to change our
minds we can do so later since it's loosening a restriction, not adding one.

>
https://codereview.chromium.org/1859163002/diff/20001/content/public/common/w...
> File content/public/common/web_preferences.h (right):
> 
>
https://codereview.chromium.org/1859163002/diff/20001/content/public/common/w...
> content/public/common/web_preferences.h:221: bool
> resue_global_for_unowned_main_frame;
> On 2016/04/06 17:46:53, sievers wrote:
> > Can you add a comment that this is for WebView and backwards-compatibility
of
> > certain apps only (you can probably summarize the comment from
'Settings.in')?
> 
> Done.

[boliu, 2016-04-06 17:59:26]

The CQ bit was checked by boliu@chromium.org

[boliu, 2016-04-06 17:59:26]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
dcheng@chromium.org, sievers@chromium.org, torne@chromium.org
Link to the patchset: https://codereview.chromium.org/1859163002/#ps40001
(title: "comment")

[commit-bot: I haz the power, 2016-04-06 17:59:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1859163002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1859163002/40001

[commit-bot: I haz the power, 2016-04-06 19:08:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-06 19:10:09]

Description was changed from

==========
android webview: setShouldReuseGlobalForUnownedMainFrame

For apps targeting M and below. Break this behavior for apps targetting
N and above.

BUG=593984
==========

to

==========
android webview: setShouldReuseGlobalForUnownedMainFrame

For apps targeting M and below. Break this behavior for apps targetting
N and above.

BUG=593984

Committed: https://crrev.com/1a00f26b150c8bcd160a53694e003c48c33f6d89
Cr-Commit-Position: refs/heads/master@{#385517}
==========

[commit-bot: I haz the power, 2016-04-06 19:10:11]

Patchset 3 (id:??) landed as
https://crrev.com/1a00f26b150c8bcd160a53694e003c48c33f6d89
Cr-Commit-Position: refs/heads/master@{#385517}