Move ImageFetcher to its own target.

Move ImageFetcher to its own target.

This CL moves the ImageFetcher class to its own target to prevent circular
dependencies.

BUG=667747

[gambard, 2016-11-22 13:56:50]

gambard@chromium.org changed reviewers:
+ sdefresne@chromium.org

[gambard, 2016-11-22 13:56:50]

PTAL.

[sdefresne, 2016-11-22 14:03:52]

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

https://codereview.chromium.org/2521253002/diff/40001/ios/chrome/browser/net/...
ios/chrome/browser/net/BUILD.gn:17: "image_fetcher.h",
You also want to move image_fetcher.h to the "image_fetcher" target (see comment
below first).

https://codereview.chromium.org/2521253002/diff/40001/ios/chrome/browser/net/...
ios/chrome/browser/net/BUILD.gn:45: "//ios/chrome/browser/net/image_fetcher",
You don't have to move the files, just create a target named "image_fetcher" in
the current BUILD.gn file, and the dependency will be on
"//ios/chrome/browser/net:image_fetcher".

https://codereview.chromium.org/2521253002/diff/40001/ios/chrome/browser/net/...
ios/chrome/browser/net/BUILD.gn:59: source_set("test_support") {
I don't think we need to change "test_support" as it does not depends on
anything currently, so there is not much risk of having circular dependency.

https://codereview.chromium.org/2521253002/diff/40001/ios/chrome/browser/net/...
ios/chrome/browser/net/BUILD.gn:73: "metrics_network_client_unittest.mm",
We do not need to change "unit_tests" target, as only the test app depends on
it, so no risk of circular dependencies.

[sdefresne, 2016-11-22 14:08:16]

On 2016/11/22 14:03:52, sdefresne wrote:
>
https://codereview.chromium.org/2521253002/diff/40001/ios/chrome/browser/net/...
> File ios/chrome/browser/net/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2521253002/diff/40001/ios/chrome/browser/net/...
> ios/chrome/browser/net/BUILD.gn:17: "image_fetcher.h",
> You also want to move image_fetcher.h to the "image_fetcher" target (see
comment
> below first).
> 
>
https://codereview.chromium.org/2521253002/diff/40001/ios/chrome/browser/net/...
> ios/chrome/browser/net/BUILD.gn:45: "//ios/chrome/browser/net/image_fetcher",
> You don't have to move the files, just create a target named "image_fetcher"
in
> the current BUILD.gn file, and the dependency will be on
> "//ios/chrome/browser/net:image_fetcher".
> 
>
https://codereview.chromium.org/2521253002/diff/40001/ios/chrome/browser/net/...
> ios/chrome/browser/net/BUILD.gn:59: source_set("test_support") {
> I don't think we need to change "test_support" as it does not depends on
> anything currently, so there is not much risk of having circular dependency.
> 
>
https://codereview.chromium.org/2521253002/diff/40001/ios/chrome/browser/net/...
> ios/chrome/browser/net/BUILD.gn:73: "metrics_network_client_unittest.mm",
> We do not need to change "unit_tests" target, as only the test app depends on
> it, so no risk of circular dependencies.

BTW, my comment was based on the description that this change was written to
break a circular dependency.

If the goal is to move the file to a separate target for better organisation of
the code (i.e. we expect to have other sub-directories for
//ios/chrome/browser/net), then lgtm.

[gambard, 2016-11-22 14:25:12]

Thanks, I have split the target, PTAL.

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

https://codereview.chromium.org/2521253002/diff/40001/ios/chrome/browser/net/...
ios/chrome/browser/net/BUILD.gn:17: "image_fetcher.h",
On 2016/11/22 14:03:52, sdefresne wrote:
> You also want to move image_fetcher.h to the "image_fetcher" target (see
comment
> below first).

Done.

https://codereview.chromium.org/2521253002/diff/40001/ios/chrome/browser/net/...
ios/chrome/browser/net/BUILD.gn:45: "//ios/chrome/browser/net/image_fetcher",
On 2016/11/22 14:03:52, sdefresne wrote:
> You don't have to move the files, just create a target named "image_fetcher"
in
> the current BUILD.gn file, and the dependency will be on
> "//ios/chrome/browser/net:image_fetcher".

Done.

https://codereview.chromium.org/2521253002/diff/40001/ios/chrome/browser/net/...
ios/chrome/browser/net/BUILD.gn:59: source_set("test_support") {
On 2016/11/22 14:03:52, sdefresne wrote:
> I don't think we need to change "test_support" as it does not depends on
> anything currently, so there is not much risk of having circular dependency.

Done.

https://codereview.chromium.org/2521253002/diff/40001/ios/chrome/browser/net/...
ios/chrome/browser/net/BUILD.gn:73: "metrics_network_client_unittest.mm",
On 2016/11/22 14:03:52, sdefresne wrote:
> We do not need to change "unit_tests" target, as only the test app depends on
> it, so no risk of circular dependencies.

Done.

[sdefresne, 2016-11-22 14:38:19]

https://codereview.chromium.org/2521253002/diff/60001/ios/chrome/browser/net/...
File ios/chrome/browser/net/BUILD.gn (right):

https://codereview.chromium.org/2521253002/diff/60001/ios/chrome/browser/net/...
ios/chrome/browser/net/BUILD.gn:51: ":image_fetcher",
Can you add a TODO to remove this public_deps (or move it to deps) once the
downstream code has been changed to not require it?

[sdefresne, 2016-11-22 14:38:26]

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

[commit-bot: I haz the power, 2016-11-22 14:38:47]

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

[gambard, 2016-11-22 14:44:13]

Thanks, PTAL.

https://codereview.chromium.org/2521253002/diff/60001/ios/chrome/browser/net/...
File ios/chrome/browser/net/BUILD.gn (right):

https://codereview.chromium.org/2521253002/diff/60001/ios/chrome/browser/net/...
ios/chrome/browser/net/BUILD.gn:51: ":image_fetcher",
On 2016/11/22 14:38:19, sdefresne wrote:
> Can you add a TODO to remove this public_deps (or move it to deps) once the
> downstream code has been changed to not require it?

Done.

[sdefresne, 2016-11-22 14:47:48]

lgtm

[gambard, 2016-11-22 14:48:05]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2016-11-22 14:48:19]

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-11-22 14:52:50]

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

[commit-bot: I haz the power, 2016-11-22 14:52:51]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)