[Favicon] Adding FallbackIconUrlParser.

[Favicon] Adding FallbackIconUrlParser.

Design: go/chrome-fallback-icons

This adds FallbackIconUrlParser, which will be used to parse the
"explict flow" host chrome://fallback-icon/ to extract size,
FallbackIconStyle, and URL needed to render Fallback icons.

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

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

[huangs, 2015-02-13 21:42:02]

huangs@chromium.org changed reviewers:
+ jhawkins@google.com

[huangs, 2015-02-13 21:42:03]

This was sliced off bigger CL per request 2 weeks ago.  PTAL.

[huangs, 2015-02-16 17:19:09]

Ping jhawkins@.

[huangs, 2015-02-17 15:24:33]

huangs@chromium.org changed reviewers:
+ cpu@chromium.org
- jhawkins@google.com

[huangs, 2015-02-17 15:24:53]

Changing reviewer to cpu@, PTAL. Thanks!

[huangs, 2015-02-17 15:34:56]

huangs@chromium.org changed reviewers:
+ jhawkins@chromium.org
- cpu@chromium.org

[huangs, 2015-02-17 15:34:57]

Didn't know Monday was US holiday as well. Setting reviewer back to jhawkins@. 
PTAL.

[James Hawkins, 2015-02-18 18:03:41]

On 2015/02/17 15:34:57, huangs wrote:
> Didn't know Monday was US holiday as well. Setting reviewer back to jhawkins@.

> PTAL.

Note that I'm OOO till next Monday.

[huangs, 2015-02-18 18:07:06]

huangs@chromium.org changed reviewers:
+ cpu@chromium.org
- jhawkins@chromium.org

[huangs, 2015-02-18 18:07:06]

Thanks for the update!  Bugging cpu@ for this then.  PTAL.

[cpu_(ooo_6.6-7.5), 2015-02-19 03:49:19]

some comments for starters.

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
File chrome/common/favicon/fallback_icon_url_parser.cc (right):

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.cc:23: }
We don't have such helper somewhere else in our codebase?

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
File chrome/common/favicon/fallback_icon_url_parser.h (right):

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.h:11: #include
"third_party/skia/include/core/SkColor.h"
can you use a fwd decl instead of #include ?

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.h:17: std::string url;
we usually don't have strings doing url jobs. use GURL

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.h:20: int size_in_pixels;
can you have a negative size? if not use size_t or even uint32_t.

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.h:28: bool ParseIconColor(const
std::string& str, SkColor* color);
this function seems out of place, also |str| should be more descriptive. See if
you can remove this from here.

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.h:36:
favicon_base::FallbackIconStyle* style);
you seem to be returning 2 of the 3 items in ParsedFallbackIconPath, so rather
use that

or

make the next method not use a struct.

[huangs, 2015-02-19 05:54:05]

Thanks!  My internet is not working well Wednesday and Thursday (OOO) to
implement this, but I replied with planned changes + questions.  PTAL.

[huangs, 2015-02-20 16:06:45]

Ugh, forgot to send out the planned changes. Implementing these soon.

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
File chrome/common/favicon/fallback_icon_url_parser.cc (right):

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.cc:23: }
Moot if I use SkParse::FindColor().

Meanwhile, I didn't find one a convenient one to use at first, but searching
again (for "<= 'F'") I found templates in string_util.h: IsHexDigit() and
HexDigitToInt().

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
File chrome/common/favicon/fallback_icon_url_parser.h (right):

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.h:11: #include
"third_party/skia/include/core/SkColor.h"
SkColor is a typedef:

typedef uint32_t SkColor;

So forward declare (with class/struct/typedef) won't work.  I checked and
/ui/views/controls/slider.h embeds "typedef unsigned int SkColor;" directly,
which seems rather yucky.

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.h:17: std::string url;
I'm following the style of the matching file favicon_url_parser.h .  Should both
be changed then?

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.h:20: int size_in_pixels;
Will use size_t.

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.h:28: bool ParseIconColor(const
std::string& str, SkColor* color);
How about using SkParse::FindColor()? It will change spec to require "#" for hex
colors, but also allows named color.

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.h:36:
favicon_base::FallbackIconStyle* style);
On 2015/02/19 03:49:18, cpu wrote:
> you seem to be returning 2 of the 3 items in ParsedFallbackIconPath, so rather
> use that
> 
> or
> 
> make the next method not use a struct.

Will take the latter case.  I suppose specific unit tests don't have to call the
specialized routine, but just stub out "known good" params for params it doesn't
care about.

[huangs, 2015-02-20 20:39:32]

+sugoi@ for OWNER review of Skia changes. I'm using Skia's color parser, so they
should not be exclude from skia_library any more.

PTAL.

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
File chrome/common/favicon/fallback_icon_url_parser.h (right):

https://codereview.chromium.org/924063003/diff/20001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.h:20: int size_in_pixels;
On second thought, this is int because all favicon code uses int for sizes.  If
we were to change this it should be done in one fell swoop.

[huangs, 2015-02-23 18:30:19]

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

[huangs, 2015-02-23 18:30:19]

Oops forgot to add sugoi@ for OWN review of Skia build files.

Also pinging cpu@ for review of otherfiles.

[sugoi1, 2015-02-23 19:00:51]

On 2015/02/23 18:30:19, huangs wrote:
> Oops forgot to add sugoi@ for OWN review of Skia build files.
> 
> Also pinging cpu@ for review of otherfiles.

LGTM for skia files

[cpu_(ooo_6.6-7.5), 2015-02-24 18:42:22]

https://codereview.chromium.org/924063003/diff/40001/chrome/common/favicon/fa...
File chrome/common/favicon/fallback_icon_url_parser.cc (right):

https://codereview.chromium.org/924063003/diff/40001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.cc:16: :
size_in_pixels_(gfx::kFaviconSize) {
I think indentation here is 4 spaces.

https://codereview.chromium.org/924063003/diff/40001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.cc:34: std::string url_str =
path.substr(slash + 1);
what if slash is the last character?

https://codereview.chromium.org/924063003/diff/40001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.cc:36: return url_str.empty() ||
url_.is_valid();  // Allow mepty URL.
typo in comment.

[huangs, 2015-02-24 19:30:33]

Updated, PTAL (sorry presubmit was slow).

https://codereview.chromium.org/924063003/diff/40001/chrome/common/favicon/fa...
File chrome/common/favicon/fallback_icon_url_parser.cc (right):

https://codereview.chromium.org/924063003/diff/40001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.cc:16: :
size_in_pixels_(gfx::kFaviconSize) {
On 2015/02/24 18:42:22, cpu wrote:
> I think indentation here is 4 spaces.

Done.

https://codereview.chromium.org/924063003/diff/40001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.cc:34: std::string url_str =
path.substr(slash + 1);
We'd get path.substr(length), which by definition is empty string. Then we'd get
empty url_, which is allowed.  Adding comment though.

https://codereview.chromium.org/924063003/diff/40001/chrome/common/favicon/fa...
chrome/common/favicon/fallback_icon_url_parser.cc:36: return url_str.empty() ||
url_.is_valid();  // Allow mepty URL.
On 2015/02/24 18:42:22, cpu wrote:
> typo in comment.

Done.

[cpu_(ooo_6.6-7.5), 2015-02-24 20:27:18]

lgtm

[huangs, 2015-02-24 20:29:47]

On 2015/02/24 20:27:18, cpu wrote:
> lgtm

Thanks! Submitting.

[huangs, 2015-02-24 20:29:51]

The CQ bit was checked by huangs@chromium.org

[huangs, 2015-02-24 20:29:52]

The patchset sent to the CQ was uploaded after l-g-t-m from sugoi@chromium.org
Link to the patchset: https://codereview.chromium.org/924063003/#ps60001 (title:
"Fix spacing and comments.")

[commit-bot: I haz the power, 2015-02-24 20:31:12]

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

[commit-bot: I haz the power, 2015-02-24 22:13:28]

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

[commit-bot: I haz the power, 2015-02-24 22:13:30]

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

[huangs, 2015-03-01 22:54:28]

Was busy last few days dealing with unrelated IME blocking issue.

Just determined cause of failure: This was a component build issue. To fix this
we have to add SK_API:

class SK_API SkParse {...}

Probably should make this a separate CL.

[huangs, 2015-03-01 23:01:56]

May as well make it same CL, good for context as well.

[huangs, 2015-03-01 23:08:03]

Not sure that's even possible.  Created
https://codereview.chromium.org/971623003 .

[huangs, 2015-03-02 20:01:09]

The CQ bit was checked by huangs@chromium.org

[huangs, 2015-03-02 20:01:10]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
sugoi@chromium.org
Link to the patchset: https://codereview.chromium.org/924063003/#ps140001
(title: "Sync, to include the fix from https://codereview.chromium.org/971623003
.")

[commit-bot: I haz the power, 2015-03-02 20:01:52]

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

[commit-bot: I haz the power, 2015-03-02 21:52:09]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-02 21:52:44]

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