Move navigation interception component to content/components

Move navigation interception component to content/components

1. Move C++ source from chrome/browser/component/navigation_interception
   to content/components/navigation_interception.
2. Change the namespace from navigation_interception:: to content::
3. Change Java package from org.chromium.chrome.browser.components.
   navigation_interception to org.chromium.content.components.
   navigation_interception.

I will follow up with another patch to completely remove
chrome/browser/component/navigation_interception once external
dependencies have been updated to use the component at its new
component

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

[John Knottenbelt, 2012-10-31 18:02:31]

First cut at moving the navigation_interception component into it's new home. If
you feel anybody should be added to this review at this stage, please do add
them.

One thing that I'm not sure about in this patch is where the unit test should
go, and also the changes to the various DEPS files to make the checkdeps tool
happy.

Thanks!

[Jói, 2012-10-31 21:18:26]

Mostly LGTM, some nits below.  I think we can defer the unit test build change,
just add a TODO to chrome_tests.gypi, but it needs to be followed up soon.

Cheers,
Jói

https://codereview.chromium.org/11293017/diff/1/android_webview/DEPS
File android_webview/DEPS (right):

https://codereview.chromium.org/11293017/diff/1/android_webview/DEPS#newcode11
android_webview/DEPS:11: "+chrome/browser/component",
You could change this to a ! instead of a + rule to show that it is going away
soon (this will cause a presubmit warning to occur if anybody adds new
dependencies to chrome/browser/component).

https://codereview.chromium.org/11293017/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/11293017/diff/1/chrome/chrome_tests.gypi#newc...
chrome/chrome_tests.gypi:1249:
'../content/components/navigation_interception/intercept_navigation_resource_throttle_unittest.cc',
Add a TODO here to move tests to content/components/content_components.gyp.  I
think we can do this as a follow-up because we need to establish a new unittest
executable first, and add it to bot configurations so it runs on trybots and
such.

I doubt we want tests from content/components to be part of content_unittests,
although maybe I'm wrong.  John (jam@) could perhaps chime in.

https://codereview.chromium.org/11293017/diff/1/content/components/navigation...
File content/components/navigation_interception/DEPS (right):

https://codereview.chromium.org/11293017/diff/1/content/components/navigation...
content/components/navigation_interception/DEPS:4:
"+navigation_interception/jni/InterceptNavigationDelegate_jni.h",
perhaps comment that this is a generated file?

https://codereview.chromium.org/11293017/diff/1/content/components/navigation...
File
content/components/navigation_interception/intercept_navigation_resource_throttle.cc
(right):

https://codereview.chromium.org/11293017/diff/1/content/components/navigation...
content/components/navigation_interception/intercept_navigation_resource_throttle.cc:122:
}  // content
content -> namespace content

[mkosiba (inactive), 2012-10-31 22:41:26]

https://codereview.chromium.org/11293017/diff/1/content/components/navigation...
File content/components/navigation_interception/DEPS (right):

https://codereview.chromium.org/11293017/diff/1/content/components/navigation...
content/components/navigation_interception/DEPS:4:
"+navigation_interception/jni/InterceptNavigationDelegate_jni.h",
I always thought it is a BAD IDEA(tm) to generate code to the source tree.. we
have a system of generating the jni header to the out/ folder in place already,
why do we need to depart from it in this case?

https://codereview.chromium.org/11293017/diff/1/content/components/navigation...
File content/components/navigation_interception/navigation_interception.gypi
(right):

https://codereview.chromium.org/11293017/diff/1/content/components/navigation...
content/components/navigation_interception/navigation_interception.gypi:64:
'jni_gen_dir': '../content/components/navigation_interception',
why not just navigation_interception?

also.. this shouldn't work, if you look at jni_generator.gypi the path where
we'd place the generated files under would be
out/Debug/../content/components/navigation_interception

[John Knottenbelt, 2012-11-02 14:48:25]

http://codereview.chromium.org/11293017/diff/1/android_webview/DEPS
File android_webview/DEPS (right):

http://codereview.chromium.org/11293017/diff/1/android_webview/DEPS#newcode11
android_webview/DEPS:11: "+chrome/browser/component",
On 2012/10/31 21:18:27, Jói wrote:
> You could change this to a ! instead of a + rule to show that it is going away
> soon (this will cause a presubmit warning to occur if anybody adds new
> dependencies to chrome/browser/component).

Done.

http://codereview.chromium.org/11293017/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/11293017/diff/1/chrome/chrome_tests.gypi#newco...
chrome/chrome_tests.gypi:1249:
'../content/components/navigation_interception/intercept_navigation_resource_throttle_unittest.cc',
On 2012/10/31 21:18:27, Jói wrote:
> Add a TODO here to move tests to content/components/content_components.gyp.  I
> think we can do this as a follow-up because we need to establish a new
unittest
> executable first, and add it to bot configurations so it runs on trybots and
> such.
> 
> I doubt we want tests from content/components to be part of content_unittests,
> although maybe I'm wrong.  John (jam@) could perhaps chime in.

Done.

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
File content/components/navigation_interception/DEPS (right):

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
content/components/navigation_interception/DEPS:4:
"+navigation_interception/jni/InterceptNavigationDelegate_jni.h",
On 2012/10/31 22:41:26, Martin Kosiba wrote:
> I always thought it is a BAD IDEA(tm) to generate code to the source tree.. we
> have a system of generating the jni header to the out/ folder in place
already,
> why do we need to depart from it in this case?

The file *is* generated to the out/Debug/gen directory. The gyp file adds that
to the include path. I added this rule because otherwise checkdeps complains
that  no rule exists to allow the include.

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
File
content/components/navigation_interception/intercept_navigation_resource_throttle.cc
(right):

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
content/components/navigation_interception/intercept_navigation_resource_throttle.cc:122:
}  // content
On 2012/10/31 21:18:27, Jói wrote:
> content -> namespace content

Done.

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
File content/components/navigation_interception/navigation_interception.gypi
(right):

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
content/components/navigation_interception/navigation_interception.gypi:64:
'jni_gen_dir': '../content/components/navigation_interception',
On 2012/10/31 22:41:26, Martin Kosiba wrote:
> why not just navigation_interception?

This is a possibility, do you think that prefixing with content/components in
the out/Debug/gen directory is redundant? It seemed to me to keep the generated
files out of the top-level generated files namespace.

> 
> also.. this shouldn't work, if you look at jni_generator.gypi the path where
> we'd place the generated files under would be
> out/Debug/../content/components/navigation_interception

There is an unfortunate feature of gyp that variables ending in _dir are
"path-name relativised", which means that gyp wants to prefix any relative paths
with the relative path between the gypi and the gyp that is including it.

http://code.google.com/p/gyp/wiki/InputFormatReference#Pathname_Relativization

This is the reason for the '../'. The proper fix is to use a differently named
variable as input into the jni_generator.gypi include. I think this is best
fixed in a separate patch (I'll file a bug).

jni_generator.gypi will then join the SHARED_INTERMEDIATES_DIR, which is
out/Debug/gen with jni_gen_dir, so the output will be
out/Debug/gen/navigation_interception/../content/components/navigation_interception
which gyp then rewrites as
out/Debug/gen/content/components/navigation_interception.

[joth, 2012-11-02 15:18:53]

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
File content/components/navigation_interception/DEPS (right):

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
content/components/navigation_interception/DEPS:4:
"+navigation_interception/jni/InterceptNavigationDelegate_jni.h",
On 2012/11/02 14:48:25, John Knottenbelt wrote:
> On 2012/10/31 22:41:26, Martin Kosiba wrote:
> > I always thought it is a BAD IDEA(tm) to generate code to the source tree..
we
> > have a system of generating the jni header to the out/ folder in place
> already,
> > why do we need to depart from it in this case?
> 
> The file *is* generated to the out/Debug/gen directory. The gyp file adds that
> to the include path. I added this rule because otherwise checkdeps complains
> that  no rule exists to allow the include.

gotcha. I think the normal pattern it to just say "+jni" in the DEPS.
(see base/DEPS, content/DEPS etc)

[mkosiba (inactive), 2012-11-02 15:36:27]

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
File content/components/navigation_interception/DEPS (right):

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
content/components/navigation_interception/DEPS:4:
"+navigation_interception/jni/InterceptNavigationDelegate_jni.h",
On 2012/11/02 14:48:25, John Knottenbelt wrote:
> On 2012/10/31 22:41:26, Martin Kosiba wrote:
> > I always thought it is a BAD IDEA(tm) to generate code to the source tree..
we
> > have a system of generating the jni header to the out/ folder in place
> already,
> > why do we need to depart from it in this case?
> 
> The file *is* generated to the out/Debug/gen directory. The gyp file adds that
> to the include path. I added this rule because otherwise checkdeps complains
> that  no rule exists to allow the include.

ah.. I see. Everyone else uses "+jni"

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
File content/components/navigation_interception/navigation_interception.gypi
(right):

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
content/components/navigation_interception/navigation_interception.gypi:20:
'<(SHARED_INTERMEDIATE_DIR)/content/components',
maybe use SHARED_INTERMEDIATES_DIR/content/components/navigation_interception
(or if you like what I proposed below:
SHARED_INTERMEDIATES_DIR/navigation_interception). Then you could simplify the
DEPS rule to be "+jni".

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
content/components/navigation_interception/navigation_interception.gypi:64:
'jni_gen_dir': '../content/components/navigation_interception',
On 2012/11/02 14:48:25, John Knottenbelt wrote:
> On 2012/10/31 22:41:26, Martin Kosiba wrote:
> > why not just navigation_interception?
> 
> This is a possibility, do you think that prefixing with content/components in
> the out/Debug/gen directory is redundant? It seemed to me to keep the
generated
> files out of the top-level generated files namespace.
> 
> > 
> > also.. this shouldn't work, if you look at jni_generator.gypi the path where
> > we'd place the generated files under would be
> > out/Debug/../content/components/navigation_interception
> 
> There is an unfortunate feature of gyp that variables ending in _dir are
> "path-name relativised", which means that gyp wants to prefix any relative
paths
> with the relative path between the gypi and the gyp that is including it.
> 
> http://code.google.com/p/gyp/wiki/InputFormatReference#Pathname_Relativization
> 
> This is the reason for the '../'. The proper fix is to use a differently named
> variable as input into the jni_generator.gypi include. I think this is best
> fixed in a separate patch (I'll file a bug).
> 
> jni_generator.gypi will then join the SHARED_INTERMEDIATES_DIR, which is
> out/Debug/gen with jni_gen_dir, so the output will be
>
out/Debug/gen/navigation_interception/../content/components/navigation_interception
> which gyp then rewrites as
> out/Debug/gen/content/components/navigation_interception.

ah.. I see.. I didn't know about the relativization feature. That being said,
everyone else seems to follow a convention of 'jni_gen_dir' : <target_name>.

I don't think there would be a problem if you simply used
'../navigation_interception' (and added a comment about the .. being there due
to relativization).
I'm assuming the 'common folder' problem you're referring to was due to the fact
that the browser_component_jni_headers spat out all of the generated headers
into one folder, but if you have separate targets those should output to
separate folders.

I guess the main reason I'd get rid of the content/components prefix is that we
don't have a separate namespace for each component.

[John Knottenbelt, 2012-11-02 16:34:44]

Thanks for the help with the DEPS and jni generator! Here it is again.

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
File content/components/navigation_interception/navigation_interception.gypi
(right):

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
content/components/navigation_interception/navigation_interception.gypi:20:
'<(SHARED_INTERMEDIATE_DIR)/content/components',
On 2012/11/02 15:36:27, Martin Kosiba wrote:
> maybe use SHARED_INTERMEDIATES_DIR/content/components/navigation_interception
> (or if you like what I proposed below:
> SHARED_INTERMEDIATES_DIR/navigation_interception). Then you could simplify the
> DEPS rule to be "+jni".

Done.

http://codereview.chromium.org/11293017/diff/1/content/components/navigation_...
content/components/navigation_interception/navigation_interception.gypi:64:
'jni_gen_dir': '../content/components/navigation_interception',
On 2012/11/02 15:36:27, Martin Kosiba wrote:
> On 2012/11/02 14:48:25, John Knottenbelt wrote:
> > On 2012/10/31 22:41:26, Martin Kosiba wrote:
> > > why not just navigation_interception?
> > 
> > This is a possibility, do you think that prefixing with content/components
in
> > the out/Debug/gen directory is redundant? It seemed to me to keep the
> generated
> > files out of the top-level generated files namespace.
> > 
> > > 
> > > also.. this shouldn't work, if you look at jni_generator.gypi the path
where
> > > we'd place the generated files under would be
> > > out/Debug/../content/components/navigation_interception
> > 
> > There is an unfortunate feature of gyp that variables ending in _dir are
> > "path-name relativised", which means that gyp wants to prefix any relative
> paths
> > with the relative path between the gypi and the gyp that is including it.
> > 
> >
http://code.google.com/p/gyp/wiki/InputFormatReference#Pathname_Relativization
> > 
> > This is the reason for the '../'. The proper fix is to use a differently
named
> > variable as input into the jni_generator.gypi include. I think this is best
> > fixed in a separate patch (I'll file a bug).
> > 
> > jni_generator.gypi will then join the SHARED_INTERMEDIATES_DIR, which is
> > out/Debug/gen with jni_gen_dir, so the output will be
> >
>
out/Debug/gen/navigation_interception/../content/components/navigation_interception
> > which gyp then rewrites as
> > out/Debug/gen/content/components/navigation_interception.
> 
> ah.. I see.. I didn't know about the relativization feature. That being said,
> everyone else seems to follow a convention of 'jni_gen_dir' : <target_name>.
> 
> I don't think there would be a problem if you simply used
> '../navigation_interception' (and added a comment about the .. being there due
> to relativization).
> I'm assuming the 'common folder' problem you're referring to was due to the
fact
> that the browser_component_jni_headers spat out all of the generated headers
> into one folder, but if you have separate targets those should output to
> separate folders.
> 
> I guess the main reason I'd get rid of the content/components prefix is that
we
> don't have a separate namespace for each component.

Done.

[mkosiba (inactive), 2012-11-02 16:50:25]

lgtm

[jam, 2012-11-02 16:52:37]

https://codereview.chromium.org/11293017/diff/11001/content/components/naviga...
File content/components/navigation_interception/DEPS (right):

https://codereview.chromium.org/11293017/diff/11001/content/components/naviga...
content/components/navigation_interception/DEPS:4:
"+chrome/test/base/chrome_render_view_host_test_harness.h",
anything below content/ shouldn't depend on chrome/. how about you make this use
content/public/test/test_renderer_host.h instead? you can then run it in
content_unittests

[jam, 2012-11-02 16:57:56]

https://codereview.chromium.org/11293017/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/11293017/diff/1/chrome/chrome_tests.gypi#newc...
chrome/chrome_tests.gypi:1249:
'../content/components/navigation_interception/intercept_navigation_resource_throttle_unittest.cc',
On 2012/10/31 21:18:27, Jói wrote:
> Add a TODO here to move tests to content/components/content_components.gyp.  I
> think we can do this as a follow-up because we need to establish a new
unittest
> executable first, and add it to bot configurations so it runs on trybots and
> such.
> 
> I doubt we want tests from content/components to be part of content_unittests,
> although maybe I'm wrong.  John (jam@) could perhaps chime in.

(just saw this)

I'm fine having a separate step later to move them into a separate target. but i
would really prefer that nothing moves into content/components with chrome
dependencies. this should be easy to remove per my other comment

[mkosiba (inactive), 2012-11-02 17:20:39]

On 2012/11/02 16:57:56, John Abd-El-Malek wrote:
> https://codereview.chromium.org/11293017/diff/1/chrome/chrome_tests.gypi
> File chrome/chrome_tests.gypi (right):
> 
>
https://codereview.chromium.org/11293017/diff/1/chrome/chrome_tests.gypi#newc...
> chrome/chrome_tests.gypi:1249:
>
'../content/components/navigation_interception/intercept_navigation_resource_throttle_unittest.cc',
> On 2012/10/31 21:18:27, Jói wrote:
> > Add a TODO here to move tests to content/components/content_components.gyp. 
I
> > think we can do this as a follow-up because we need to establish a new
> unittest
> > executable first, and add it to bot configurations so it runs on trybots and
> > such.
> > 
> > I doubt we want tests from content/components to be part of
content_unittests,
> > although maybe I'm wrong.  John (jam@) could perhaps chime in.
> 
> (just saw this)
> 
> I'm fine having a separate step later to move them into a separate target. but
i
> would really prefer that nothing moves into content/components with chrome
> dependencies. this should be easy to remove per my other comment

The tests run fine using the test_renderer_host (checked that some time ago).
They do need to be moved into content_unittests for that to work though.

[jam, 2012-11-02 17:23:15]

On 2012/11/02 17:20:39, Martin Kosiba wrote:
> On 2012/11/02 16:57:56, John Abd-El-Malek wrote:
> > https://codereview.chromium.org/11293017/diff/1/chrome/chrome_tests.gypi
> > File chrome/chrome_tests.gypi (right):
> > 
> >
>
https://codereview.chromium.org/11293017/diff/1/chrome/chrome_tests.gypi#newc...
> > chrome/chrome_tests.gypi:1249:
> >
>
'../content/components/navigation_interception/intercept_navigation_resource_throttle_unittest.cc',
> > On 2012/10/31 21:18:27, Jói wrote:
> > > Add a TODO here to move tests to
content/components/content_components.gyp. 
> I
> > > think we can do this as a follow-up because we need to establish a new
> > unittest
> > > executable first, and add it to bot configurations so it runs on trybots
and
> > > such.
> > > 
> > > I doubt we want tests from content/components to be part of
> content_unittests,
> > > although maybe I'm wrong.  John (jam@) could perhaps chime in.
> > 
> > (just saw this)
> > 
> > I'm fine having a separate step later to move them into a separate target.
but
> i
> > would really prefer that nothing moves into content/components with chrome
> > dependencies. this should be easy to remove per my other comment
> 
> The tests run fine using the test_renderer_host (checked that some time ago).
> They do need to be moved into content_unittests for that to work though.

yep, I think that's preferable until you have a separate target for components
(if you guys/Joi think that's better, up to you all).

[John Knottenbelt, 2012-11-02 19:02:47]

Adding owners. Please could you review:

Yaron - chrome/browser/android
Darin - chrome/browser/renderer_hosts, gyp changes in chrome/

Thanks!

[Jói, 2012-11-02 19:29:35]

>> The tests run fine using the test_renderer_host (checked that some time
>> ago).
>> They do need to be moved into content_unittests for that to work though.
>
>
> yep, I think that's preferable until you have a separate target for
> components
> (if you guys/Joi think that's better, up to you all).

I'm fine with that plan.

Cheers,
Jói


On Fri, Nov 2, 2012 at 5:23 PM,  <jam@chromium.org> wrote:
> On 2012/11/02 17:20:39, Martin Kosiba wrote:
>>
>> On 2012/11/02 16:57:56, John Abd-El-Malek wrote:
>> > https://codereview.chromium.org/11293017/diff/1/chrome/chrome_tests.gypi
>> > File chrome/chrome_tests.gypi (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/11293017/diff/1/chrome/chrome_tests.gypi#newc...
>>
>> > chrome/chrome_tests.gypi:1249:
>> >
>
>
>
'../content/components/navigation_interception/intercept_navigation_resource_throttle_unittest.cc',
>>
>> > On 2012/10/31 21:18:27, Jói wrote:
>> > > Add a TODO here to move tests to
>
> content/components/content_components.gyp.
>>
>> I
>> > > think we can do this as a follow-up because we need to establish a new
>> > unittest
>> > > executable first, and add it to bot configurations so it runs on
>> > > trybots
>
> and
>>
>> > > such.
>> > >
>> > > I doubt we want tests from content/components to be part of
>> content_unittests,
>> > > although maybe I'm wrong.  John (jam@) could perhaps chime in.
>> >
>> > (just saw this)
>> >
>> > I'm fine having a separate step later to move them into a separate
>> > target.
>
> but
>>
>> i
>> > would really prefer that nothing moves into content/components with
>> > chrome
>> > dependencies. this should be easy to remove per my other comment
>
>
>> The tests run fine using the test_renderer_host (checked that some time
>> ago).
>> They do need to be moved into content_unittests for that to work though.
>
>
> yep, I think that's preferable until you have a separate target for
> components
> (if you guys/Joi think that's better, up to you all).
>
> https://codereview.chromium.org/11293017/

[jam, 2012-11-02 20:37:54]

thanks, lgtm with one nit

https://codereview.chromium.org/11293017/diff/8006/content/components/navigat...
File content/components/navigation_interception/DEPS (right):

https://codereview.chromium.org/11293017/diff/8006/content/components/navigat...
content/components/navigation_interception/DEPS:13: "!chrome/test/base",
now this and line 4 aren't needed anymore

[jam, 2012-11-02 20:46:22]

On 2012/11/02 19:02:47, John Knottenbelt wrote:
> Adding owners. Please could you review:
> 
> Yaron - chrome/browser/android
> Darin - chrome/browser/renderer_hosts, gyp changes in chrome/

lgtm (to save you time, since I'm an owner there)
> 
> Thanks!

[jam, 2012-11-02 20:46:41]

https://codereview.chromium.org/11293017/diff/8006/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/11293017/diff/8006/chrome/chrome.gyp#newcode1053
chrome/chrome.gyp:1053:
'../content/components/content_components.gyp:navigation_interception_java',
nit: order

[Yaron, 2012-11-03 00:54:56]

lgtm

[John Knottenbelt, 2012-11-05 11:55:06]

https://codereview.chromium.org/11293017/diff/8006/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/11293017/diff/8006/chrome/chrome.gyp#newcode1053
chrome/chrome.gyp:1053:
'../content/components/content_components.gyp:navigation_interception_java',
On 2012/11/02 20:46:42, John Abd-El-Malek wrote:
> nit: order

Done.

https://codereview.chromium.org/11293017/diff/8006/content/components/navigat...
File content/components/navigation_interception/DEPS (right):

https://codereview.chromium.org/11293017/diff/8006/content/components/navigat...
content/components/navigation_interception/DEPS:13: "!chrome/test/base",
On 2012/11/02 20:37:54, John Abd-El-Malek wrote:
> now this and line 4 aren't needed anymore

Done.

[mkosiba (inactive), 2012-11-05 11:56:52]

java class move LGTM

[joth, 2012-11-05 14:16:27]

lgtm

[John Knottenbelt, 2012-11-05 15:45:48]

John, Joi, please take a look.

This solves the circular dependency between content.gyp and
content_components.gyp by removing content_components.gyp.

[Jói, 2012-11-05 16:02:01]

https://codereview.chromium.org/11293017/diff/3009/content/components/navigat...
File content/components/navigation_interception/navigation_interception.gypi
(right):

https://codereview.chromium.org/11293017/diff/3009/content/components/navigat...
content/components/navigation_interception/navigation_interception.gypi:9:
'target_name': 'navigation_interception',
There was a recent discussion on chromium-dev@
(https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-d...)
and I think the consensus was that until we have better tooling support, .gypi
files should be colocated with the .gyp file that includes them.

jam@, do you agree that is the consensus for now?  If so we should move this to
content/ at least until the tools are better, and for now can add per-file
OWNERS for this .gypi to content/OWNERS.

[John Knottenbelt, 2012-11-05 16:15:14]

https://codereview.chromium.org/11293017/diff/3009/content/components/navigat...
File content/components/navigation_interception/navigation_interception.gypi
(right):

https://codereview.chromium.org/11293017/diff/3009/content/components/navigat...
content/components/navigation_interception/navigation_interception.gypi:9:
'target_name': 'navigation_interception',
I've been struggling with the pathname relativization a fair bit, so this sounds
good to me, if it's ok to have another gypi in content/.

Please advise how I should rename this file, I'm thinking something like
content_components_navigation_interception.gypi ?

On 2012/11/05 16:02:01, Jói wrote:
> There was a recent discussion on chromium-dev@
>
(https://groups.google.com/a/chromium.org/forum/?fromgroups=#%21topic/chromium...)
> and I think the consensus was that until we have better tooling support, .gypi
> files should be colocated with the .gyp file that includes them.
> 
> jam@, do you agree that is the consensus for now?  If so we should move this
to
> content/ at least until the tools are better, and for now can add per-file
> OWNERS for this .gypi to content/OWNERS.

[jam, 2012-11-05 20:33:13]

https://codereview.chromium.org/11293017/diff/3009/content/components/navigat...
File content/components/navigation_interception/navigation_interception.gypi
(right):

https://codereview.chromium.org/11293017/diff/3009/content/components/navigat...
content/components/navigation_interception/navigation_interception.gypi:9:
'target_name': 'navigation_interception',
On 2012/11/05 16:02:01, Jói wrote:
> There was a recent discussion on chromium-dev@
>
(https://groups.google.com/a/chromium.org/forum/?fromgroups=#%21topic/chromium...)
> and I think the consensus was that until we have better tooling support, .gypi
> files should be colocated with the .gyp file that includes them.
> 
> jam@, do you agree that is the consensus for now?  If so we should move this
to
> content/ at least until the tools are better, and for now can add per-file
> OWNERS for this .gypi to content/OWNERS.

sgtm

[Jói, 2012-11-05 21:07:39]

> Please advise how I should rename this file, I'm thinking something like
> content_components_navigation_interception.gypi ?

Sounds right to me.

On Mon, Nov 5, 2012 at 4:15 PM,  <jknotten@chromium.org> wrote:
>
>
https://codereview.chromium.org/11293017/diff/3009/content/components/navigat...
> File
> content/components/navigation_interception/navigation_interception.gypi
> (right):
>
>
https://codereview.chromium.org/11293017/diff/3009/content/components/navigat...
> content/components/navigation_interception/navigation_interception.gypi:9:
> 'target_name': 'navigation_interception',
> I've been struggling with the pathname relativization a fair bit, so
> this sounds good to me, if it's ok to have another gypi in content/.
>
> Please advise how I should rename this file, I'm thinking something like
> content_components_navigation_interception.gypi ?
>
>
> On 2012/11/05 16:02:01, Jói wrote:
>>
>> There was a recent discussion on chromium-dev@
>
>
>
(https://groups.google.com/a/chromium.org/forum/?fromgroups=#%21topic/chromium...)
>
>> and I think the consensus was that until we have better tooling
>
> support, .gypi
>>
>> files should be colocated with the .gyp file that includes them.
>
>
>> jam@, do you agree that is the consensus for now?  If so we should
>
> move this to
>>
>> content/ at least until the tools are better, and for now can add
>
> per-file
>>
>> OWNERS for this .gypi to content/OWNERS.
>
>
> https://codereview.chromium.org/11293017/

[John Knottenbelt, 2012-11-06 17:07:06]

This moves the gypi to content/ so that it is side by side with content.gyp.
Adds per-file OWNERS.

[Jói, 2012-11-06 21:53:07]

LGTM

[commit-bot: I haz the power, 2012-11-07 10:23:17]

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

[commit-bot: I haz the power, 2012-11-07 11:28:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[John Knottenbelt, 2012-11-07 15:27:50]

I've added a few bits in chrome/browser/components so that the downstream tree
can continue to build. 

I'll remove these changes entirely once downstream is updated to use the code at
the new location / namespace / package.

[Jói, 2012-11-07 16:21:11]

LGTM

[Torne, 2012-11-07 16:47:04]

Android.mk change LGTM

[commit-bot: I haz the power, 2012-11-07 17:35:43]

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

[commit-bot: I haz the power, 2012-11-07 17:35:55]

Failed to apply patch for content/OWNERS:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/OWNERS
  Hunk #1 FAILED at 4.
  1 out of 1 hunk FAILED -- saving rejects to file content/OWNERS.rej

Patch:       content/OWNERS
Index: content/OWNERS
diff --git a/content/OWNERS b/content/OWNERS
index
9e887c62b0125f14f8d9fbe7626ebc9aa3c22173..960dd1dd69e432a23d01a83cd60db67a443dd5ab
100644
--- a/content/OWNERS
+++ b/content/OWNERS
@@ -4,3 +4,7 @@ creis@chromium.org
 darin@chromium.org
 jam@chromium.org
 piman@chromium.org
+
+per-file content_components_navigation_interception.gypi =
jknotten@chromium.org
+per-file content_components_navigation_interception.gypi = joth@chromium.org
+per-file content_components_navigation_interception.gypi = mkosiba@chromium.org

[commit-bot: I haz the power, 2012-11-08 10:39:17]

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

[commit-bot: I haz the power, 2012-11-08 10:39:38]

Presubmit check for 11293017-12093 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  android_webview/Android.mk, line 32, 90 chars

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
  android_webview/Android.mk

Presubmit checks took 2.5s to calculate.

[commit-bot: I haz the power, 2012-11-08 14:31:09]

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

[commit-bot: I haz the power, 2012-11-08 14:31:27]

Presubmit check for 11293017-5094 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  android_webview/Android.mk, line 32, 90 chars

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
  android_webview/Android.mk

Presubmit checks took 2.4s to calculate.

[commit-bot: I haz the power, 2012-11-08 14:49:04]

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

[commit-bot: I haz the power, 2012-11-08 14:49:31]

Presubmit check for 11293017-5094 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  android_webview/Android.mk, line 32, 90 chars

Presubmit checks took 3.1s to calculate.

[commit-bot: I haz the power, 2012-11-08 15:26:40]

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

[commit-bot: I haz the power, 2012-11-08 18:11:51]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-08 20:28:47]

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

[commit-bot: I haz the power, 2012-11-08 21:05:21]

Change committed as 166758

[commit-bot: I haz the power, 2012-11-09 08:17:00]

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

[commit-bot: I haz the power, 2012-11-09 10:06:18]

Change committed as 166883