[Favicon] Add FallbackIconStyle and FallbackIconService.

[Favicon] Add FallbackIconStyle and FallbackIconService.

Design: go/chrome-fallback-icons

This implements
- FallbackIconStyle: A struct to specify fallback data.
- FallbackIconService: Helper class to render the fallback icon as a
  filled rounded square with a single letter from the URL.
- Extra dependencies are needed to convert URL to the icon letter.

This CL is sliced off from https://codereview.chromium.org/835903005/ .

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

[huangs, 2015-01-31 00:28:19]

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

[huangs, 2015-01-31 00:28:19]

OWNER review to stevenjb@. The files are moved from
https://codereview.chromium.org/835903005/ and (so far) are unmodified. Verified
that build is successful -- the new files are not called by anyone.

[stevenjb, 2015-02-02 21:21:09]

lgtm

[huangs, 2015-02-02 23:09:55]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2015-02-02 23:10:47]

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

[commit-bot: I haz the power, 2015-02-02 23:17:30]

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

[commit-bot: I haz the power, 2015-02-02 23:17:31]

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

[huangs, 2015-02-02 23:26:14]

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

[huangs, 2015-02-02 23:26:15]

OWNER review to rsleevi@: Added dependency to 
  +net/base/registry_controlled_domains/registry_controlled_domain.h

from components/favicon_base/DEPS .

The use is in fallback_icon_service.cc: GetFallbackIconText(), which finds the
domain name to extract the first letter to render fallback icon.  E.g.,
"www.google.com" and "google.com" both yields 'g', which then becomes 'G'.

PTAL.

[Ryan Sleevi, 2015-02-03 00:23:17]

LGTM, but do not land this until you've filed an appropriate bug and added a
BUG= comment.

You should not be doing feature work without associated tracking bugs.

https://codereview.chromium.org/886163003/diff/1/components/favicon_base/fall...
File components/favicon_base/fallback_icon_service.cc (right):

https://codereview.chromium.org/886163003/diff/1/components/favicon_base/fall...
components/favicon_base/fallback_icon_service.cc:36:
base::i18n::ToUpper(base::ASCIIToUTF16(std::string(1, domain[0])));
nit: base::ASCIIToUTF16(domain.substr(0, 1));

[huangs, 2015-02-04 03:29:36]

Updated, and filed http://crbug.com/455063 to track this.  Submitting.

https://codereview.chromium.org/886163003/diff/1/components/favicon_base/fall...
File components/favicon_base/fallback_icon_service.cc (right):

https://codereview.chromium.org/886163003/diff/1/components/favicon_base/fall...
components/favicon_base/fallback_icon_service.cc:36:
base::i18n::ToUpper(base::ASCIIToUTF16(std::string(1, domain[0])));
On 2015/02/03 00:23:17, Ryan Sleevi wrote:
> nit: base::ASCIIToUTF16(domain.substr(0, 1));

Done.

[huangs, 2015-02-04 03:29:41]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2015-02-04 03:29:54]

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

[commit-bot: I haz the power, 2015-02-04 03:37:01]

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

[commit-bot: I haz the power, 2015-02-04 03:37:02]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[huangs, 2015-02-07 05:22:20]

I can fix the compile problem on Linux-gn if I add

"//chrome/app/resources:platform_locale_settings"

to BUILD.gn (in fact I did this in favicon_base.gyp).  However, adding
dependency on /chrome from /components seems illegal. A few alternatives to fix
this:

1. Get callers to inject |fallback_icon_font_list_| on constructing
FallbackIconService, so font selection code can live in Chrome.
2. Migrate IDS_SANS_SERIF_FONT_FAMILY and friends from
chrome/app/resources:platform_locale_settings to components\resources

For simplicity I'm going to do (1).

[huangs, 2015-02-09 23:26:04]

Ping stevejb@: I changed the structure a little since your LGTM:
1. FallbackIconService now takes font_list as construction parameter, so font
selection is pushed to caller in Chrome, which can access
platform_locale-settings.
2. FallackIconStyle is moved to its own file, so favicon_types.h won't have to
include Skia stuff, which causes dependency issues.

PTAL.

[huangs, 2015-02-11 04:40:55]

This is low risk and dragged on too long. Submitting.

[huangs, 2015-02-11 04:41:04]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2015-02-11 04:42:10]

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

[Ryan Sleevi, 2015-02-11 04:42:51]

On 2015/02/11 04:40:55, huangs wrote:
> This is low risk and dragged on too long. Submitting.

That is not a healthy attitude.

You should reach out to stevejb@ explicitly, or find another reviewer.

[Ryan Sleevi, 2015-02-11 04:42:58]

The CQ bit was unchecked by rsleevi@chromium.org

[huangs, 2015-02-11 05:06:21]

Ah okay, apologies.

I'll bug people tomorrow then.

[stevenjb, 2015-02-11 18:04:37]

Apologies for the delay, was busy the past couple of days.

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

https://codereview.chromium.org/886163003/diff/160001/components/favicon_base...
components/favicon_base/fallback_icon_service.cc:26: const int
kMaxFallbackFaviconSize = 288;
nit: document reason for '288'. "Arbitrary maximum icon size" is fine, but that
way in the future if there is reason to change this, the next dev will know if
it is OK (or not).

https://codereview.chromium.org/886163003/diff/160001/components/favicon_base...
components/favicon_base/fallback_icon_service.cc:82: if (!icon_text.empty()) {
nit: this could be an early exit

https://codereview.chromium.org/886163003/diff/160001/components/favicon_base...
components/favicon_base/fallback_icon_service.cc:84: if (font_size > 0) {
nit: this too

https://codereview.chromium.org/886163003/diff/160001/components/favicon_base...
File components/favicon_base/fallback_icon_style.h (right):

https://codereview.chromium.org/886163003/diff/160001/components/favicon_base...
components/favicon_base/fallback_icon_style.h:15: struct FallbackIconStyle {
From the style guide: "Use a struct only for passive objects that carry data;
everything else is a class."

[huangs, 2015-02-11 20:28:39]

Thanks. Updated, PTAL.

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

https://codereview.chromium.org/886163003/diff/160001/components/favicon_base...
components/favicon_base/fallback_icon_service.cc:26: const int
kMaxFallbackFaviconSize = 288;
On 2015/02/11 18:04:37, stevenjb wrote:
> nit: document reason for '288'. "Arbitrary maximum icon size" is fine, but
that
> way in the future if there is reason to change this, the next dev will know if
> it is OK (or not).

Done.

https://codereview.chromium.org/886163003/diff/160001/components/favicon_base...
components/favicon_base/fallback_icon_service.cc:82: if (!icon_text.empty()) {
On 2015/02/11 18:04:37, stevenjb wrote:
> nit: this could be an early exit

Done.

https://codereview.chromium.org/886163003/diff/160001/components/favicon_base...
components/favicon_base/fallback_icon_service.cc:84: if (font_size > 0) {
On 2015/02/11 18:04:37, stevenjb wrote:
> nit: this too

Done.

https://codereview.chromium.org/886163003/diff/160001/components/favicon_base...
File components/favicon_base/fallback_icon_style.h (right):

https://codereview.chromium.org/886163003/diff/160001/components/favicon_base...
components/favicon_base/fallback_icon_style.h:15: struct FallbackIconStyle {
I want the ability to directly access member variables.

Changing the member functions to helper utilities in favicon_base.

[stevenjb, 2015-02-11 21:15:58]

lgtm

https://codereview.chromium.org/886163003/diff/180001/components/favicon_base...
File components/favicon_base/fallback_icon_style.h (right):

https://codereview.chromium.org/886163003/diff/180001/components/favicon_base...
components/favicon_base/fallback_icon_style.h:38: bool
ValidateFallbackIconStyle(const FallbackIconStyle& style);
FWIW I think this would have been better as a class with accessors, but this is
fine.

[huangs, 2015-02-11 21:21:56]

Thanks!

https://codereview.chromium.org/886163003/diff/180001/components/favicon_base...
File components/favicon_base/fallback_icon_style.h (right):

https://codereview.chromium.org/886163003/diff/180001/components/favicon_base...
components/favicon_base/fallback_icon_style.h:38: bool
ValidateFallbackIconStyle(const FallbackIconStyle& style);
On 2015/02/11 21:15:58, stevenjb wrote:
> FWIW I think this would have been better as a class with accessors, but this
is
> fine.

Acknowledged.

[huangs, 2015-02-11 21:32:56]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2015-02-11 21:33:50]

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

[commit-bot: I haz the power, 2015-02-11 22:18:48]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-02-11 22:19:31]

Patchset 10 (id:??) landed as
https://crrev.com/3382fc9d60d79fc9d829fd626a4d591d2f2a23ff
Cr-Commit-Position: refs/heads/master@{#315854}