[Favicon] Adding FallbackIconUrlParser.

chrome/common/favicon/fallback_icon_url_parser.h

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_COMMON_FAVICON_FALLBACK_ICON_URL_PARSER_H_
+#define CHROME_COMMON_FAVICON_FALLBACK_ICON_URL_PARSER_H_
+
+#include <string>
+
+#include "components/favicon_base/fallback_icon_style.h"
+#include "third_party/skia/include/core/SkColor.h"

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

can you use a fwd decl instead of #include ?

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

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.

+
+namespace chrome {
+
+struct ParsedFallbackIconPath {
+  // The page URL the fallback icon is requested for.
+  std::string url;

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

we usually don't have strings doing url jobs. use GURL

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

I'm following the style of the matching file favicon_url_parser.h .  Should both
be changed then?

+
+  // The size of the requested fallback icon in pixels.
+  int size_in_pixels;

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

can you have a negative size? if not use size_t or even uint32_t.

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

Will use size_t.

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

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.

+
+  // Styling specifications of fallback icon.
+  favicon_base::FallbackIconStyle style;
+};
+
+// Parses a color in "RGB", "RRGGBB", or "RRGGBBAA" format. Returns true on
+// success.
+bool ParseIconColor(const std::string& str, SkColor* color);

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

this function seems out of place, also |str| should be more descriptive. See if
you can remove this from here.

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

How about using SkParse::FindColor()? It will change spec to require "#" for hex
colors, but also allows named color.

+
+
+// Parses |specs_str|, which should be the comma-separated value portion
+// in the format described at the top of the file
+// "chrome/browser/ui/webui/fallback_icon_source.h".
+bool ParseFallbackIconSpecs(const std::string& specs_str,
+                            int *size,
+                            favicon_base::FallbackIconStyle* style);

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

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/20 16:06:45]

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.

+
+// Parses |path|, which should be in the format described at the top of the
+// file "chrome/browser/ui/webui/fallback_icon_source.h".
+bool ParseFallbackIconPath(const std::string& path,
+                           ParsedFallbackIconPath* parsed);
+
+}  // namespace chrome
+
+#endif  // CHROME_COMMON_FAVICON_FALLBACK_ICON_URL_PARSER_H_