[Icons NTP] Refactoring: Moving FallbackIconService to components\favicon\core

[Icons NTP] Refactoring: Moving FallbackIconService to components\favicon\core

The class used to be in components\favicon_base. The DEP changes arise
due to movement of fallback icon drawing code.

BUG=467712
Committed: https://crrev.com/ced2f17446e45420a4d6072d67adce436e32a6de
Cr-Commit-Position: refs/heads/master@{#322157}

[huangs, 2015-03-24 17:01:06]

huangs@chromium.org changed reviewers:
+ bsalomon@google.com, danakj@chromium.org, pkasting@chromium.org,
sky@chromium.org

[huangs, 2015-03-24 17:01:07]

This is sliced off crrev.com/996253002.  Later I'll add FallbackIconClient and
do more refactoring.

OWNER reviews:

sky@ for most of the changes.

pkasting@ for DEP
'+net/base/registry_controlled_domains/registry_controlled_domain.h'  (will move
this again in the follow up)

bsalomon@ for DEP '+third_party/skia/include',

danakj@ for DEP '+ui/gfx'

PTAL.  Thanks!

[danakj, 2015-03-24 17:02:16]

On 2015/03/24 17:01:07, huangs wrote:
> This is sliced off crrev.com/996253002.  Later I'll add FallbackIconClient and
> do more refactoring.
> 
> OWNER reviews:
> 
> sky@ for most of the changes.
> 
> pkasting@ for DEP
> '+net/base/registry_controlled_domains/registry_controlled_domain.h'  (will
move
> this again in the follow up)
> 
> bsalomon@ for DEP '+third_party/skia/include',
> 
> danakj@ for DEP '+ui/gfx'

sky@ can cover this too.

[danakj, 2015-03-24 17:02:26]

danakj@chromium.org changed reviewers:
- danakj@chromium.org

[sky, 2015-03-24 18:29:02]

LGTM

[huangs, 2015-03-24 20:05:52]

The CQ bit was checked by huangs@chromium.org

[huangs, 2015-03-24 20:05:52]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1032683002/#ps20001
(title: "Putting back +third_party/skia/include for favicon_base DEPS.")

[commit-bot: I haz the power, 2015-03-24 20:06:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1032683002/20001

[commit-bot: I haz the power, 2015-03-24 20:14:29]

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

[commit-bot: I haz the power, 2015-03-24 20:14:30]

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

[huangs, 2015-03-24 20:16:29]

Ping pkasting@ and bsalomon@: I need your approvals for additions to
components/favicon/core/DEPS .  Thanks!

[Peter Kasting, 2015-03-24 20:52:48]

On 2015/03/24 20:16:29, huangs wrote:
> Ping pkasting@ and bsalomon@: I need your approvals for additions to
> components/favicon/core/DEPS .  Thanks!

I'm not sure why you need my approval; I'm not a favicon OWNER.  If you're
looking for an evaluation about whether depending on net/ is OK, you'd probably
want to check with someone who understands componentizatino better, or a net/
OWNER.

[huangs, 2015-03-24 21:18:15]

You were listed in 
\net\base\registry_controlled_domains\OWNER.  I moved a dependency to 

"+net/base/registry_controlled_domains/registry_controlled_domain.h"

and the system recommended you as well.

[Peter Kasting, 2015-03-24 21:19:55]

On 2015/03/24 21:18:15, huangs wrote:
> You were listed in 
> \net\base\registry_controlled_domains\OWNER.  I moved a dependency to 
> 
> "+net/base/registry_controlled_domains/registry_controlled_domain.h"
> 
> and the system recommended you as well.

I understand the RCDS code and can review changes to it, but I don't really have
an idea of what sorts of problems I'm supposed to look out for with this type of
dependency-adding.

I thought you only needed approval from an OWNER of your updated DEPS file,
anyway?

[huangs, 2015-03-24 21:30:23]

The failed test states

** Presubmit ERRORS **
Missing LGTM from OWNERS of dependencies added to DEPS:
    '+net/base/registry_controlled_domains/registry_controlled_domain.h',
    '+third_party/skia/include',

[Peter Kasting, 2015-03-24 21:32:09]

On 2015/03/24 21:30:23, huangs wrote:
> The failed test states
> 
> ** Presubmit ERRORS **
> Missing LGTM from OWNERS of dependencies added to DEPS:
>     '+net/base/registry_controlled_domains/registry_controlled_domain.h',
>     '+third_party/skia/include',

Ugh.  OK, then please check with a componentization expert and/or general net/
OWNER who would be more likely to know whether this is safe to approve.  I don't
feel like I can review.

[huangs, 2015-03-24 21:36:06]

No problem, thanks!

[huangs, 2015-03-24 21:39:18]

huangs@chromium.org changed reviewers:
+ rsleevi@chromium.org
- pkasting@chromium.org

[huangs, 2015-03-24 21:39:19]

OWNER review for rsleevi@.  I need approval because I 
moved

"+net/base/registry_controlled_domains/registry_controlled_domain.h"

from components/favicon_base/DEPS to components/favicon/core/DEPS .

PTAL. Also pinging bsalomon@.

[Ryan Sleevi, 2015-03-24 21:43:22]

lgtm

[bsalomon, 2015-03-25 14:08:59]

https://codereview.chromium.org/1032683002/diff/20001/components/favicon/core...
File components/favicon/core/DEPS (right):

https://codereview.chromium.org/1032683002/diff/20001/components/favicon/core...
components/favicon/core/DEPS:8: "+third_party/skia/include",
lgtm

[huangs, 2015-03-25 14:09:59]

Thanks. Committing!

https://codereview.chromium.org/1032683002/diff/20001/components/favicon/core...
File components/favicon/core/DEPS (right):

https://codereview.chromium.org/1032683002/diff/20001/components/favicon/core...
components/favicon/core/DEPS:8: "+third_party/skia/include",
On 2015/03/25 14:08:59, bsalomon wrote:
> lgtm

Acknowledged.

[huangs, 2015-03-25 14:10:03]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2015-03-25 14:10:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1032683002/20001

[commit-bot: I haz the power, 2015-03-25 14:20:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-25 14:21:35]

Patchset 2 (id:??) landed as
https://crrev.com/ced2f17446e45420a4d6072d67adce436e32a6de
Cr-Commit-Position: refs/heads/master@{#322157}