Move WebContentsDelegateAndroid to content/components

Move WebContentsDelegateAndroid to content/components

1. Move chrome/browser/component/web_contents_delegate_android to
   content/components/web_contents_delegate_android.
2. Rename package org.chromium.chrome.browser.component.
   web_contents_delegate_android to org.chromium.content.components.
   web_contents_delegate_android.
3. Remove shouldOverrideUrlLoading from WebContentsDelegateAndroid
   interface, which is no longer required.

I will follow up with a second change to completely remove
chrome/browser/component/web_contents_delegate_android once
external dependencies on it have been removed.

BUG=157575
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167670

[jam, 2012-11-12 17:24:39]

lgtm

https://codereview.chromium.org/11365199/diff/1/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/11365199/diff/1/chrome/chrome.gyp#newcode1054
chrome/chrome.gyp:1054:
'../content/content.gyp:web_contents_delegate_android_java',
nit: order

https://codereview.chromium.org/11365199/diff/1/content/components/web_conten...
File
content/components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/11365199/diff/1/content/components/web_conten...
content/components/web_contents_delegate_android/web_contents_delegate_android.cc:53:
const content::OpenURLParams& params) {
ditto

https://codereview.chromium.org/11365199/diff/1/content/components/web_conten...
File
content/components/web_contents_delegate_android/web_contents_delegate_android.h
(right):

https://codereview.chromium.org/11365199/diff/1/content/components/web_conten...
content/components/web_contents_delegate_android/web_contents_delegate_android.h:43:
class WebContentsDelegateAndroid : public content::WebContentsDelegate {
nit: get rid of content:: everywhere

[John Knottenbelt, 2012-11-12 18:07:46]

https://codereview.chromium.org/11365199/diff/1/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/11365199/diff/1/chrome/chrome.gyp#newcode1054
chrome/chrome.gyp:1054:
'../content/content.gyp:web_contents_delegate_android_java',
On 2012/11/12 17:24:40, John Abd-El-Malek wrote:
> nit: order

Done.

https://codereview.chromium.org/11365199/diff/1/content/components/web_conten...
File
content/components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/11365199/diff/1/content/components/web_conten...
content/components/web_contents_delegate_android/web_contents_delegate_android.cc:53:
const content::OpenURLParams& params) {
On 2012/11/12 17:24:40, John Abd-El-Malek wrote:
> ditto

Done.

https://codereview.chromium.org/11365199/diff/1/content/components/web_conten...
File
content/components/web_contents_delegate_android/web_contents_delegate_android.h
(right):

https://codereview.chromium.org/11365199/diff/1/content/components/web_conten...
content/components/web_contents_delegate_android/web_contents_delegate_android.h:43:
class WebContentsDelegateAndroid : public content::WebContentsDelegate {
On 2012/11/12 17:24:40, John Abd-El-Malek wrote:
> nit: get rid of content:: everywhere

Done.

[Jói, 2012-11-12 18:41:33]

LGTM

Just a question to be sure:  Did you not need to leave a forwarding header
(similar to
chrome/browser/component/navigation_interception/intercept_navigation_delegate.h)
for now?

Cheers,
Jói

[John Knottenbelt, 2012-11-13 10:03:29]

Thanks Joi, we don't need the forwarding header because downstream depends on 
chrome/browser/android/chrome_web_contents_delegate_android.h (which thankfully
isn't moving). It does, however, depend on the
components.gyp:web_contents_delegate_android_java target, which is why that's
still there (but will be removed soon).

[Jói, 2012-11-13 10:49:25]

Cool, LGTM.

On Tue, Nov 13, 2012 at 10:03 AM,  <jknotten@chromium.org> wrote:
> Thanks Joi, we don't need the forwarding header because downstream depends
> on
> chrome/browser/android/chrome_web_contents_delegate_android.h (which
> thankfully
> isn't moving). It does, however, depend on the
> components.gyp:web_contents_delegate_android_java target, which is why
> that's
> still there (but will be removed soon).
>
> https://codereview.chromium.org/11365199/

[joth, 2012-11-13 19:26:14]

lgtm for android_webview

https://codereview.chromium.org/11365199/diff/7003/build/android/findbugs_fil...
File build/android/findbugs_filter/findbugs_known_bugs.txt (right):

https://codereview.chromium.org/11365199/diff/7003/build/android/findbugs_fil...
build/android/findbugs_filter/findbugs_known_bugs.txt:190: M P UPM: Private
method
org.chromium.content.components.web_contents_delegate_android.WebContentsDelegateAndroid.onLoadProgressChanged(double)
is never called  At WebContentsDelegateAndroid.java
I'm surprised this is the only example of this warning we have... there are many
other @CalledByNative private methods.
*shrug*

https://codereview.chromium.org/11365199/diff/7003/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11365199/diff/7003/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:2618:
'../content/content.gyp:web_contents_delegate_android',
'<(DEPTH)/content...' ?

[John Knottenbelt, 2012-11-14 10:53:28]

https://codereview.chromium.org/11365199/diff/7003/build/android/findbugs_fil...
File build/android/findbugs_filter/findbugs_known_bugs.txt (right):

https://codereview.chromium.org/11365199/diff/7003/build/android/findbugs_fil...
build/android/findbugs_filter/findbugs_known_bugs.txt:190: M P UPM: Private
method
org.chromium.content.components.web_contents_delegate_android.WebContentsDelegateAndroid.onLoadProgressChanged(double)
is never called  At WebContentsDelegateAndroid.java
On 2012/11/13 19:26:14, joth wrote:
> I'm surprised this is the only example of this warning we have... there are
many
> other @CalledByNative private methods.
> *shrug*

Indeed, it is rather mysterious. Will take a look to see if I can see why
findbugs is identifying this particular case and not others.

https://codereview.chromium.org/11365199/diff/7003/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11365199/diff/7003/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:2618:
'../content/content.gyp:web_contents_delegate_android',
Discussed with torne, who tells me <(DEPTH) should only be used when the .gypi
can be included from more than one .gyp in different places, otherwise a
relative path should be preferred. 

On 2012/11/13 19:26:14, joth wrote:
> '<(DEPTH)/content...' ?

[John Knottenbelt, 2012-11-14 12:22:59]

https://codereview.chromium.org/11365199/diff/7003/build/android/findbugs_fil...
File build/android/findbugs_filter/findbugs_known_bugs.txt (right):

https://codereview.chromium.org/11365199/diff/7003/build/android/findbugs_fil...
build/android/findbugs_filter/findbugs_known_bugs.txt:190: M P UPM: Private
method
org.chromium.content.components.web_contents_delegate_android.WebContentsDelegateAndroid.onLoadProgressChanged(double)
is never called  At WebContentsDelegateAndroid.java
I investigated this some more. The warning disappears if I rename the
onLoadProgressChanged(double) to some other method name, e.g.
onLoadProgressChangedInternal(double). This seems pretty crazy to me, and
perhaps related to onLoadProgressInternal having two overloads.

Happy to make such a change to the code (but separately to this CL) if you
recommend it.

On 2012/11/14 10:53:28, John Knottenbelt wrote:
> On 2012/11/13 19:26:14, joth wrote:
> > I'm surprised this is the only example of this warning we have... there are
> many
> > other @CalledByNative private methods.
> > *shrug*
> 
> Indeed, it is rather mysterious. Will take a look to see if I can see why
> findbugs is identifying this particular case and not others.

[commit-bot: I haz the power, 2012-11-14 12:26:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jknotten@chromium.org/11365199/10006

[commit-bot: I haz the power, 2012-11-14 14:05:17]

Change committed as 167670

[joth, 2012-11-14 17:33:36]

On 14 November 2012 04:22, <jknotten@chromium.org> wrote:

>
> https://codereview.chromium.**org/11365199/diff/7003/build/**
>
android/findbugs_filter/**findbugs_known_bugs.txt<https://codereview.chromium.org/11365199/diff/7003/build/android/findbugs_filter/findbugs_known_bugs.txt>
> File build/android/findbugs_filter/**findbugs_known_bugs.txt (right):
>
> https://codereview.chromium.**org/11365199/diff/7003/build/**
>
android/findbugs_filter/**findbugs_known_bugs.txt#**newcode190<https://codereview.chromium.org/11365199/diff/7003/build/android/findbugs_filter/findbugs_known_bugs.txt#newcode190>
> build/android/findbugs_filter/**findbugs_known_bugs.txt:190: M P UPM:
> Private method
> org.chromium.content.**components.web_contents_**delegate_android.**
> WebContentsDelegateAndroid.**onLoadProgressChanged(double)
> is never called  At WebContentsDelegateAndroid.**java
> I investigated this some more. The warning disappears if I rename the
> onLoadProgressChanged(double) to some other method name, e.g.
> onLoadProgressChangedInternal(**double). This seems pretty crazy to me,
> and perhaps related to onLoadProgressInternal having two overloads.
>
> Happy to make such a change to the code (but separately to this CL) if
> you recommend it.
>
>
> Yes, changing the name wouldn't be a bad idea anyway, as it will help make
it clearer what's good for subclasses vs what's internal detail. (we could
move the private method to the end too).
notifyLoadProgressChange or onLPCInternal are Ok with me.

Thanks!





>  On 2012/11/14 10:53:28, John Knottenbelt wrote:
>
>> On 2012/11/13 19:26:14, joth wrote:
>> > I'm surprised this is the only example of this warning we have...
>>
> there are
>
>> many
>> > other @CalledByNative private methods.
>> > *shrug*
>>
>
>  Indeed, it is rather mysterious. Will take a look to see if I can see
>>
> why
>
>> findbugs is identifying this particular case and not others.
>>
>
>
https://codereview.chromium.**org/11365199/<https://codereview.chromium.org/1...
>