Remove dependency of common on variations browser code

Remove dependency of common on variations browser code


R=asvitkine@chromium.org
BUG=404809, 405295
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291293

[scottmg, 2014-08-21 20:19:31]

Need to do GN still, but is this what you had in mind?

[scottmg, 2014-08-21 20:32:06]

On 2014/08/21 20:19:31, scottmg wrote:
> Need to do GN still, but is this what you had in mind?

Hm, looks like the GN build doesn't include variations_http_header_provider at
all. So I guess I won't worry about that for now.

[Alexei Svitkine (slow), 2014-08-21 21:07:10]

Close to what I meant, see comments below. Thanks!

https://codereview.chromium.org/490733002/diff/60001/components/variations.gypi
File components/variations.gypi (right):

https://codereview.chromium.org/490733002/diff/60001/components/variations.gy...
components/variations.gypi:9: 'target_name': 'variations_common',
I'd keep this as 'variations'.

https://codereview.chromium.org/490733002/diff/60001/components/variations.gy...
components/variations.gypi:15: '../base/base.gyp:base',
Would you mind adding the following comment at the top of the file? (To avoid
this library accidentally getting more dependencies as it happened in the case
with the header provider.)

# List of dependencies is intentionally very minimal. Please avoid adding extra
# dependencies without first checking with OWNERS.

https://codereview.chromium.org/490733002/diff/60001/components/variations.gy...
components/variations.gypi:63: 'target_name': 'variations',
I'd name this variations_http_provider, since that's the only thing it has at
this point. (I can revisit the name if/when more files are added to this
target).

Then, only chrome/browser that needs it can depend on this (in addition to the
variations target, since that's needed because chrome/browser still uses its
protos.)

https://codereview.chromium.org/490733002/diff/60001/components/variations.gy...
components/variations.gypi:70:
'../third_party/mt19937ar/mt19937ar.gyp:mt19937ar',
I don't think this dependency is needed.

[scottmg, 2014-08-21 21:21:48]

Thanks! Better-ish now hopefully.

https://codereview.chromium.org/490733002/diff/60001/components/variations.gypi
File components/variations.gypi (right):

https://codereview.chromium.org/490733002/diff/60001/components/variations.gy...
components/variations.gypi:9: 'target_name': 'variations_common',
On 2014/08/21 21:07:10, Alexei Svitkine wrote:
> I'd keep this as 'variations'.

Done.

(Was based on many other components (see chrome_common) having a similar naming
scheme.)

https://codereview.chromium.org/490733002/diff/60001/components/variations.gy...
components/variations.gypi:15: '../base/base.gyp:base',
On 2014/08/21 21:07:10, Alexei Svitkine wrote:
> Would you mind adding the following comment at the top of the file? (To avoid
> this library accidentally getting more dependencies as it happened in the case
> with the header provider.)
> 
> # List of dependencies is intentionally very minimal. Please avoid adding
extra
> # dependencies without first checking with OWNERS.

Done.

https://codereview.chromium.org/490733002/diff/60001/components/variations.gy...
components/variations.gypi:63: 'target_name': 'variations',
On 2014/08/21 21:07:10, Alexei Svitkine wrote:
> I'd name this variations_http_provider, since that's the only thing it has at
> this point. (I can revisit the name if/when more files are added to this
> target).
> 
> Then, only chrome/browser that needs it can depend on this (in addition to the
> variations target, since that's needed because chrome/browser still uses its
> protos.)

Done.

https://codereview.chromium.org/490733002/diff/60001/components/variations.gy...
components/variations.gypi:70:
'../third_party/mt19937ar/mt19937ar.gyp:mt19937ar',
On 2014/08/21 21:07:10, Alexei Svitkine wrote:
> I don't think this dependency is needed.

Done.

[Alexei Svitkine (slow), 2014-08-21 21:36:34]

LGTM, though you might also need to add a dependency on this target to
suggestions component. +mathp, who can tell you more.

https://codereview.chromium.org/490733002/diff/120001/components/variations.gypi
File components/variations.gypi (right):

https://codereview.chromium.org/490733002/diff/120001/components/variations.g...
components/variations.gypi:6: # dependencies without first checking with OWNERS.
Nit: I was thinking this can be within the dependencies block itself, so it's
more likely someone trying to change that will see the comment.

[scottmg, 2014-08-21 21:40:39]

https://codereview.chromium.org/490733002/diff/120001/components/variations.gypi
File components/variations.gypi (right):

https://codereview.chromium.org/490733002/diff/120001/components/variations.g...
components/variations.gypi:6: # dependencies without first checking with OWNERS.
On 2014/08/21 21:36:34, Alexei Svitkine wrote:
> Nit: I was thinking this can be within the dependencies block itself, so it's
> more likely someone trying to change that will see the comment.

Done.

[scottmg, 2014-08-21 21:43:09]

On 2014/08/21 21:36:34, Alexei Svitkine wrote:
> LGTM, though you might also need to add a dependency on this target to
> suggestions component. +mathp, who can tell you more.

OK, suggestions is only used by components_tests and browser, so it will
probably won't strictly be required to link.

Let me know if it makes more sense to have suggestions also depend on
variations_http_provider and I'll add it here.

Thanks

> 
>
https://codereview.chromium.org/490733002/diff/120001/components/variations.gypi
> File components/variations.gypi (right):
> 
>
https://codereview.chromium.org/490733002/diff/120001/components/variations.g...
> components/variations.gypi:6: # dependencies without first checking with
OWNERS.
> Nit: I was thinking this can be within the dependencies block itself, so it's
> more likely someone trying to change that will see the comment.

[scottmg, 2014-08-21 21:46:32]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-08-21 21:49:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scottmg@chromium.org/490733002/140001

[commit-bot: I haz the power, 2014-08-21 23:15:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[commit-bot: I haz the power, 2014-08-21 23:21:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-21 23:21:21]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[scottmg, 2014-08-21 23:24:05]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-08-21 23:29:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scottmg@chromium.org/490733002/140001

[commit-bot: I haz the power, 2014-08-21 23:39:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[commit-bot: I haz the power, 2014-08-22 01:18:13]

Committed patchset #6 (140001) as 291293

[blundell, 2014-08-25 08:21:53]

https://codereview.chromium.org/490733002/diff/140001/components/variations.gypi
File components/variations.gypi (right):

https://codereview.chromium.org/490733002/diff/140001/components/variations.g...
components/variations.gypi:65: 'target_name': 'variations_http_provider',
This is code that should be depended on only from the browser, whereas the
variations target above can be depended on from browser and renderer code, is
that correct? If so, the best way to indicate that would be to move the
variations component into a structure where it had common/ and browser/
subdirectories and do the following target renames (and corresponding file
moves):
variations -> variations_common
variations_http_provider -> variations_browser