Split //ios/chrome/browser in multiple targets.

Split //ios/chrome/browser in multiple targets.

Create a new target for each sub-directory of src/ios/chrome/browser.
Keep //ios/chrome/browser as a temporary target dependending on all
of them to avoid breaking the roll.

Remove some obsolete test files never used.

BUG=658242
Committed: https://crrev.com/87704da23bea0783ef0184a566398d5072154c41
Cr-Commit-Position: refs/heads/master@{#427721}

[sdefresne, 2016-10-24 20:25:18]

sdefresne@chromium.org changed reviewers:
+ brettw@chromium.org, dpranke@chromium.org, rohitrao@chromium.org

[sdefresne, 2016-10-24 20:25:19]

Rohit: can you take a look?

brettw/dpranke: do you think there is value in doing that kind of changes? My
feeling is that even though it is painful it has some value as we end up with
unit_tests targets for the test in the same directories as the source and we
have a better view of the dependencies, but I'm not sure how valuable those are
versus the difficulty of maintaining them. WDYT?

[brettw, 2016-10-24 21:40:39]

I think these kinds of things are slightly useful.

[sdefresne, 2016-10-25 16:50:59]

The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 16:51:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sdefresne, 2016-10-25 16:51:35]

sdefresne@chromium.org changed reviewers:
- brettw@chromium.org, dpranke@chromium.org

[sdefresne, 2016-10-25 16:51:35]

Rohit: this is now ready for review. PTAL and CQ if LGTY.

[commit-bot: I haz the power, 2016-10-25 18:10:20]

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

[commit-bot: I haz the power, 2016-10-25 18:10:21]

Dry run: This issue passed the CQ dry run.

[rohitrao (ping after 24h), 2016-10-25 19:48:53]

LGTM

This will need a rebase, since I added some new testdata files.

I continue to be worried that this will make development harder, but I can't
decide if it will be a noticeable difference.

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/goog...
File ios/chrome/browser/google/BUILD.gn (right):

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/goog...
ios/chrome/browser/google/BUILD.gn:24: source_set("google_logo") {
I'd name this logo_service.

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/BUILD.gn (right):

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/BUILD.gn:81:
source_set("passwords_generation_utils") {
Why was this split out?

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/pref...
File ios/chrome/browser/prefs/browser_prefs.mm (right):

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/pref...
ios/chrome/browser/prefs/browser_prefs.mm:42: #include
"ios/chrome/browser/pref_names.h"
This include looks like it's duplicated?

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/rlz/...
File ios/chrome/browser/rlz/BUILD.gn (right):

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/rlz/...
ios/chrome/browser/rlz/BUILD.gn:5: source_set("rlz") {
Should this whole target be behind enable_rlz?

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/sign...
File ios/chrome/browser/signin/BUILD.gn (right):

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/sign...
ios/chrome/browser/signin/BUILD.gn:53:
"//ios/chrome/browser:browser_no_public_deps",
Why didn't we need any of these deps before?  Were we getting them transitively?
 How?

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/webp...
File ios/chrome/browser/webp_transcode/BUILD.gn (right):

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/webp...
ios/chrome/browser/webp_transcode/BUILD.gn:35:
"//ios/chrome/test/data/webp_transcode/test.jpg",
This part feels a little weird.  Should this rule be in
ios/chrome/test/data/BUILD.gn instead?  I think we're better off having the
testdata rule live next to the unit_test rule, but having to use absolute paths
for the files seems unfortunate.

[rohitrao (ping after 24h), 2016-10-25 19:50:18]

rohitrao@chromium.org changed reviewers:
+ marq@chromium.org, noyau@chromium.org, pkl@chromium.org

[rohitrao (ping after 24h), 2016-10-25 19:50:19]

Adding others FYI so they can see what the new BUILD files will look like.

[sdefresne, 2016-10-25 21:10:23]

Thank you for the review.

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/goog...
File ios/chrome/browser/google/BUILD.gn (right):

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/goog...
ios/chrome/browser/google/BUILD.gn:24: source_set("google_logo") {
On 2016/10/25 19:48:53, rohitrao wrote:
> I'd name this logo_service.

Done.

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/BUILD.gn (right):

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/BUILD.gn:81:
source_set("passwords_generation_utils") {
On 2016/10/25 19:48:53, rohitrao wrote:
> Why was this split out?

To break a cycle with //ios/chrome/browser/autofill that depends on
//ios/chrome/browser/passwords just for password_generation_utils.h. I can
change this to an allow_circular_includes_from if you prefer.

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/pref...
File ios/chrome/browser/prefs/browser_prefs.mm (right):

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/pref...
ios/chrome/browser/prefs/browser_prefs.mm:42: #include
"ios/chrome/browser/pref_names.h"
On 2016/10/25 19:48:53, rohitrao wrote:
> This include looks like it's duplicated?

Done.

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/rlz/...
File ios/chrome/browser/rlz/BUILD.gn (right):

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/rlz/...
ios/chrome/browser/rlz/BUILD.gn:5: source_set("rlz") {
On 2016/10/25 19:48:53, rohitrao wrote:
> Should this whole target be behind enable_rlz?

No, in general we avoid putting targets behind if(). This allow for example to
write unit tests for it, even if the support is disabled in the app. And if no
target depend on it, the file will not be loaded and the target won't be
defined.

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/sign...
File ios/chrome/browser/signin/BUILD.gn (right):

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/sign...
ios/chrome/browser/signin/BUILD.gn:53:
"//ios/chrome/browser:browser_no_public_deps",
On 2016/10/25 19:48:53, rohitrao wrote:
> Why didn't we need any of these deps before?  Were we getting them
transitively?
>  How?

Yes, they were part of //ios/chrome/browser and //ios/chrome/browser/signin was
in its allow_circular_includes_from list.

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/webp...
File ios/chrome/browser/webp_transcode/BUILD.gn (right):

https://codereview.chromium.org/2443373002/diff/40001/ios/chrome/browser/webp...
ios/chrome/browser/webp_transcode/BUILD.gn:35:
"//ios/chrome/test/data/webp_transcode/test.jpg",
On 2016/10/25 19:48:53, rohitrao wrote:
> This part feels a little weird.  Should this rule be in
> ios/chrome/test/data/BUILD.gn instead?  I think we're better off having the
> testdata rule live next to the unit_test rule, but having to use absolute
paths
> for the files seems unfortunate.

I have the same feeling, but I prefer having the bundle_data in the same
location as the code depending on it. Maybe we can move the files themselves to
ios/chrome/browser/webp_transcode/test_data in a followup CL?

[sdefresne, 2016-10-25 21:15:25]

The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 21:16:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sdefresne, 2016-10-25 21:44:27]

The CQ bit was unchecked by sdefresne@chromium.org

[sdefresne, 2016-10-26 16:01:00]

The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run

[sdefresne, 2016-10-26 16:01:02]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2016-10-26 16:01:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2443373002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-26 16:01:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 17:12:51]

Description was changed from

==========
Split //ios/chrome/browser in multiple targets.

Create a new target for each sub-directory of src/ios/chrome/browser.
Keep //ios/chrome/browser as a temporary target dependending on all
of them to avoid breaking the roll.

Remove some obsolete test files never used.

BUG=658242
==========

to

==========
Split //ios/chrome/browser in multiple targets.

Create a new target for each sub-directory of src/ios/chrome/browser.
Keep //ios/chrome/browser as a temporary target dependending on all
of them to avoid breaking the roll.

Remove some obsolete test files never used.

BUG=658242

Committed: https://crrev.com/87704da23bea0783ef0184a566398d5072154c41
Cr-Commit-Position: refs/heads/master@{#427721}
==========

[commit-bot: I haz the power, 2016-10-26 17:12:53]

Patchset 5 (id:??) landed as
https://crrev.com/87704da23bea0783ef0184a566398d5072154c41
Cr-Commit-Position: refs/heads/master@{#427721}

[Dirk Pranke, 2016-10-26 17:51:40]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2016-10-26 17:51:41]

lgtm as well.