Move content/components/web_contents_delegate_android to components/web_contents_delegate_android

Move content/components/web_contents_delegate_android to components/web_contents_delegate_android

Moving files, change namespace from content to components,
added/updated DEPS files, and fix builds

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

[boliu, 2013-01-10 20:20:22]

brettw: chrome/ and content DEPS/gyp changes
yfriedman: android code in chrome/
joth: android_webview/, also you are the owner of the component
joi: components/

Also cc-ing a bunch of people who might be interested.

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
chrome/browser/android/chrome_jni_registrar.cc:28: // Register JNI for
components we depend on.
Please review this. I reordered the registrars are called.

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
components/web_contents_delegate_android/web_contents_delegate_android.cc:11:
#include "content/public/browser/android/content_view_core.h"
removed a whole bunch of unneeded includes and moved some from header

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
File components/web_contents_delegate_android/web_contents_delegate_android.gypi
(right):

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
components/web_contents_delegate_android/web_contents_delegate_android.gypi:58:
'jni_gen_dir': '.',
This is due to the gypi file moved relatively to the source files. I think
fixing this is better than updating include_dirs on line 26.

[boliu, 2013-01-10 20:28:11]

Joi: Is there a bug number tracking this move?

[Jói, 2013-01-10 20:45:15]

No, but it's a good idea to have one since several changes will be
involved.  Created http://crbug.com/169312

On Thu, Jan 10, 2013 at 12:28 PM,  <boliu@chromium.org> wrote:
> Joi: Is there a bug number tracking this move?
>
> https://codereview.chromium.org/11823046/

[Jói, 2013-01-10 21:17:12]

I reviewed every file in the change.

Reminder:  Add the bug I created to the BUG= line.

Cheers,
Jói

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/DEPS#newcode8
chrome/browser/DEPS:8: # TODO(boliu): Remove when content/compoments is moved to
components.
nit: compoments -> components

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
chrome/browser/android/chrome_jni_registrar.cc:29: { "color_chooser_android",
components::RegisterColorChooserAndroid },
This was "ColorChooserAndroid" before the change, not "color_chooser_android".
Intentional?

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
chrome/browser/android/chrome_jni_registrar.cc:31: {
"web_contents_delegate_android",
This did not appear in the list previously.  Intentional?

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
File components/web_contents_delegate_android/color_chooser_android.cc (right):

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
components/web_contents_delegate_android/color_chooser_android.cc:13:
content::ColorChooser* content::ColorChooser::Create(
No need for content:: prefixes in this line and next two.

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
File components/web_contents_delegate_android/web_contents_delegate_android.gypi
(right):

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
components/web_contents_delegate_android/web_contents_delegate_android.gypi:1: #
Copyright (c) 2012 The Chromium Authors. All rights reserved.
I think we want the .gypi files to live directly under src/components/.

Feel free to add per-file OWNERS for the .gypi to src/components/OWNERS.

[kaiwang, 2013-01-10 21:34:09]

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
File components/web_contents_delegate_android/web_contents_delegate_android.gypi
(right):

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
components/web_contents_delegate_android/web_contents_delegate_android.gypi:1: #
Copyright (c) 2012 The Chromium Authors. All rights reserved.
From components/README, it seems suggesting to have foo.gypi under
components/foo.
For navigation_interception I added a gyp instead of gypi.
I don't know which is better. Let's choose a convention and I'll follow.

[boliu, 2013-01-10 21:40:25]

Going to wait for reviews from other reviewers before uploading new patch set.

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
chrome/browser/android/chrome_jni_registrar.cc:29: { "color_chooser_android",
components::RegisterColorChooserAndroid },
On 2013/01/10 21:17:13, Jói wrote:
> This was "ColorChooserAndroid" before the change, not "color_chooser_android".
> Intentional?

Yes, I don't think those names are used except in logging. I followed the style
in the registrar file in aw/

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
chrome/browser/android/chrome_jni_registrar.cc:31: {
"web_contents_delegate_android",
On 2013/01/10 21:17:13, Jói wrote:
> This did not appear in the list previously.  Intentional?

Yes, this is from the first line in RegisterJni that's removed.

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
File components/web_contents_delegate_android/web_contents_delegate_android.gypi
(right):

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
components/web_contents_delegate_android/web_contents_delegate_android.gypi:1: #
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/01/10 21:34:09, kaiwang wrote:
> From components/README, it seems suggesting to have foo.gypi under
> components/foo.
> For navigation_interception I added a gyp instead of gypi.
> I don't know which is better. Let's choose a convention and I'll follow.

Right..so in the visitedlink one (https://codereview.chromium.org/11825011/) and
this Patch Set, I'm following the convection:

src/components/components.gyp # only includes from other gypi's
src/components/foo/foo.gypi # lists all targets of foo

If this is the path we follow, then whoever lands first will create a
components.gyp with one gypi line and those after will need to rebase :)

[Jói, 2013-01-10 21:51:23]

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
File components/web_contents_delegate_android/web_contents_delegate_android.gypi
(right):

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
components/web_contents_delegate_android/web_contents_delegate_android.gypi:1: #
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/01/10 21:40:25, boliu wrote:
> On 2013/01/10 21:34:09, kaiwang wrote:
> > From components/README, it seems suggesting to have foo.gypi under
> > components/foo.
> > For navigation_interception I added a gyp instead of gypi.
> > I don't know which is better. Let's choose a convention and I'll follow.
> 
> Right..so in the visitedlink one (https://codereview.chromium.org/11825011/)
and
> this Patch Set, I'm following the convection:
> 
> src/components/components.gyp # only includes from other gypi's
> src/components/foo/foo.gypi # lists all targets of foo
> 
> If this is the path we follow, then whoever lands first will create a
> components.gyp with one gypi line and those after will need to rebase :)

The convention is to keep .gypi files alongside the .gyp files that reference
them, to avoid confusing relative path situations.  See the .gyp and .gypi files
under src/content/ as an example.

[Yaron, 2013-01-10 21:56:39]

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
chrome/browser/android/chrome_jni_registrar.cc:29: { "color_chooser_android",
components::RegisterColorChooserAndroid },
If color_chooser is part of WCDA, it should be registered as part of WCDA's
registrar. Why is it separate here? It also looks like webview doesn't register
it yet.. is there an API that hasn't been implemented yet?

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
File components/web_contents_delegate_android/web_contents_delegate_android.gypi
(right):

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
components/web_contents_delegate_android/web_contents_delegate_android.gypi:58:
'jni_gen_dir': '.',
On 2013/01/10 20:20:22, boliu wrote:
> This is due to the gypi file moved relatively to the source files. I think
> fixing this is better than updating include_dirs on line 26.

So what does this path end up being?

[boliu, 2013-01-10 22:02:13]

New patch coming..

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
chrome/browser/android/chrome_jni_registrar.cc:29: { "color_chooser_android",
components::RegisterColorChooserAndroid },
On 2013/01/10 21:56:40, Yaron wrote:
> If color_chooser is part of WCDA, it should be registered as part of WCDA's
> registrar. Why is it separate here? It also looks like webview doesn't
register
> it yet.. is there an API that hasn't been implemented yet?

ColorChooser was added and never used, and I agree it should be in its own
component. So...I could remove it here?

CL it was added: https://chromiumcodereview.appspot.com/11316153

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
File components/web_contents_delegate_android/web_contents_delegate_android.gypi
(right):

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
components/web_contents_delegate_android/web_contents_delegate_android.gypi:58:
'jni_gen_dir': '.',
On 2013/01/10 21:56:40, Yaron wrote:
> On 2013/01/10 20:20:22, boliu wrote:
> > This is due to the gypi file moved relatively to the source files. I think
> > fixing this is better than updating include_dirs on line 26.
> 
> So what does this path end up being?

<(SHARED_INTERMEDIATE_DIR)/web_contents_delegate_android

But we have already decided to move this gypi file up one level, so this change
will be reverted. See discussion at top of this file.

[joth, 2013-01-10 22:21:52]

aw/ lgtm with one question

https://codereview.chromium.org/11823046/diff/2002/android_webview/native/DEPS
File android_webview/native/DEPS (right):

https://codereview.chromium.org/11823046/diff/2002/android_webview/native/DEP...
android_webview/native/DEPS:5: "+components",  # For jni registers.
Hmm dunno what granularity is best, but we should either move this to aw/lib to
avoid native/ dipping into random components, or just put "+components+ in the
top level aw/DEPS (i.e. just say all components are fine for all of aw/ to use).

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
chrome/browser/android/chrome_jni_registrar.cc:29: { "color_chooser_android",
components::RegisterColorChooserAndroid },
On 2013/01/10 22:02:13, boliu wrote:
> On 2013/01/10 21:56:40, Yaron wrote:
> > If color_chooser is part of WCDA, it should be registered as part of WCDA's
> > registrar. Why is it separate here? It also looks like webview doesn't
> register
> > it yet.. is there an API that hasn't been implemented yet?
> 
> ColorChooser was added and never used, and I agree it should be in its own
> component. So...I could remove it here?
> 
> CL it was added: https://chromiumcodereview.appspot.com/11316153

yah Hooking it up in aw/ is covered by http://b/7621527


I don't really know the motivation on the first param or if anyone finds them
useful these days, but I think the general convention is to use the ClassName
version. (you got unlucky in the  ones  you copied style from aw/, most of them
are camel case)


.. my brain does beg for us to use a macro to make these table definitions a bit
clearer :)

[boliu, 2013-01-10 22:35:38]

https://codereview.chromium.org/11823046/diff/2002/android_webview/native/DEPS
File android_webview/native/DEPS (right):

https://codereview.chromium.org/11823046/diff/2002/android_webview/native/DEP...
android_webview/native/DEPS:5: "+components",  # For jni registers.
On 2013/01/10 22:21:53, joth wrote:
> Hmm dunno what granularity is best, but we should either move this to aw/lib
to
> avoid native/ dipping into random components, or just put "+components+ in the
> top level aw/DEPS (i.e. just say all components are fine for all of aw/ to
use).
> 

Do you mean moving android_webview_jni_registrar.cc to lib? Sounds reasonable.

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/DEPS#newcode8
chrome/browser/DEPS:8: # TODO(boliu): Remove when content/compoments is moved to
components.
On 2013/01/10 21:17:13, Jói wrote:
> nit: compoments -> components

Done.

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/11823046/diff/2002/chrome/browser/android/chr...
chrome/browser/android/chrome_jni_registrar.cc:29: { "color_chooser_android",
components::RegisterColorChooserAndroid },
On 2013/01/10 22:21:53, joth wrote:
> On 2013/01/10 22:02:13, boliu wrote:
> > On 2013/01/10 21:56:40, Yaron wrote:
> > > If color_chooser is part of WCDA, it should be registered as part of
WCDA's
> > > registrar. Why is it separate here? It also looks like webview doesn't
> > register
> > > it yet.. is there an API that hasn't been implemented yet?
> > 
> > ColorChooser was added and never used, and I agree it should be in its own
> > component. So...I could remove it here?
> > 
> > CL it was added: https://chromiumcodereview.appspot.com/11316153
Ok, I was a bit confused. Native WebContentsDelegatesAndroid creates the
ColorChooser, which creates the java side, so they should be in the same
component. Will move the register function to where it should be.

Joth: Does this mean webview will crash and burn with <input type=color>? I'll
try adding it to aw and see if it still compiles.

> 
> yah Hooking it up in aw/ is covered by http://b/7621527
> 
> 
> I don't really know the motivation on the first param or if anyone finds them
> useful these days, but I think the general convention is to use the ClassName
> version. (you got unlucky in the  ones  you copied style from aw/, most of
them
> are camel case)
> 
> 
> .. my brain does beg for us to use a macro to make these table definitions a
bit
> clearer :)

I can make them all camel case.

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
File components/web_contents_delegate_android/color_chooser_android.cc (right):

https://codereview.chromium.org/11823046/diff/2002/components/web_contents_de...
components/web_contents_delegate_android/color_chooser_android.cc:13:
content::ColorChooser* content::ColorChooser::Create(
On 2013/01/10 21:17:13, Jói wrote:
> No need for content:: prefixes in this line and next two.

Done.

[joth, 2013-01-10 22:46:30]

On 10 January 2013 14:35, <boliu@chromium.org> wrote:

>
> https://codereview.chromium.**org/11823046/diff/2002/**
>
android_webview/native/DEPS#**newcode5<https://codereview.chromium.org/11823046/diff/2002/android_webview/native/DEPS#newcode5>
> android_webview/native/DEPS:5: "+components",  # For jni registers.
> On 2013/01/10 22:21:53, joth wrote:
>
>> Hmm dunno what granularity is best, but we should either move this to
>>
> aw/lib to
>
>> avoid native/ dipping into random components, or just put
>>
> "+components+ in the
>
>> top level aw/DEPS (i.e. just say all components are fine for all of
>>
> aw/ to use).
>
>
> Do you mean moving android_webview_jni_registrar.**cc to lib? Sounds
> reasonable.
>

Actually I was thinking more leave this method where it is (but ultimately
*just* registering aw/native classes) and register all other required
components in android_webview/lib/main/webview_entry_point.cc JNI_OnLoad
(we already register some other content/ stuff there anyway)

[boliu, 2013-01-10 23:48:45]

PTAL

Moved gypi file up. Moved ColorChooser jni register. Moved component register in
aw to lib/. Address other comments.

[Jói, 2013-01-11 00:27:18]

LGTM

[joth, 2013-01-11 01:09:22]

lgtm

https://codereview.chromium.org/11823046/diff/14001/android_webview/lib/DEPS
File android_webview/lib/DEPS (right):

https://codereview.chromium.org/11823046/diff/14001/android_webview/lib/DEPS#...
android_webview/lib/DEPS:3: "+components",  # For jni registers.
nit: "registrars".

[Yaron, 2013-01-11 03:15:15]

lgtm

[miguelg, 2013-01-11 15:38:47]

lgtm for the color chooser changes

On Fri, Jan 11, 2013 at 3:15 AM,  <yfriedman@chromium.org> wrote:
> lgtm
>
>
>
> https://codereview.chromium.org/11823046/

[boliu, 2013-01-11 16:54:27]

Nico: Could you stamp non-android chrome/ changes? They are DEPS/gyp changes
only.

[Nico, 2013-01-11 17:19:27]

lgtm

[commit-bot: I haz the power, 2013-01-11 17:20:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/11823046/26004

[commit-bot: I haz the power, 2013-01-11 17:20:46]

Presubmit check for 11823046-26004 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content

Presubmit checks took 3.3s to calculate.

[boliu, 2013-01-11 17:23:46]

Avi: Could you stamp non-components content/ changes? They are gyp changes only.

[Avi (use Gerrit), 2013-01-11 20:54:27]

stampity stamp lgtm

[commit-bot: I haz the power, 2013-01-11 20:55:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/11823046/26004

[commit-bot: I haz the power, 2013-01-11 22:12:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/11823046/47001

[commit-bot: I haz the power, 2013-01-12 00:44:13]

Retried try job too often on mac_rel for step(s) browser_tests

[commit-bot: I haz the power, 2013-01-12 00:53:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/11823046/47001

[commit-bot: I haz the power, 2013-01-12 16:15:27]

Retried try job too often on win7_aura for step(s) browser_tests

[commit-bot: I haz the power, 2013-01-12 17:30:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/11823046/40005

[commit-bot: I haz the power, 2013-01-12 23:04:33]

Change committed as 176598