[Fallback Icons] Refactor FallbackIconService to be a BrowserContext-level singleton

[Fallback Icons] Refactor FallbackIconService to be a BrowserContext-level singleton

- FallbackIconServiceFactory now get sFallbackIconService singleton from
  BrowserContext.
- FallbackIconClient & ChromeFallbackIconClient: wrap external code so the Favicon
  component won't have to include them. Specifically:
  - Choosing fonts: also removed duplicate code.
  - Extracting letter from URL to render Fallback.

BUG=455063
Committed: https://crrev.com/f16444b34b44d6e9de555756e24017ff7670d73a
Cr-Commit-Position: refs/heads/master@{#322653}

[sdefresne, 2015-03-11 16:22:05]

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

[sdefresne, 2015-03-11 16:22:06]

drive-by-comments, hope you don't mind ...

https://codereview.chromium.org/996253002/diff/1/components/favicon/core/brow...
File components/favicon/core/browser/fallback_icon_client.h (right):

https://codereview.chromium.org/996253002/diff/1/components/favicon/core/brow...
components/favicon/core/browser/fallback_icon_client.h:21: ~FallbackIconClient()
override{};
nit: remove trailing semi-colon, style is

~FallbackIconClient() override {}

https://codereview.chromium.org/996253002/diff/1/components/favicon/core/brow...
components/favicon/core/browser/fallback_icon_client.h:23: virtual
favicon_base::FallbackIconService* GetFallbackIconService() = 0;
drive-by: please put comment for those methods

[huangs, 2015-03-11 18:00:25]

Thanks. Updated with some refactoring.

https://codereview.chromium.org/996253002/diff/1/components/favicon/core/brow...
File components/favicon/core/browser/fallback_icon_client.h (right):

https://codereview.chromium.org/996253002/diff/1/components/favicon/core/brow...
components/favicon/core/browser/fallback_icon_client.h:21: ~FallbackIconClient()
override{};
Done.  This was copied from favicon_client.h though.  Updating that as well.

https://codereview.chromium.org/996253002/diff/1/components/favicon/core/brow...
components/favicon/core/browser/fallback_icon_client.h:23: virtual
favicon_base::FallbackIconService* GetFallbackIconService() = 0;
Done, also renamed GetFontList() to GetFontNameList() since gfx:FontList() is
something else.

[sdefresne, 2015-03-11 18:06:20]

https://codereview.chromium.org/996253002/diff/1/components/favicon/core/brow...
File components/favicon/core/browser/fallback_icon_client.h (right):

https://codereview.chromium.org/996253002/diff/1/components/favicon/core/brow...
components/favicon/core/browser/fallback_icon_client.h:21: ~FallbackIconClient()
override{};
On 2015/03/11 18:00:25, huangs wrote:
> Done.  This was copied from favicon_client.h though.  Updating that as well.

FaviconClient is already fixed by https://codereview.chromium.org/987113005/.

[huangs, 2015-03-14 02:00:09]

Waiting on crrev.com/983043003 and may to merge , but advanced reviewing would
be appreciated.

[sdefresne, 2015-03-16 09:11:29]

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/favicon/c...
File chrome/browser/favicon/chrome_fallback_icon_client.cc (right):

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/favicon/c...
chrome/browser/favicon/chrome_fallback_icon_client.cc:27: return
FallbackIconServiceFactory::GetForProfile(
As I'll say later, please remove this method.

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/favicon/f...
File chrome/browser/favicon/fallback_icon_service_factory.cc (right):

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/favicon/f...
chrome/browser/favicon/fallback_icon_service_factory.cc:31: return NULL;
nit: s/NULL/nullptr/ everywhere (except in comments where it should be null).

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/search/in...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:131:
ChromeFallbackIconClientFactory::GetForProfile(profile_)));
FallbackIconSource only call the FallbackIconClient::GetFallbackIconService() on
the client so instead pass a pointer to the FallbackIconService here.

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/fallback_icon_source.cc (right):

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/fallback_icon_source.cc:72:
favicon_base::FallbackIconService* fallback_icon_service =
Why receive the client to only use it to access the service. Just inject the
service instead.

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/fallback_icon_source.h (right):

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/fallback_icon_source.h:51: explicit
FallbackIconSource(FallbackIconClient* fallback_icon_client);
Should instead use the FallbackIconService.

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:87:
ChromeFallbackIconClientFactory::GetForProfile(profile)));
Same.

https://codereview.chromium.org/996253002/diff/40001/chrome/test/base/testing...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/996253002/diff/40001/chrome/test/base/testing...
chrome/test/base/testing_profile.h:167: // Creates the fallbck icon service.
Consequent calls would recreate the
nit: s/fallbck/fallback/

https://codereview.chromium.org/996253002/diff/40001/components/favicon/core/...
File components/favicon/core/browser/fallback_icon_client.h (right):

https://codereview.chromium.org/996253002/diff/40001/components/favicon/core/...
components/favicon/core/browser/fallback_icon_client.h:24: virtual
favicon_base::FallbackIconService* GetFallbackIconService() = 0;
Don't provide virtual method on the ${Feature}Client to access the
${Feature}Service, this is an anti-pattern I'd like to see disappear.

The code accessing the Client should either be passed a pointer to the Service
if it also need the service, or if accessing the Client through a factory, it
can just use the Service factory instead.

The reason for opposing to this pattern is that the Service already has a
dependency on the Client. So if the Client has a dependency on the Service we
have a case of circular dependency, which does not work.

More generally the Client is supposed to be an implementation detail of the
Service and not accessed outside of the Service.

[huangs, 2015-03-16 22:06:11]

Updated, PTAL.

Also, I'm wondering I should move fallback_icon_service.* to where
favicon_service.* is?  Thanks.

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/favicon/c...
File chrome/browser/favicon/chrome_fallback_icon_client.cc (right):

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/favicon/c...
chrome/browser/favicon/chrome_fallback_icon_client.cc:27: return
FallbackIconServiceFactory::GetForProfile(
On 2015/03/16 09:11:29, sdefresne wrote:
> As I'll say later, please remove this method.

Done.

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/favicon/f...
File chrome/browser/favicon/fallback_icon_service_factory.cc (right):

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/favicon/f...
chrome/browser/favicon/fallback_icon_service_factory.cc:31: return NULL;
Done.  Doing this for new code (rather than global change).

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/search/in...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:131:
ChromeFallbackIconClientFactory::GetForProfile(profile_)));
On 2015/03/16 09:11:29, sdefresne wrote:
> FallbackIconSource only call the FallbackIconClient::GetFallbackIconService()
on
> the client so instead pass a pointer to the FallbackIconService here.

Done.

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/fallback_icon_source.cc (right):

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/fallback_icon_source.cc:72:
favicon_base::FallbackIconService* fallback_icon_service =
On 2015/03/16 09:11:29, sdefresne wrote:
> Why receive the client to only use it to access the service. Just inject the
> service instead.

Done.

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/fallback_icon_source.h (right):

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/fallback_icon_source.h:51: explicit
FallbackIconSource(FallbackIconClient* fallback_icon_client);
On 2015/03/16 09:11:29, sdefresne wrote:
> Should instead use the FallbackIconService.

Done.

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):

https://codereview.chromium.org/996253002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:87:
ChromeFallbackIconClientFactory::GetForProfile(profile)));
On 2015/03/16 09:11:29, sdefresne wrote:
> Same.

Done.

https://codereview.chromium.org/996253002/diff/40001/chrome/test/base/testing...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/996253002/diff/40001/chrome/test/base/testing...
chrome/test/base/testing_profile.h:167: // Creates the fallbck icon service.
Consequent calls would recreate the
On 2015/03/16 09:11:29, sdefresne wrote:
> nit: s/fallbck/fallback/

Done.

https://codereview.chromium.org/996253002/diff/40001/components/favicon/core/...
File components/favicon/core/browser/fallback_icon_client.h (right):

https://codereview.chromium.org/996253002/diff/40001/components/favicon/core/...
components/favicon/core/browser/fallback_icon_client.h:24: virtual
favicon_base::FallbackIconService* GetFallbackIconService() = 0;
Ah oops. I thought that's the endorsed pattern, hence the change. Done.

[sdefresne, 2015-03-17 09:41:52]

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/c...
File chrome/browser/favicon/chrome_fallback_icon_client.h (right):

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/c...
chrome/browser/favicon/chrome_fallback_icon_client.h:22: //
ChromeFallbackIconClient implements the the FallbackIconClient interface.
nit: s/the the/the/

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/c...
chrome/browser/favicon/chrome_fallback_icon_client.h:25: explicit
ChromeFallbackIconClient(Profile* profile);
You don't use profile, so there is no need to pass it to the constructor nor
keep a pointer to it as a member variable.

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/c...
File chrome/browser/favicon/chrome_fallback_icon_client_factory.h (right):

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/c...
chrome/browser/favicon/chrome_fallback_icon_client_factory.h:9: #include
"chrome/browser/favicon/chrome_fallback_icon_client.h"
You don't need to #include those two files (singleton.h and
chrome_fallback_icon_client.h), instead forward declare the
DefaultSingletonTraits struct and FallbackIconClient.

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/f...
File chrome/browser/favicon/fallback_icon_service_factory.cc (right):

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/f...
chrome/browser/favicon/fallback_icon_service_factory.cc:43:
DependsOn(HistoryServiceFactory::GetInstance());
Why this dependency if you don't use the HistoryServiceFactory?

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/f...
File chrome/browser/favicon/fallback_icon_service_factory.h (right):

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/f...
chrome/browser/favicon/fallback_icon_service_factory.h:8: #include
"base/memory/singleton.h"
No need for this #include since you forward-declare DefaultSingletonTraits
struct.

https://codereview.chromium.org/996253002/diff/60001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/996253002/diff/60001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:219: static_cast<Profile*>(profile));
nit: Profile::FromBrowserContext(profile)

https://codereview.chromium.org/996253002/diff/60001/components/favicon_base/...
File components/favicon_base/fallback_icon_service.cc (right):

https://codereview.chromium.org/996253002/diff/60001/components/favicon_base/...
components/favicon_base/fallback_icon_service.cc:11: #include
"components/favicon/core/browser/fallback_icon_client.h"
favicon_base component must not depends on favicon component. I'd suggest moving
fallback_icon_service.{h,cc} to components/favicon/core/browser if it need to
depends on fallback_icon_client.h or to move fallback_icon_client.h to
components/favicon_base.

I prefer the first option as the goal of favicon_base was to be as small as
possible, providing a common layer that both favicon and history could depends
on. Since history does not use FallbackIconService, there is no need to have it
in the favicon_base component.

[huangs, 2015-03-23 03:28:35]

sdefresne@: Merged with your cleanup CL crrev.com/983043003 , and included my
recent addition of LargeIcon stuff.  PTAL.

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/c...
File chrome/browser/favicon/chrome_fallback_icon_client.h (right):

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/c...
chrome/browser/favicon/chrome_fallback_icon_client.h:22: //
ChromeFallbackIconClient implements the the FallbackIconClient interface.
On 2015/03/17 09:41:52, sdefresne wrote:
> nit: s/the the/the/

Done.

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/c...
chrome/browser/favicon/chrome_fallback_icon_client.h:25: explicit
ChromeFallbackIconClient(Profile* profile);
On 2015/03/17 09:41:52, sdefresne wrote:
> You don't use profile, so there is no need to pass it to the constructor nor
> keep a pointer to it as a member variable.

Done.

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/c...
File chrome/browser/favicon/chrome_fallback_icon_client_factory.h (right):

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/c...
chrome/browser/favicon/chrome_fallback_icon_client_factory.h:9: #include
"chrome/browser/favicon/chrome_fallback_icon_client.h"
On 2015/03/17 09:41:52, sdefresne wrote:
> You don't need to #include those two files (singleton.h and
> chrome_fallback_icon_client.h), instead forward declare the
> DefaultSingletonTraits struct and FallbackIconClient.

Done.

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/f...
File chrome/browser/favicon/fallback_icon_service_factory.cc (right):

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/f...
chrome/browser/favicon/fallback_icon_service_factory.cc:43:
DependsOn(HistoryServiceFactory::GetInstance());
Removed History stuff.

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/f...
File chrome/browser/favicon/fallback_icon_service_factory.h (right):

https://codereview.chromium.org/996253002/diff/60001/chrome/browser/favicon/f...
chrome/browser/favicon/fallback_icon_service_factory.h:8: #include
"base/memory/singleton.h"
On 2015/03/17 09:41:52, sdefresne wrote:
> No need for this #include since you forward-declare DefaultSingletonTraits
> struct.

Done.

https://codereview.chromium.org/996253002/diff/60001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/996253002/diff/60001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:219: static_cast<Profile*>(profile));
On 2015/03/17 09:41:52, sdefresne wrote:
> nit: Profile::FromBrowserContext(profile)

Done.

https://codereview.chromium.org/996253002/diff/60001/components/favicon_base/...
File components/favicon_base/fallback_icon_service.cc (right):

https://codereview.chromium.org/996253002/diff/60001/components/favicon_base/...
components/favicon_base/fallback_icon_service.cc:11: #include
"components/favicon/core/browser/fallback_icon_client.h"
Moved to components/favicon/core to join siblings favicon_service.* (browser
directory got deleted).

[huangs, 2015-03-24 05:11:06]

sdefresne@: PTAL. Thanks!

[chromium-reviews, 2015-03-24 10:49:13]

I'm OOO this week and won't be able to give a look before next Monday.
Sorry,
-- Sylvain
Le mar. 24 mars 2015 à 06:11, <huangs@chromium.org> a écrit :

> sdefresne@: PTAL. Thanks!
>
> https://codereview.chromium.org/996253002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[huangs, 2015-03-24 12:46:47]

On 2015/03/24 10:49:13, chromium-reviews wrote:
> I'm OOO this week and won't be able to give a look before next Monday.
> Sorry,
> -- Sylvain
> Le mar. 24 mars 2015 à 06:11, <mailto:huangs@chromium.org> a écrit :
> 
> > sdefresne@: PTAL. Thanks!
> >
> > https://codereview.chromium.org/996253002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Thanks for letting me know.  I'll bug sky@ for this.

[huangs, 2015-03-25 21:56:38]

huangs@chromium.org changed reviewers:
+ sky@chromium.org
- sdefresne@chromium.org

[huangs, 2015-03-25 21:56:39]

Pulled movement of FallbackIconService to https://crrev.com/1032683002/.

Requesting review from sky@, PTAL

[sky, 2015-03-25 23:19:36]

On 2015/03/25 21:56:39, huangs wrote:
> Pulled movement of FallbackIconService to https://crrev.com/1032683002/.
> 
> Requesting review from sky@, PTAL

The whole thing, or just parts?

[huangs, 2015-03-26 00:49:13]

On 2015/03/25 23:19:36, sky wrote:
> On 2015/03/25 21:56:39, huangs wrote:
> > Pulled movement of FallbackIconService to https://crrev.com/1032683002/.
> > 
> > Requesting review from sky@, PTAL
> 
> The whole thing, or just parts?

Preferably the whole thing, since it's componentization and favicon. Let me know
if you want me to split it some more.

[huangs, 2015-03-26 03:30:24]

huangs@chromium.org changed reviewers:
+ dbeam@chromium.org, jcivelli@chromium.org, pkotwicz@chromium.org,
samarth@chromium.org

[huangs, 2015-03-26 03:30:25]

Breaking the CL seems rather counter-productive at this point.  I'll invite more
reviewers.

sky@:
  components\favicon.gypi
  components\favicon\core\BUILD.gn
  components\favicon\core\DEPS
  components\favicon\core\fallback_icon_client.h
  components\favicon\core\fallback_icon_service.cc
  components\favicon\core\fallback_icon_service.h

pkotwicz@:
  chrome\browser\favicon\DEPS
  chrome\browser\favicon\chrome_fallback_icon_client.cc
  chrome\browser\favicon\chrome_fallback_icon_client.h
  chrome\browser\favicon\chrome_fallback_icon_client_factory.cc
  chrome\browser\favicon\chrome_fallback_icon_client_factory.h
  chrome\browser\favicon\fallback_icon_service_factory.cc
  chrome\browser\favicon\fallback_icon_service_factory.h

samarth@:
  chrome\browser\search\instant_service.cc

dbeam@:
  chrome\browser\ui\webui\fallback_icon_source.cc
  chrome\browser\ui\webui\fallback_icon_source.h
  chrome\browser\ui\webui\large_icon_source.cc
  chrome\browser\ui\webui\large_icon_source.h
  chrome\browser\ui\webui\ntp\most_visited_handler.cc

jcivelli@:
  chrome\test\base\testing_profile.cc
  chrome\test\base\testing_profile.h

PTAL. Thanks!

[huangs, 2015-03-26 03:35:28]

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

[huangs, 2015-03-26 03:35:29]

Also +rsleevi@ for DEPS change: moving

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

from components/favicon/core/DEPS to chrome/browser/favicon.

[Jay Civelli, 2015-03-26 05:14:56]

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/test/base...
File chrome/test/base/testing_profile.h (right):

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/test/base...
chrome/test/base/testing_profile.h:10: #include <vector>
Do you need these includes?

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/test/base...
chrome/test/base/testing_profile.h:170: // service.
I am not sure I understand the "Consequent calls would recreate the
 service." Looking at the code it does not look like calling that again will
recreate anything.

[beaudoin, 2015-03-26 12:53:54]

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

[beaudoin, 2015-03-26 12:53:55]

Drive-by LGTM with a couple of nits. Nice clean-up! Thanks!

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/f...
File chrome/browser/favicon/chrome_fallback_icon_client.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/f...
chrome/browser/favicon/chrome_fallback_icon_client.cc:38: url,
net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
Indentation.

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/f...
File chrome/browser/favicon/chrome_fallback_icon_client_factory.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/f...
chrome/browser/favicon/chrome_fallback_icon_client_factory.cc:16:
BrowserContextDependencyManager::GetInstance()) {
Indentation should be 8 I believe.

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/f...
File chrome/browser/favicon/fallback_icon_service_factory.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/f...
chrome/browser/favicon/fallback_icon_service_factory.cc:21: // Profile must be
OffTheRecord in this case.
Not sure I like the phrasing, I'd go with:
// Profile is OffTheRecord with access.

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/f...
chrome/browser/favicon/fallback_icon_service_factory.cc:27: // Profile is
OffTheRecord without access.
Explain in the comment why this should never happen.

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/s...
File chrome/browser/search/instant_service.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/s...
chrome/browser/search/instant_service.cc:146: new
FallbackIconSource(fallback_icon_service));
I'm not super aware of the lifecycle of these different objects, but I just want
to point out that |favicon_service| and|fallback_icon_service| have to outlive
the LargeIconSource and FallbackIconSource. If you're confident they do I'm
fine.

[sky, 2015-03-26 14:09:37]

components LGTM

https://chromiumcodereview.appspot.com/996253002/diff/140001/components/favic...
File components/favicon/core/fallback_icon_client.h (right):

https://chromiumcodereview.appspot.com/996253002/diff/140001/components/favic...
components/favicon/core/fallback_icon_client.h:24: ~FallbackIconClient()
override {}
Make this protected so that it's clearer FallbackIconService doesn't own this
class.

[huangs, 2015-03-26 18:26:07]

Updated, PTAL.

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/f...
File chrome/browser/favicon/chrome_fallback_icon_client.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/f...
chrome/browser/favicon/chrome_fallback_icon_client.cc:38: url,
net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
On 2015/03/26 12:53:55, beaudoin wrote:
> Indentation.

Done.

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/f...
File chrome/browser/favicon/chrome_fallback_icon_client_factory.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/f...
chrome/browser/favicon/chrome_fallback_icon_client_factory.cc:16:
BrowserContextDependencyManager::GetInstance()) {
On 2015/03/26 12:53:55, beaudoin wrote:
> Indentation should be 8 I believe.

Done.

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/f...
File chrome/browser/favicon/fallback_icon_service_factory.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/f...
chrome/browser/favicon/fallback_icon_service_factory.cc:21: // Profile must be
OffTheRecord in this case.
The file is branched from favicon_service_factory.cc and that's what's done
there. Same with below.  Should both be fixed?

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/f...
chrome/browser/favicon/fallback_icon_service_factory.cc:27: // Profile is
OffTheRecord without access.
Will continue discussion once I uploaded with lower diff threshold.

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/s...
File chrome/browser/search/instant_service.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/browser/s...
chrome/browser/search/instant_service.cc:146: new
FallbackIconSource(fallback_icon_service));
I added these because favicon_service === nullptr for a unit test, and this
triggers assert I added.  Come to think of it, doing defensive programming here
is likely bad.  I should be defensive within the services (observed a similar
pattern in previous codes).

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/test/base...
File chrome/test/base/testing_profile.h (right):

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/test/base...
chrome/test/base/testing_profile.h:10: #include <vector>
I don't, but these were suggested by "git cl lint".

https://chromiumcodereview.appspot.com/996253002/diff/140001/chrome/test/base...
chrome/test/base/testing_profile.h:170: // service.
The comment was copied from the line 173.  By "Looking at the code" you meant
digging into FaviconServiceFactory and see that BuildFallbackIconService() only
gets called once at most?

Removed "Consequent calls..." from new code.

https://chromiumcodereview.appspot.com/996253002/diff/140001/components/favic...
File components/favicon/core/fallback_icon_client.h (right):

https://chromiumcodereview.appspot.com/996253002/diff/140001/components/favic...
components/favicon/core/fallback_icon_client.h:24: ~FallbackIconClient()
override {}
On 2015/03/26 14:09:37, sky wrote:
> Make this protected so that it's clearer FallbackIconService doesn't own this
> class.

Done.

[Ryan Sleevi, 2015-03-26 18:36:01]

DEP LGTM

[huangs, 2015-03-26 19:12:42]

Thanks!

Ping pkotwicz@, samarth@, and dbeam@, PTAL.

[pkotwicz, 2015-03-27 03:52:09]

Just nits

https://codereview.chromium.org/996253002/diff/160001/chrome/browser/favicon/...
File chrome/browser/favicon/chrome_fallback_icon_client.cc (right):

https://codereview.chromium.org/996253002/diff/160001/chrome/browser/favicon/...
chrome/browser/favicon/chrome_fallback_icon_client.cc:34: // TODO(huangs):
Handle non-ASCII ("xn--") domain names.
Nit: Move the comment inside the function.

Returns -> Return
"a page URL" -> "|url|"

https://codereview.chromium.org/996253002/diff/160001/chrome/browser/favicon/...
File chrome/browser/favicon/chrome_fallback_icon_client_factory.cc (right):

https://codereview.chromium.org/996253002/diff/160001/chrome/browser/favicon/...
chrome/browser/favicon/chrome_fallback_icon_client_factory.cc:10: #include
"chrome/browser/profiles/profile.h"
Nit: Including profile.h is unnecessary because you don't do anything with the
profile. (You just pass it on to GetServiceForBrowserContext())

https://codereview.chromium.org/996253002/diff/160001/chrome/browser/favicon/...
chrome/browser/favicon/chrome_fallback_icon_client_factory.cc:38: return client;
Nit: "return new ChromeFallbackIconClient();"

https://codereview.chromium.org/996253002/diff/160001/chrome/browser/favicon/...
File chrome/browser/favicon/fallback_icon_service_factory.cc (right):

https://codereview.chromium.org/996253002/diff/160001/chrome/browser/favicon/...
chrome/browser/favicon/fallback_icon_service_factory.cc:8: #include
"base/prefs/pref_service.h"
Is this include necessary?

https://codereview.chromium.org/996253002/diff/160001/chrome/browser/favicon/...
chrome/browser/favicon/fallback_icon_service_factory.cc:29: return nullptr;
Is there a reason that you are passing in the |sat| parameter? From looking at
your CL, you always pass in EXPLICIT_ACCESS.

Aside: I think that the ServiceAccessType parameter can be removed from
FaviconServiceFactory.

https://codereview.chromium.org/996253002/diff/160001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/996253002/diff/160001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:138: profile_,
ServiceAccessType::EXPLICIT_ACCESS);
Nit: Remove new line

https://codereview.chromium.org/996253002/diff/160001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:142: 
Nit: Remove new line

https://codereview.chromium.org/996253002/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/large_icon_source.cc (right):

https://codereview.chromium.org/996253002/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/large_icon_source.cc:69: !favicon_service_) {
Nit: You can remove the check for |favicon_service_| here. You should have
returned on line 61 if |favicon_service_| is null

https://codereview.chromium.org/996253002/diff/160001/components/favicon/core...
File components/favicon/core/fallback_icon_client.h (right):

https://codereview.chromium.org/996253002/diff/160001/components/favicon/core...
components/favicon/core/fallback_icon_client.h:17: class FallbackIconService;
Can you remove the forward declaration?

https://codereview.chromium.org/996253002/diff/160001/components/favicon/core...
File components/favicon/core/fallback_icon_service.cc (right):

https://codereview.chromium.org/996253002/diff/160001/components/favicon/core...
components/favicon/core/fallback_icon_service.cc:76: // TODO(huangs): See how
expensive gfx::FontList() is, and possibly cache.
You can remove this TODO :)

[huangs, 2015-03-27 17:33:07]

PTAL, did a bit more cleanup, and 25% similarity match so branched files don't
look like new files.

https://chromiumcodereview.appspot.com/996253002/diff/160001/chrome/browser/f...
File chrome/browser/favicon/chrome_fallback_icon_client.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/160001/chrome/browser/f...
chrome/browser/favicon/chrome_fallback_icon_client.cc:34: // TODO(huangs):
Handle non-ASCII ("xn--") domain names.
Done, but keeping "Returns". My observation of the convention is that there's an
implied "This function..." before function comments.

https://chromiumcodereview.appspot.com/996253002/diff/160001/chrome/browser/f...
File chrome/browser/favicon/chrome_fallback_icon_client_factory.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/160001/chrome/browser/f...
chrome/browser/favicon/chrome_fallback_icon_client_factory.cc:10: #include
"chrome/browser/profiles/profile.h"
GetServiceForBrowserContext() call has an implicit upcast from Profile to
content::BrowserContext.

However, digging deeper I realized I can do more refactoring and replace Profile
with BrowserContext.

https://chromiumcodereview.appspot.com/996253002/diff/160001/chrome/browser/f...
chrome/browser/favicon/chrome_fallback_icon_client_factory.cc:38: return client;
On 2015/03/27 03:52:09, pkotwicz wrote:
> Nit: "return new ChromeFallbackIconClient();"

Done.

https://chromiumcodereview.appspot.com/996253002/diff/160001/chrome/browser/f...
File chrome/browser/favicon/fallback_icon_service_factory.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/160001/chrome/browser/f...
chrome/browser/favicon/fallback_icon_service_factory.cc:8: #include
"base/prefs/pref_service.h"
Removed.

https://chromiumcodereview.appspot.com/996253002/diff/160001/chrome/browser/f...
chrome/browser/favicon/fallback_icon_service_factory.cc:29: return nullptr;
FallbackIconServiceFactory was forked from FaviconServiceFactory. But I'll
remove it for parsimony (if it were part of the interface it should have been in
a base class).

https://chromiumcodereview.appspot.com/996253002/diff/160001/chrome/browser/s...
File chrome/browser/search/instant_service.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/160001/chrome/browser/s...
chrome/browser/search/instant_service.cc:138: profile_,
ServiceAccessType::EXPLICIT_ACCESS);
On 2015/03/27 03:52:09, pkotwicz wrote:
> Nit: Remove new line

Done.

https://chromiumcodereview.appspot.com/996253002/diff/160001/chrome/browser/s...
chrome/browser/search/instant_service.cc:142: 
On 2015/03/27 03:52:09, pkotwicz wrote:
> Nit: Remove new line

Done.

https://chromiumcodereview.appspot.com/996253002/diff/160001/chrome/browser/u...
File chrome/browser/ui/webui/large_icon_source.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/160001/chrome/browser/u...
chrome/browser/ui/webui/large_icon_source.cc:69: !favicon_service_) {
Ah forgot to delete. Done.

https://chromiumcodereview.appspot.com/996253002/diff/160001/components/favic...
File components/favicon/core/fallback_icon_client.h (right):

https://chromiumcodereview.appspot.com/996253002/diff/160001/components/favic...
components/favicon/core/fallback_icon_client.h:17: class FallbackIconService;
On 2015/03/27 03:52:09, pkotwicz wrote:
> Can you remove the forward declaration?

Done.

https://chromiumcodereview.appspot.com/996253002/diff/160001/components/favic...
File components/favicon/core/fallback_icon_service.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/160001/components/favic...
components/favicon/core/fallback_icon_service.cc:76: // TODO(huangs): See how
expensive gfx::FontList() is, and possibly cache.
Done. :)

[pkotwicz, 2015-03-27 18:47:58]

LGTM with nits

https://codereview.chromium.org/996253002/diff/180001/chrome/browser/favicon/...
File chrome/browser/favicon/chrome_fallback_icon_client_factory.h (right):

https://codereview.chromium.org/996253002/diff/180001/chrome/browser/favicon/...
chrome/browser/favicon/chrome_fallback_icon_client_factory.h:39:
content::BrowserContext* /* context */) const override;
Nit: Don't comment out |context| in the header

https://codereview.chromium.org/996253002/diff/180001/chrome/browser/favicon/...
File chrome/browser/favicon/fallback_icon_service_factory.h (right):

https://codereview.chromium.org/996253002/diff/180001/chrome/browser/favicon/...
chrome/browser/favicon/fallback_icon_service_factory.h:19: // Profiles.
Nit: Profiles -> BrowserContexts

[pkotwicz, 2015-03-27 18:52:31]

https://codereview.chromium.org/996253002/diff/180001/chrome/browser/favicon/...
File chrome/browser/favicon/chrome_fallback_icon_client_factory.cc (right):

https://codereview.chromium.org/996253002/diff/180001/chrome/browser/favicon/...
chrome/browser/favicon/chrome_fallback_icon_client_factory.cc:43: return
chrome::GetBrowserContextRedirectedInIncognito(context);
Nit: Remove this method. The factories for FallbackIconServiceFactory should be
very similar.

I know that it "doesn't really matter" because the BrowserContext is not used by
either FallbackIconService or ChromeFallbackIconClient

[huangs, 2015-03-27 18:53:16]

Patchset #11 (id:200001) has been deleted

[huangs, 2015-03-27 19:01:51]

Thanks! Updated.

https://chromiumcodereview.appspot.com/996253002/diff/180001/chrome/browser/f...
File chrome/browser/favicon/chrome_fallback_icon_client_factory.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/180001/chrome/browser/f...
chrome/browser/favicon/chrome_fallback_icon_client_factory.cc:43: return
chrome::GetBrowserContextRedirectedInIncognito(context);
On 2015/03/27 18:52:31, pkotwicz wrote:
> Nit: Remove this method. The factories for FallbackIconServiceFactory should
be
> very similar.
> 
> I know that it "doesn't really matter" because the BrowserContext is not used
by
> either FallbackIconService or ChromeFallbackIconClient

Done.

https://chromiumcodereview.appspot.com/996253002/diff/180001/chrome/browser/f...
File chrome/browser/favicon/chrome_fallback_icon_client_factory.h (right):

https://chromiumcodereview.appspot.com/996253002/diff/180001/chrome/browser/f...
chrome/browser/favicon/chrome_fallback_icon_client_factory.h:39:
content::BrowserContext* /* context */) const override;
I thought that's standard practice to specify that param is unused. But I don't
feel that strongly about this. Changed back (also in .cc file).

https://chromiumcodereview.appspot.com/996253002/diff/180001/chrome/browser/f...
File chrome/browser/favicon/fallback_icon_service_factory.h (right):

https://chromiumcodereview.appspot.com/996253002/diff/180001/chrome/browser/f...
chrome/browser/favicon/fallback_icon_service_factory.h:19: // Profiles.
On 2015/03/27 18:47:57, pkotwicz wrote:
> Nit: Profiles -> BrowserContexts

Done.

[huangs, 2015-03-27 19:02:37]

Ping smarth@ and dbeam@ for remaining OWNER reviews:

samarth@:
  chrome\browser\search\instant_service.cc

dbeam@:
  chrome\browser\ui\webui\fallback_icon_source.cc
  chrome\browser\ui\webui\fallback_icon_source.h
  chrome\browser\ui\webui\large_icon_source.cc
  chrome\browser\ui\webui\large_icon_source.h
  chrome\browser\ui\webui\ntp\most_visited_handler.cc

Thanks!

[Dan Beam, 2015-03-27 19:21:38]

lgtm w/nits

https://chromiumcodereview.appspot.com/996253002/diff/220001/chrome/browser/u...
File chrome/browser/ui/webui/fallback_icon_source.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/220001/chrome/browser/u...
chrome/browser/ui/webui/fallback_icon_source.cc:77: if (!fallback_icon_service_)
{
maybe document when this happens? (only in tests?)

https://chromiumcodereview.appspot.com/996253002/diff/220001/chrome/browser/u...
chrome/browser/ui/webui/fallback_icon_source.cc:95: callback.Run(nullptr);
just inline "callback.Run(nullptr);" if SendNotFoundResponse() is only used once

https://chromiumcodereview.appspot.com/996253002/diff/220001/chrome/browser/u...
File chrome/browser/ui/webui/fallback_icon_source.h (right):

https://chromiumcodereview.appspot.com/996253002/diff/220001/chrome/browser/u...
chrome/browser/ui/webui/fallback_icon_source.h:15: class FallbackIconService;
nit:

class FallbackIconService;
class GURL;

unless you consider them different enough to separate

[huangs, 2015-03-27 20:09:30]

Updated.

https://chromiumcodereview.appspot.com/996253002/diff/220001/chrome/browser/u...
File chrome/browser/ui/webui/fallback_icon_source.cc (right):

https://chromiumcodereview.appspot.com/996253002/diff/220001/chrome/browser/u...
chrome/browser/ui/webui/fallback_icon_source.cc:77: if (!fallback_icon_service_)
{
On 2015/03/27 19:21:38, Dan Beam wrote:
> maybe document when this happens? (only in tests?)

Done.

https://chromiumcodereview.appspot.com/996253002/diff/220001/chrome/browser/u...
chrome/browser/ui/webui/fallback_icon_source.cc:95: callback.Run(nullptr);
On 2015/03/27 19:21:38, Dan Beam wrote:
> just inline "callback.Run(nullptr);" if SendNotFoundResponse() is only used
once

Done.

https://chromiumcodereview.appspot.com/996253002/diff/220001/chrome/browser/u...
File chrome/browser/ui/webui/fallback_icon_source.h (right):

https://chromiumcodereview.appspot.com/996253002/diff/220001/chrome/browser/u...
chrome/browser/ui/webui/fallback_icon_source.h:15: class FallbackIconService;
On 2015/03/27 19:21:38, Dan Beam wrote:
> nit:
> 
> class FallbackIconService;
> class GURL;
> 
> unless you consider them different enough to separate

Done.

[samarth, 2015-03-27 20:18:03]

lgtm

[huangs, 2015-03-27 20:19:55]

Thanks everyone!  Will commit soon.

[huangs, 2015-03-27 20:33:54]

The CQ bit was checked by huangs@chromium.org

[huangs, 2015-03-27 20:33:55]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
beaudoin@chromium.org, rsleevi@chromium.org, pkotwicz@chromium.org,
dbeam@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/996253002/#ps240001
(title: "Removing FallbackIconSource::SendNotFoundResponse().")

[commit-bot: I haz the power, 2015-03-27 20:34:26]

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

[commit-bot: I haz the power, 2015-03-27 21:59:27]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2015-03-27 22:00:22]

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