Make WebContentsDelegateAndroid pass more calls to Java.

Make WebContentsDelegateAndroid pass more calls to Java.

BUG=428784
Committed: https://crrev.com/ea83ff6e9f1c0743ca418e6c79036ae4abaadf32
Cr-Commit-Position: refs/heads/master@{#302997}

[Mathias Hällman, 2014-10-30 15:26:39]

mathiash@opera.com changed reviewers:
+ tedchoc@chromium.org

[Mathias Hällman, 2014-11-04 15:28:28]

On 2014/10/30 15:27:00, Mathias Hällman wrote:

@Ted, would you mind having a look? Patches are generally a problem for Opera,
and we figure even if the methods are not used in Java by Chrome it's a harmless
patch for Chromium to carry. Same goes for the other similar review I pushed
lately: https://codereview.chromium.org/686213003/

[Mathias Hällman, 2014-11-05 09:51:17]

mathiash@opera.com changed reviewers:
+ benm@chromium.org, nyquist@chromium.org

[Mathias Hällman, 2014-11-05 10:05:14]

On 2014/11/04 15:28:28, Mathias Hällman wrote:
> On 2014/10/30 15:27:00, Mathias Hällman wrote:
> 
> @Ted, would you mind having a look? Patches are generally a problem for Opera,
> and we figure even if the methods are not used in Java by Chrome it's a
harmless
> patch for Chromium to carry. Same goes for the other similar review I pushed
> lately: https://codereview.chromium.org/686213003/

nyquist, benm, can you two have a look? nyquist for chrome/ and benm for
components/. Or tedchoc for / if you're around of course. :)

[benm (inactive), 2014-11-05 14:44:27]

https://codereview.chromium.org/684133007/diff/1/components/web_contents_dele...
File
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java
(right):

https://codereview.chromium.org/684133007/diff/1/components/web_contents_dele...
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java:119:
return true;
same comment as in the .cc, is true the correct default here?

https://codereview.chromium.org/684133007/diff/1/components/web_contents_dele...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/684133007/diff/1/components/web_contents_dele...
components/web_contents_delegate_android/web_contents_delegate_android.cc:239:
return true;
is true the right return value here? It sounds a little counter intuitive to me
(but not certain)

https://codereview.chromium.org/684133007/diff/1/components/web_contents_dele...
components/web_contents_delegate_android/web_contents_delegate_android.cc:250:
return true;
is true the right return value here? It sounds a little counter intuitive to me
(but not certain)

[Mathias Hällman, 2014-11-05 15:01:33]

On 2014/11/05 14:44:27, benm wrote:
>
https://codereview.chromium.org/684133007/diff/1/components/web_contents_dele...
> File
>
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java
> (right):
> 
>
https://codereview.chromium.org/684133007/diff/1/components/web_contents_dele...
>
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java:119:
> return true;
> same comment as in the .cc, is true the correct default here?
> 
>
https://codereview.chromium.org/684133007/diff/1/components/web_contents_dele...
> File components/web_contents_delegate_android/web_contents_delegate_android.cc
> (right):
> 
>
https://codereview.chromium.org/684133007/diff/1/components/web_contents_dele...
> components/web_contents_delegate_android/web_contents_delegate_android.cc:239:
> return true;
> is true the right return value here? It sounds a little counter intuitive to
me
> (but not certain)
> 
>
https://codereview.chromium.org/684133007/diff/1/components/web_contents_dele...
> components/web_contents_delegate_android/web_contents_delegate_android.cc:250:
> return true;
> is true the right return value here? It sounds a little counter intuitive to
me
> (but not certain)

benm, thanks for having a look. I just took the default values from the base
class' implementations (content/public/browser/web_contents_delegate.cc). Let me
know if you want something else.

[Ted C, 2014-11-05 16:46:11]

lgtm w/ nit, but let benm confirm as well

https://codereview.chromium.org/684133007/diff/1/components/web_contents_dele...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/684133007/diff/1/components/web_contents_dele...
components/web_contents_delegate_android/web_contents_delegate_android.cc:239:
return true;
On 2014/11/05 14:44:27, benm wrote:
> is true the right return value here? It sounds a little counter intuitive to
me
> (but not certain)

These are both the defaults in web_contents_delegate.cc, but I do agree they are
a bit counter intuitive.

I still think it's best to just follow the existing delegate here and keep these
as true as written.

https://codereview.chromium.org/684133007/diff/1/components/web_contents_dele...
components/web_contents_delegate_android/web_contents_delegate_android.cc:264:
reinterpret_cast<intptr_t>(source_contents), opener_render_frame_id,
Instead of passing the raw pointer to java, can you pass
source_contents->GetJavaWebContents() here and for new_contents below?

We should try to stop passing raw web contents pointers up to javaland if at all
possible.

Actually, I guess we should change a lot of callers in this class to do that. 
Let's hold off on it for now and do this as a larger follow up.

https://codereview.chromium.org/684133007/diff/1/components/web_contents_dele...
components/web_contents_delegate_android/web_contents_delegate_android.cc:264:
reinterpret_cast<intptr_t>(source_contents), opener_render_frame_id,
Since the params don't all fit on a single line, each one should go on their
own.

[benm (inactive), 2014-11-05 17:45:03]

OK, thanks for double checking the default return, sgtm.

lgtm

[Mathias Hällman, 2014-11-06 09:08:09]

The CQ bit was checked by mathiash@opera.com

[commit-bot: I haz the power, 2014-11-06 09:09:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684133007/20001

[commit-bot: I haz the power, 2014-11-06 09:46:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-06 09:47:17]

Patchset 2 (id:??) landed as
https://crrev.com/ea83ff6e9f1c0743ca418e6c79036ae4abaadf32
Cr-Commit-Position: refs/heads/master@{#302997}