Move content/components/navigation_interception to src/components

Move content/components/navigation_interception to src/components


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

[kaiwang, 2013-01-12 01:26:00]

Reviewers, please review:

joi: components
joth: android_webview
jam: content and chrome

[kaiwang, 2013-01-12 01:31:55]

Question for jam@:

See
https://codereview.chromium.org/11830043/diff/20015/components/navigation_int...

I have to add content.gyp:content_gpu and content.gyp:content_renderer because
content_browser depends on them, although navigation_interception does not
directly depend on them...

I thought the better way is to add gpu and renderer dependencies to
content_browser. But the content gyp seems complicated distinguishing static and
non-static cases.

What's the best way do you think to remove gpu and renderer dependencies here?
Thanks

[boliu, 2013-01-12 01:35:59]

I only looked at aw/ code. My patch will land soon so you'll have some rebasing
to do.

https://codereview.chromium.org/11830043/diff/20015/android_webview/DEPS
File android_webview/DEPS (right):

https://codereview.chromium.org/11830043/diff/20015/android_webview/DEPS#newc...
android_webview/DEPS:12: "+components",
I decided to be more selective in my patch, adding specific components, in sub
directories as needed
(Do what joth says if we don't agree)

https://codereview.chromium.org/11830043/diff/20015/android_webview/native/an...
File android_webview/native/android_webview_jni_registrar.cc (right):

https://codereview.chromium.org/11830043/diff/20015/android_webview/native/an...
android_webview/native/android_webview_jni_registrar.cc:26: {
"navigation_interception", content::RegisterNavigationInterceptionJni },
Are you planning on fixing namespace in a follow up?

Also this moved to android_webview/lib/main/webview_entry_point.cc in my patch

[Jói, 2013-01-14 17:56:41]

Looked at //components only.

https://codereview.chromium.org/11830043/diff/20015/components/DEPS
File components/DEPS (left):

https://codereview.chromium.org/11830043/diff/20015/components/DEPS#oldcode15
components/DEPS:15: "+content/public/common",
I think this should stay, since it's allowed in general for any directory of any
component.  I don't mind removing content/public/test.

https://codereview.chromium.org/11830043/diff/20015/components/navigation_int...
File components/navigation_interception/component_jni_registrar.h (right):

https://codereview.chromium.org/11830043/diff/20015/components/navigation_int...
components/navigation_interception/component_jni_registrar.h:10: namespace
content {
namespace content -> namespace components, in all files that currently have
namespace content.

https://codereview.chromium.org/11830043/diff/20015/components/tracing/DEPS
File components/tracing/DEPS (left):

https://codereview.chromium.org/11830043/diff/20015/components/tracing/DEPS#o...
components/tracing/DEPS:7: "-content",
I would keep this, since it's an exception to the general rule.

[kaiwang, 2013-01-15 00:39:15]

https://codereview.chromium.org/11830043/diff/20015/android_webview/DEPS
File android_webview/DEPS (right):

https://codereview.chromium.org/11830043/diff/20015/android_webview/DEPS#newc...
android_webview/DEPS:12: "+components",
On 2013/01/12 01:35:59, boliu wrote:
> I decided to be more selective in my patch, adding specific components, in sub
> directories as needed
> (Do what joth says if we don't agree)

Done.

https://codereview.chromium.org/11830043/diff/20015/android_webview/native/an...
File android_webview/native/android_webview_jni_registrar.cc (right):

https://codereview.chromium.org/11830043/diff/20015/android_webview/native/an...
android_webview/native/android_webview_jni_registrar.cc:26: {
"navigation_interception", content::RegisterNavigationInterceptionJni },
Sure, will do

https://codereview.chromium.org/11830043/diff/20015/components/DEPS
File components/DEPS (left):

https://codereview.chromium.org/11830043/diff/20015/components/DEPS#oldcode15
components/DEPS:15: "+content/public/common",
On 2013/01/14 17:56:41, Jói wrote:
> I think this should stay, since it's allowed in general for any directory of
any
> component.  I don't mind removing content/public/test.

Done.

https://codereview.chromium.org/11830043/diff/20015/components/navigation_int...
File components/navigation_interception/component_jni_registrar.h (right):

https://codereview.chromium.org/11830043/diff/20015/components/navigation_int...
components/navigation_interception/component_jni_registrar.h:10: namespace
content {
On 2013/01/14 17:56:41, Jói wrote:
> namespace content -> namespace components, in all files that currently have
> namespace content.

Done.

https://codereview.chromium.org/11830043/diff/20015/components/tracing/DEPS
File components/tracing/DEPS (left):

https://codereview.chromium.org/11830043/diff/20015/components/tracing/DEPS#o...
components/tracing/DEPS:7: "-content",
On 2013/01/14 17:56:41, Jói wrote:
> I would keep this, since it's an exception to the general rule.

Done.

[jam, 2013-01-15 21:13:48]

On 2013/01/12 01:31:55, kaiwang wrote:
> Question for jam@:
> 
> See
>
https://codereview.chromium.org/11830043/diff/20015/components/navigation_int...
> 
> I have to add content.gyp:content_gpu and content.gyp:content_renderer because
> content_browser depends on them, although navigation_interception does not
> directly depend on them...
> 
> I thought the better way is to add gpu and renderer dependencies to
> content_browser. But the content gyp seems complicated distinguishing static
and
> non-static cases.
> 
> What's the best way do you think to remove gpu and renderer dependencies here?
> Thanks

check with dpranke, he made this work for component build and understands this
more than me

[kaiwang, 2013-01-15 22:22:09]

On 2013/01/15 21:13:48, John Abd-El-Malek wrote:
> On 2013/01/12 01:31:55, kaiwang wrote:
> > Question for jam@:
> > 
> > See
> >
>
https://codereview.chromium.org/11830043/diff/20015/components/navigation_int...
> > 
> > I have to add content.gyp:content_gpu and content.gyp:content_renderer
because
> > content_browser depends on them, although navigation_interception does not
> > directly depend on them...
> > 
> > I thought the better way is to add gpu and renderer dependencies to
> > content_browser. But the content gyp seems complicated distinguishing static
> and
> > non-static cases.
> > 
> > What's the best way do you think to remove gpu and renderer dependencies
here?
> > Thanks
> 
> check with dpranke, he made this work for component build and understands this
> more than me

Thanks!

Reviewers, please take another look. Now this change passes on all platforms

[boliu, 2013-01-15 22:40:46]

+mkosiba for fyi

aw/ looks good but give me some time to check this with our two downstream
android builds.

https://codereview.chromium.org/11830043/diff/20110/android_webview/lib/main/...
File android_webview/lib/main/webview_entry_point.cc (right):

https://codereview.chromium.org/11830043/diff/20110/android_webview/lib/main/...
android_webview/lib/main/webview_entry_point.cc:15:
kWebViewDependencyRegisteredMethods[] = {
I've got no idea how indenting should work here...

I was following the convention of method declarations with super long return
types and which does not indent the name of the function.

[boliu, 2013-01-16 17:47:13]

On 2013/01/15 22:40:46, boliu wrote:
> +mkosiba for fyi
> 
> aw/ looks good but give me some time to check this with our two downstream
> android builds.
> 

aw/ lgtm (I'm not an owner)

Downstream android builds all good.

[kaiwang, 2013-01-16 19:02:10]

Thanks!

Ping joi, joth and jam for OWNERs' review

[Jói, 2013-01-16 19:11:48]

LGTM, looked at the whole change.

[jam, 2013-01-16 19:13:51]

On 2013/01/16 19:11:48, Jói wrote:
> LGTM, looked at the whole change.

lgtm

[joth, 2013-01-16 22:38:38]

lgtm


On 16 January 2013 11:13, <jam@chromium.org> wrote:

> On 2013/01/16 19:11:48, Jói wrote:
>
>> LGTM, looked at the whole change.
>>
>
> lgtm
>
>
https://codereview.chromium.**org/11830043/<https://codereview.chromium.org/1...
>

[boliu, 2013-01-16 23:16:06]

Just noticed that it's missing the OWNERS file in
components/navigation_interception directory

[kaiwang, 2013-01-17 00:44:55]

will add

On Wednesday, January 16, 2013, wrote:

> Just noticed that it's missing the OWNERS file in
> components/navigation_**interception directory
>
>
https://codereview.chromium.**org/11830043/<https://codereview.chromium.org/1...
>


-- 
Best Regards!

Kai Wang

[commit-bot: I haz the power, 2013-01-22 02:14:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/11830043/63001

[commit-bot: I haz the power, 2013-01-22 02:54:08]

Retried try job too often on linux_chromeos for step(s) components_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-01-22 18:04:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/11830043/63001

[commit-bot: I haz the power, 2013-01-22 18:58:12]

Retried try job too often on linux_chromeos for step(s) components_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-01-22 20:24:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/11830043/65003

[commit-bot: I haz the power, 2013-01-23 01:08:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/11830043/73003

[commit-bot: I haz the power, 2013-01-23 03:55:18]

Change committed as 178238