Move FaviconAttributes to UI

Move FaviconAttributes to UI

Move the FaviconAttributes from ios/chrome/browser/favicon to
ios/chrome/browser/ui/favicon.

BUG=706427
Review-Url: https://codereview.chromium.org/2787503002
Cr-Commit-Position: refs/heads/master@{#461404}
Committed: https://chromium.googlesource.com/chromium/src/+/114e5deb1036e5430295c0e91bed96c8c4e7db95

[gambard, 2017-03-29 14:14:14]

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

[gambard, 2017-03-29 14:14:14]

PTAL!

[lpromero, 2017-03-29 14:36:30]

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

[lpromero, 2017-03-29 14:36:35]

lgtm

[commit-bot: I haz the power, 2017-03-29 14:36:39]

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

[gambard, 2017-03-29 14:38:04]

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

[gambard, 2017-03-29 14:38:05]

+sdefresne: PTAL.

[sdefresne, 2017-03-29 14:42:44]

I'm not sure this is going in the direction we want, as we want to avoid having
ios/chrome/browser depends on ios/chrome/browser/ui (dependencies are allowed in
the other direction).

It looks like you only wants to move favicon_view to ios/chrome/browser/ui.

https://codereview.chromium.org/2787503002/diff/1/ios/chrome/browser/favicon/...
File ios/chrome/browser/favicon/favicon_attributes_provider.h (right):

https://codereview.chromium.org/2787503002/diff/1/ios/chrome/browser/favicon/...
ios/chrome/browser/favicon/favicon_attributes_provider.h:9: #import
"ios/chrome/browser/ui/favicon/favicon_attributes.h"
rohitrao wants to avoid dependencies ios/chrome/browser ->
ios/chrome/browser/ui. Will favicon_attributes_provider.h moves to
ios/chrome/browser/ui eventually?

[lpromero, 2017-03-29 14:48:55]

On 2017/03/29 14:42:44, sdefresne wrote:
> I'm not sure this is going in the direction we want, as we want to avoid
having
> ios/chrome/browser depends on ios/chrome/browser/ui (dependencies are allowed
in
> the other direction).
> 
> It looks like you only wants to move favicon_view to ios/chrome/browser/ui.

No, it was intentional. Maybe what we need is:
- move the provider and maybe more to ui/favicon:favicon and favicon_view and
favicon_attributes to ui/favicon:favicon_ui.

Basically we want FaviconView and FaviconAttributes in the UI layer, while
keeping the rest off of it.

>
https://codereview.chromium.org/2787503002/diff/1/ios/chrome/browser/favicon/...
> File ios/chrome/browser/favicon/favicon_attributes_provider.h (right):
> 
>
https://codereview.chromium.org/2787503002/diff/1/ios/chrome/browser/favicon/...
> ios/chrome/browser/favicon/favicon_attributes_provider.h:9: #import
> "ios/chrome/browser/ui/favicon/favicon_attributes.h"
> rohitrao wants to avoid dependencies ios/chrome/browser ->
> ios/chrome/browser/ui. Will favicon_attributes_provider.h moves to
> ios/chrome/browser/ui eventually?

[gambard, 2017-03-29 16:04:38]

Description was changed from

==========
Move FaviconAttributes to UI

Move the FaviconAttributes from ios/chrome/browser/favicon to
ios/chrome/browser/ui/favicon.

BUG=694220
==========

to

==========
Move FaviconAttributes to UI

Move the FaviconAttributes from ios/chrome/browser/favicon to
ios/chrome/browser/ui/favicon.

BUG=706427
==========

[commit-bot: I haz the power, 2017-03-29 16:05:47]

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

[commit-bot: I haz the power, 2017-03-29 16:05:47]

Dry run: This issue passed the CQ dry run.

[gambard, 2017-03-29 16:17:18]

Thanks, PTAL.

https://codereview.chromium.org/2787503002/diff/1/ios/chrome/browser/favicon/...
File ios/chrome/browser/favicon/favicon_attributes_provider.h (right):

https://codereview.chromium.org/2787503002/diff/1/ios/chrome/browser/favicon/...
ios/chrome/browser/favicon/favicon_attributes_provider.h:9: #import
"ios/chrome/browser/ui/favicon/favicon_attributes.h"
On 2017/03/29 14:42:44, sdefresne wrote:
> rohitrao wants to avoid dependencies ios/chrome/browser ->
> ios/chrome/browser/ui. Will favicon_attributes_provider.h moves to
> ios/chrome/browser/ui eventually?

Done.

[gambard, 2017-03-29 16:17:24]

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

[commit-bot: I haz the power, 2017-03-29 16:18:04]

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

[lpromero, 2017-03-29 16:40:14]

lgtm

[commit-bot: I haz the power, 2017-03-29 17:23:16]

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

[commit-bot: I haz the power, 2017-03-29 17:23:17]

Dry run: This issue passed the CQ dry run.

[gambard, 2017-03-31 08:21:20]

sdefresne: ping

[sdefresne, 2017-03-31 08:35:03]

https://codereview.chromium.org/2787503002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/favicon/favicon_attributes_provider.h (right):

https://codereview.chromium.org/2787503002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/favicon/favicon_attributes_provider.h:9: #import
"ios/chrome/browser/ui/favicon/favicon_attributes.h"
This is the problematic dependency. ios/chrome/browser should not depends on
ios/chrome/browser/ui.

One way to solve this is to create a model only FaviconAttributes like structure
and convert it to ui FaviconAttributes in the ui layer. We did this multiple
time during componentization.

[gambard, 2017-03-31 08:37:02]

https://codereview.chromium.org/2787503002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/favicon/favicon_attributes_provider.h (right):

https://codereview.chromium.org/2787503002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/favicon/favicon_attributes_provider.h:9: #import
"ios/chrome/browser/ui/favicon/favicon_attributes.h"
On 2017/03/31 08:35:03, sdefresne wrote:
> This is the problematic dependency. ios/chrome/browser should not depends on
> ios/chrome/browser/ui.
> 
> One way to solve this is to create a model only FaviconAttributes like
structure
> and convert it to ui FaviconAttributes in the ui layer. We did this multiple
> time during componentization.

I don't understand. This file is in ios/chrome/browser/ui...

[gambard, 2017-03-31 13:19:13]

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

[gambard, 2017-03-31 13:19:15]

marq, PTAL.

[sdefresne, 2017-04-03 09:31:54]

lgtm

https://codereview.chromium.org/2787503002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/favicon/BUILD.gn (right):

https://codereview.chromium.org/2787503002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/favicon/BUILD.gn:4: source_set("favicon") {
nit: blank line

https://codereview.chromium.org/2787503002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/favicon/favicon_attributes_provider.h (right):

https://codereview.chromium.org/2787503002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/favicon/favicon_attributes_provider.h:9: #import
"ios/chrome/browser/ui/favicon/favicon_attributes.h"
On 2017/03/31 08:37:02, gambard wrote:
> On 2017/03/31 08:35:03, sdefresne wrote:
> > This is the problematic dependency. ios/chrome/browser should not depends on
> > ios/chrome/browser/ui.
> > 
> > One way to solve this is to create a model only FaviconAttributes like
> structure
> > and convert it to ui FaviconAttributes in the ui layer. We did this multiple
> > time during componentization.
> 
> I don't understand. This file is in ios/chrome/browser/ui...

Sorry, I didn't see that all files where moved.

[gambard, 2017-04-03 09:33:41]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2017-04-03 09:34:03]

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

[gambard, 2017-04-03 09:36:50]

Thanks!

https://codereview.chromium.org/2787503002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/favicon/BUILD.gn (right):

https://codereview.chromium.org/2787503002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/favicon/BUILD.gn:4: source_set("favicon") {
On 2017/04/03 09:31:53, sdefresne wrote:
> nit: blank line

Done.

[gambard, 2017-04-03 09:36:54]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-04-03 09:36:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2787503002/#ps40001
(title: "Add blank line")

[commit-bot: I haz the power, 2017-04-03 09:37:05]

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, 2017-04-03 10:36:55]

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

[commit-bot: I haz the power, 2017-04-03 10:36:57]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gambard, 2017-04-03 10:37:37]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2017-04-03 10:37:53]

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, 2017-04-03 11:08:54]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491215857816930,
"parent_rev": "bccb2a6db2d722ead168ce0244c4d9d2dc67adab", "commit_rev":
"114e5deb1036e5430295c0e91bed96c8c4e7db95"}

[commit-bot: I haz the power, 2017-04-03 11:09:47]

Description was changed from

==========
Move FaviconAttributes to UI

Move the FaviconAttributes from ios/chrome/browser/favicon to
ios/chrome/browser/ui/favicon.

BUG=706427
==========

to

==========
Move FaviconAttributes to UI

Move the FaviconAttributes from ios/chrome/browser/favicon to
ios/chrome/browser/ui/favicon.

BUG=706427

Review-Url: https://codereview.chromium.org/2787503002
Cr-Commit-Position: refs/heads/master@{#461404}
Committed:
https://chromium.googlesource.com/chromium/src/+/114e5deb1036e5430295c0e91bed...
==========

[commit-bot: I haz the power, 2017-04-03 11:09:49]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/114e5deb1036e5430295c0e91bed...