Fix TODOs related to SDK 24

Fix TODOs related to SDK 24

Originally from Issue 2427543003 written by hush

BUG=546762, 543272
Review-Url: https://codereview.chromium.org/2738663003
Cr-Commit-Position: refs/heads/master@{#455347}
Committed: https://chromium.googlesource.com/chromium/src/+/299bf5a2daf2784c7cdd2917483733e2262efb55

[Shimi Zhang, 2017-03-07 19:32:42]

The CQ bit was checked by ctzsm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 19:35:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Shimi Zhang, 2017-03-07 20:18:05]

The CQ bit was unchecked by ctzsm@chromium.org

[Shimi Zhang, 2017-03-07 21:17:56]

ctzsm@chromium.org changed reviewers:
+ boliu@chromium.org

[Shimi Zhang, 2017-03-07 21:17:56]

Please review this CL, thanks :)

[boliu, 2017-03-08 00:02:25]

Remove the "resolve conflict" part of Cl description

lgtm after both comments are fixed

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

https://codereview.chromium.org/2738663003/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:88:
private boolean mOffscreenPreRaster = false;
don't change this line

[Shimi Zhang, 2017-03-08 01:14:35]

Description was changed from

==========
resolve conflicts on top of issue 2427543003: Fix TODOs related to SDK 24

BUG=546762, 543272
==========

to

==========
Apply issue 2427543003: Fix TODOs related to SDK 24

BUG=546762, 543272
==========

[Shimi Zhang, 2017-03-08 01:14:46]

The CQ bit was checked by ctzsm@chromium.org

[Shimi Zhang, 2017-03-08 01:14:47]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2738663003/#ps20001
(title: "Apply issue 2427543003: Fix TODOs related to SDK 24")

[Shimi Zhang, 2017-03-08 01:14:52]

The CQ bit was checked by ctzsm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 01:15:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2017-03-08 01:20:43]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2017-03-08 01:21:57]

I mean drop the "Apply issue 2427543003" part altogether. You can reference that
CL in the description saying it's originally written by hush etc if you want.
But don't put that in the first line. The first line is supposed to be a summary
of what the CL does, not how it was written.

[Shimi Zhang, 2017-03-08 01:29:16]

Description was changed from

==========
Apply issue 2427543003: Fix TODOs related to SDK 24

BUG=546762, 543272
==========

to

==========
Originally from Issue 2427543003 written by hush

BUG=546762, 543272
==========

[Shimi Zhang, 2017-03-08 01:33:49]

On 2017/03/08 01:21:57, boliu wrote:
> I mean drop the "Apply issue 2427543003" part altogether. You can reference
that
> CL in the description saying it's originally written by hush etc if you want.
> But don't put that in the first line. The first line is supposed to be a
summary
> of what the CL does, not how it was written.

how about now?

[boliu, 2017-03-08 01:40:31]

On 2017/03/08 01:33:49, Shimi Zhang wrote:
> On 2017/03/08 01:21:57, boliu wrote:
> > I mean drop the "Apply issue 2427543003" part altogether. You can reference
> that
> > CL in the description saying it's originally written by hush etc if you
want.
> > But don't put that in the first line. The first line is supposed to be a
> summary
> > of what the CL does, not how it was written.
> 
> how about now?

Err... "subject" is actually useless. It's only used as the title of this code
review. The actual commit takes its entire description from the "description"
field. So copy the subject as the first line of description, then we are good.

You are finding a lot of odd UX in our code review tool... but it's being
deprecated soon. hopefully the new thing is better

[Shimi Zhang, 2017-03-08 01:54:46]

Description was changed from

==========
Originally from Issue 2427543003 written by hush

BUG=546762, 543272
==========

to

==========
Fix TODOs related to SDK 24

Originally from Issue 2427543003 written by hush

BUG=546762, 543272
==========

[Shimi Zhang, 2017-03-08 02:01:26]

Ah, I see, so only description field matters for commit, and the first
line is summary, rest of them are detailed description.

[boliu, 2017-03-08 02:03:46]

On 2017/03/08 02:01:26, Shimi Zhang wrote:
> Ah, I see, so only description field matters for commit, and the first
> line is summary, rest of them are detailed description.

yep, lgtm

[Shimi Zhang, 2017-03-08 02:05:15]

The CQ bit was checked by ctzsm@chromium.org

[commit-bot: I haz the power, 2017-03-08 02:06:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 02:16:07]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488938715745830,
"parent_rev": "ec1588c5c4dd140af8075163d6f11cc64f37c0b4", "commit_rev":
"299bf5a2daf2784c7cdd2917483733e2262efb55"}

[commit-bot: I haz the power, 2017-03-08 02:17:06]

Description was changed from

==========
Fix TODOs related to SDK 24

Originally from Issue 2427543003 written by hush

BUG=546762, 543272
==========

to

==========
Fix TODOs related to SDK 24

Originally from Issue 2427543003 written by hush

BUG=546762, 543272

Review-Url: https://codereview.chromium.org/2738663003
Cr-Commit-Position: refs/heads/master@{#455347}
Committed:
https://chromium.googlesource.com/chromium/src/+/299bf5a2daf2784c7cdd29174837...
==========

[commit-bot: I haz the power, 2017-03-08 02:17:07]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/299bf5a2daf2784c7cdd29174837...

[Torne, 2017-03-10 15:53:41]

torne@chromium.org changed reviewers:
+ torne@chromium.org

[Torne, 2017-03-10 15:53:42]

https://codereview.chromium.org/2738663003/diff/20001/android_webview/glue/ja...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2738663003/diff/20001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2214:
mWebViewPrivate.super_startActivityForResult(intent, requestCode);
This change caused crbug.com/699972 - this function only exists on N and above.
On M, we need to keep calling View.startActivityForResult I think? It's not
clear if the reflection is actually necessary here or not; that function already
existed in M as far as I can tell.

[boliu, 2017-03-10 16:54:47]

https://codereview.chromium.org/2738663003/diff/20001/android_webview/glue/ja...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2738663003/diff/20001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2214:
mWebViewPrivate.super_startActivityForResult(intent, requestCode);
On 2017/03/10 15:53:42, Torne wrote:
> This change caused crbug.com/699972 - this function only exists on N and
above.
> On M, we need to keep calling View.startActivityForResult I think? It's not
> clear if the reflection is actually necessary here or not; that function
already
> existed in M as far as I can tell.

Huh, odd. How did this not throw an exception before then..?

Revert for now I guess?

[Torne, 2017-03-10 16:59:45]

On 2017/03/10 16:54:47, boliu wrote:
>
https://codereview.chromium.org/2738663003/diff/20001/android_webview/glue/ja...
> File
>
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
> (right):
> 
>
https://codereview.chromium.org/2738663003/diff/20001/android_webview/glue/ja...
>
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2214:
> mWebViewPrivate.super_startActivityForResult(intent, requestCode);
> On 2017/03/10 15:53:42, Torne wrote:
> > This change caused crbug.com/699972 - this function only exists on N and
> above.
> > On M, we need to keep calling View.startActivityForResult I think? It's not
> > clear if the reflection is actually necessary here or not; that function
> already
> > existed in M as far as I can tell.
> 
> Huh, odd. How did this not throw an exception before then..?

View.startActivityForResult is already there in M, but is @hide. Calling it by
reflection works, but we can't reference it directly because it's not in the
SDK.

WebViewPrivateAccess.super_startActivityForResult was added to N to enable us to
not need the reflection any more, but it's not there in M.

We'll need to keep using reflection for M.

> 
> Revert for now I guess?

[boliu, 2017-03-10 17:00:35]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2745623003/ by boliu@chromium.org.

The reason for reverting is: BUG=699972.

[boliu, 2017-03-10 17:03:45]

On 2017/03/10 16:59:45, Torne wrote:
> On 2017/03/10 16:54:47, boliu wrote:
> >
>
https://codereview.chromium.org/2738663003/diff/20001/android_webview/glue/ja...
> > File
> >
>
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2738663003/diff/20001/android_webview/glue/ja...
> >
>
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:2214:
> > mWebViewPrivate.super_startActivityForResult(intent, requestCode);
> > On 2017/03/10 15:53:42, Torne wrote:
> > > This change caused crbug.com/699972 - this function only exists on N and
> > above.
> > > On M, we need to keep calling View.startActivityForResult I think? It's
not
> > > clear if the reflection is actually necessary here or not; that function
> > already
> > > existed in M as far as I can tell.
> > 
> > Huh, odd. How did this not throw an exception before then..?
> 
> View.startActivityForResult is already there in M, but is @hide. Calling it by
> reflection works, but we can't reference it directly because it's not in the
> SDK.
> 
> WebViewPrivateAccess.super_startActivityForResult was added to N to enable us
to
> not need the reflection any more, but it's not there in M.
> 
> We'll need to keep using reflection for M.
> 
> > 
> > Revert for now I guess?

That's tricky... need a looong comment then