Supporting high dpi favicons in Instant Extended.

Preparation for supporting high dpi favicons in Instant Extended.

The logic in FaviconSource::ParsePath() has been moved from chrome/browser/ui/* to chrome/common/favicon* allowing it to be used from the renderer. Also, chrome/browser/favicon/favicon_types.h to chrome/common/favicon/favicon_types.h for the same reason.

Related internal CL: cl/46805180

To test this CL:
1) git cl patch 15388002
2) ./build/gyp_chromium
2) compile chrome
3) run chrome with --enable-instant-extended-api


BUG=227087
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211085

[pedro (no code reviews), 2013-05-23 00:05:20]

Hey Sreeram, this CL is almost ready, but I wanted to ask you for a first look
to make sure I'm not doing something terribly wrong. :)

I'm working on the tests now, though I'm not sure how to pass a WebFrame to
ChromeContentRendererClient::WillSendRequest() inside the test.

[sreeram, 2013-06-04 22:01:56]

Generally looks good.

Not sure about how to add a unittest. If it's hard to figure out how to add it
in chrome_content_renderer_client_unittest.cc, see if you can add tests in a
level higher up (i.e., something that ends up calling WillSendRequest), e.g.:
content/renderer/render_view_impl_unittest.cc.

https://codereview.chromium.org/15388002/diff/5001/chrome/browser/bookmarks/b...
File chrome/browser/bookmarks/bookmark_html_writer.cc (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/browser/bookmarks/b...
chrome/browser/bookmarks/bookmark_html_writer.cc:24: #include
"chrome/common/favicon_types.h"
You don't need this include (here and in many other files in this CL).
"chrome/browser/favicon/favicon_types.h" (included above) includes
"chrome/common/favicon_types.h".

https://codereview.chromium.org/15388002/diff/5001/chrome/browser/search/inst...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/browser/search/inst...
chrome/browser/search/instant_service.cc:103: }
Why not just delete this method entirely?

https://codereview.chromium.org/15388002/diff/5001/chrome/browser/ui/webui/fa...
File chrome/browser/ui/webui/favicon_source.h (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/browser/ui/webui/fa...
chrome/browser/ui/webui/favicon_source.h:121: // the file. Returns true if
|raw_path| could be parsed.
Change "raw_path" to "path" in both lines above.

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_types.h
File chrome/common/favicon_types.h (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_type...
chrome/common/favicon_types.h:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
No "(c)".

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
File chrome/common/favicon_url_parser.cc (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
No "(c)".

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.cc:17: // description of what each does.
The .h file doesn't actually give any description. I know you're just moving
this comment, but could you also update it?

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.cc:41: // TODO make url string
Remove this?

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.cc:50: std::string* params) {
Same alignment issue as in the .h file.

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.cc:54: *params = "";
Move this to below line 57, to keep the same order as in the argument list.

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
File chrome/common/favicon_url_parser.h (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
No "(c)".

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.h:8: #include "ui/webui/web_ui_util.h"
Remove this include. Instead, add the following:

#include <string>

#include "ui/base/layout.h"

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.h:14: // could be parsed.
Please also document |icon_types|, |supports_origin_parameter| and |params|.

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.h:22: std::string* params);
Nit: Lines 16-22 should each be indented by one more space.

https://codereview.chromium.org/15388002/diff/5001/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1004: (url.host() == "thumb"
|| url.host() == "favicon")) {
Use kChromeUIThumbnailHost, kChromeUIFaviconHost and kChromeUITouchIconHost
instead.

Ideally, we should use Favicon::GetSource() and ThumbnailSource::GetSource(),
but we can't get at those methods from here in the renderer.

https://codereview.chromium.org/15388002/diff/5001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1023: // |favicon_url| which
it is expected to be the id of a Most Visited
which it is -> which is

https://codereview.chromium.org/15388002/diff/5001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1025: // favicon is being
requested).
This comment is kinda superfluous. Just delete it.

https://codereview.chromium.org/15388002/diff/5001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1043: item_url.spec();
Do you need to URL-escape the item_url?

https://codereview.chromium.org/15388002/diff/5001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox_api.js:336: item.faviconUrl =
GenerateFaviconUrl(item.rid);
These Shadow DOM elements are going to be removed soon. Given that, I don't see
the point of adding this here. You might as well just add this to the
local_ntp.js or wherever you need to compute it (given that there's no secret
ingredient anyway).

[pedro (no code reviews), 2013-06-07 23:34:21]

FYI, I'll wait for @kmadhusu CL to land first. Then I'll rebase this CL again
and will send it for another round of comments.

So, please wait for my next comment before reviewing this CL. Thanks!

https://codereview.chromium.org/15388002/diff/5001/chrome/browser/bookmarks/b...
File chrome/browser/bookmarks/bookmark_html_writer.cc (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/browser/bookmarks/b...
chrome/browser/bookmarks/bookmark_html_writer.cc:24: #include
"chrome/common/favicon_types.h"
On 2013/06/04 22:01:56, sreeram wrote:
> You don't need this include (here and in many other files in this CL).
> "chrome/browser/favicon/favicon_types.h" (included above) includes
> "chrome/common/favicon_types.h".

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/browser/search/inst...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/browser/search/inst...
chrome/browser/search/instant_service.cc:103: }
On 2013/06/04 22:01:56, sreeram wrote:
> Why not just delete this method entirely?

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/browser/ui/webui/fa...
File chrome/browser/ui/webui/favicon_source.h (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/browser/ui/webui/fa...
chrome/browser/ui/webui/favicon_source.h:121: // the file. Returns true if
|raw_path| could be parsed.
On 2013/06/04 22:01:56, sreeram wrote:
> Change "raw_path" to "path" in both lines above.

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_types.h
File chrome/common/favicon_types.h (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_type...
chrome/common/favicon_types.h:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
On 2013/06/04 22:01:56, sreeram wrote:
> No "(c)".

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
File chrome/common/favicon_url_parser.cc (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/06/04 22:01:56, sreeram wrote:
> No "(c)".

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.cc:17: // description of what each does.
On 2013/06/04 22:01:56, sreeram wrote:
> The .h file doesn't actually give any description. I know you're just moving
> this comment, but could you also update it?

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.cc:41: // TODO make url string
On 2013/06/04 22:01:56, sreeram wrote:
> Remove this?

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.cc:50: std::string* params) {
On 2013/06/04 22:01:56, sreeram wrote:
> Same alignment issue as in the .h file.

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.cc:54: *params = "";
On 2013/06/04 22:01:56, sreeram wrote:
> Move this to below line 57, to keep the same order as in the argument list.

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
File chrome/common/favicon_url_parser.h (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/06/04 22:01:56, sreeram wrote:
> No "(c)".

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.h:8: #include "ui/webui/web_ui_util.h"
On 2013/06/04 22:01:56, sreeram wrote:
> Remove this include. Instead, add the following:
> 
> #include <string>
> 
> #include "ui/base/layout.h"

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.h:14: // could be parsed.
On 2013/06/04 22:01:56, sreeram wrote:
> Please also document |icon_types|, |supports_origin_parameter| and |params|.

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/common/favicon_url_...
chrome/common/favicon_url_parser.h:22: std::string* params);
On 2013/06/04 22:01:56, sreeram wrote:
> Nit: Lines 16-22 should each be indented by one more space.

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1004: (url.host() == "thumb"
|| url.host() == "favicon")) {
On 2013/06/04 22:01:56, sreeram wrote:
> Use kChromeUIThumbnailHost, kChromeUIFaviconHost and kChromeUITouchIconHost
> instead.
> 
> Ideally, we should use Favicon::GetSource() and ThumbnailSource::GetSource(),
> but we can't get at those methods from here in the renderer.

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1023: // |favicon_url| which
it is expected to be the id of a Most Visited
On 2013/06/04 22:01:56, sreeram wrote:
> which it is -> which is

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1025: // favicon is being
requested).
On 2013/06/04 22:01:56, sreeram wrote:
> This comment is kinda superfluous. Just delete it.

Done.

https://codereview.chromium.org/15388002/diff/5001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1043: item_url.spec();
On 2013/06/04 22:01:56, sreeram wrote:
> Do you need to URL-escape the item_url?

Good question. I'm not sure if we need to do that.

https://codereview.chromium.org/15388002/diff/5001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/15388002/diff/5001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox_api.js:336: item.faviconUrl =
GenerateFaviconUrl(item.rid);
On 2013/06/04 22:01:56, sreeram wrote:
> These Shadow DOM elements are going to be removed soon. Given that, I don't
see
> the point of adding this here. You might as well just add this to the
> local_ntp.js or wherever you need to compute it (given that there's no secret
> ingredient anyway).

The faviconUrl will be used as the background image of a normal div, not a
Shadow DOM one, so I think it makes sense putting this code in here. By doing so
we'll make both the live and local NTPs to support high dpi favicons.

[sreeram, 2013-06-10 18:09:46]

Removing myself as reviewer, as I no longer work on Instant/Extended.

[pedro (no code reviews), 2013-06-14 00:22:34]

Hey Samarth, could you please take a look at this CL?

[samarth, 2013-06-14 21:54:46]

Not really sure an interactive UI test is all that meaningful here. It's more
important to test the finicky parsing code.

Thanks,
Samarth

https://codereview.chromium.org/15388002/diff/21001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_types.h (right):

https://codereview.chromium.org/15388002/diff/21001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_types.h:9: #include
"chrome/common/favicon_types.h"
This is up to the favicon OWNERS but I'd prefer a single place (in
chrome/common) to have all the types rather than being split across multiple
files.

https://codereview.chromium.org/15388002/diff/21001/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/15388002/diff/21001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/favicon_source.cc:151: bool
FaviconSource::ParsePath(const std::string& path,
Doesn't seem like much point for this function to continue to exist.

https://codereview.chromium.org/15388002/diff/21001/chrome/common/favicon_url...
File chrome/common/favicon_url_parser.cc (right):

https://codereview.chromium.org/15388002/diff/21001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.cc:42: bool ParseFaviconPath(const std::string&
path,
Yikes! Are there no tests for this?  Please add some unit tests.  It should be
simple enough.

https://codereview.chromium.org/15388002/diff/21001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.cc:45: bool* is_icon_url,
Rather than taking in a slew of output params, how about a ParsedFaviconURL
struct that has the output?

https://codereview.chromium.org/15388002/diff/21001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.cc:117: // The format of favicon URLs used in
Instant Extended does not support
We don't use it today. What's wrong with leaving support in?

This function is complex and does a ton of stuff already. Add yet another switch
to its behavior just adds more complexity.

https://codereview.chromium.org/15388002/diff/21001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.cc:137: // The favicon parameters need to be
returned in order to allow Instant
I find this confusing. I think it would be more natural to return how much of
the URL was actually consumed. The caller can use that value intelligently.

https://codereview.chromium.org/15388002/diff/21001/chrome/common/favicon_url...
File chrome/common/favicon_url_parser.h (right):

https://codereview.chromium.org/15388002/diff/21001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.h:15: // file
"chrome/browser/ui/webui/favicon_source.h". Returns true if |path|
Move the definition of the format here.

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/resources...
chrome/renderer/resources/extensions/searchbox_api.js:271: function
UpdateFaviconUrl(faviconUrl) {
Don't try to parse this URL in javascript.  Instead, just pass in the relevant
fields (rendererId, mostVisitedId) and construct the URL here.

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:216: bool
SearchBox::GenerateFaviconURLFromTransientURL(const GURL& transient_url,
Please update the docs in the .h for this.

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:218: // TODO(pedrosimonetti): DO NOT
SUBMIT
Take out debugging info

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:222: std::string raw_path =
transient_url.path().substr(1);
Don't just assume: please add checks that the path is non-empty and that the
first character is /. Probably makes sense to be a DCHECK.

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:225: int size_in_dip = 16;
This should be set by ParseFaviconPath, no? Don't define a new constant either
(either initialize to 0/-1 or some common constant).

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:230: &is_icon_url, &favicon_url,
&size_in_dip, &scale_factor, &params);
Just return false here if success is not true. Or better yet, just wrap this
call in an if. I don't see the need to rewrite to chrome-search://invalid.

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:230: &is_icon_url, &favicon_url,
&size_in_dip, &scale_factor, &params);
If you follow my comment from the parse and return how much of the URL string
was parsed, the rest of this function will be more straightforward.  You just
need to pass the remainder of the string to the helper function to get a URL and
then join the first part of the string with the second part.

[pedro (no code reviews), 2013-06-18 22:35:13]

Hey Samarth,

Please take another look. I've refactored the code in order to make it easier to
create unit tests. I've also included unit tests for the favicon URL parsing.

https://codereview.chromium.org/15388002/diff/21001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_types.h (right):

https://codereview.chromium.org/15388002/diff/21001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_types.h:9: #include
"chrome/common/favicon_types.h"
On 2013/06/14 21:54:46, samarth wrote:
> This is up to the favicon OWNERS but I'd prefer a single place (in
> chrome/common) to have all the types rather than being split across multiple
> files.

That's a good point, but as we discussed offline, I'll wait for the OWNERS
review before doing this change.

https://codereview.chromium.org/15388002/diff/21001/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/15388002/diff/21001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/favicon_source.cc:151: bool
FaviconSource::ParsePath(const std::string& path,
On 2013/06/14 21:54:46, samarth wrote:
> Doesn't seem like much point for this function to continue to exist.

Good point. Removed.

https://codereview.chromium.org/15388002/diff/21001/chrome/common/favicon_url...
File chrome/common/favicon_url_parser.cc (right):

https://codereview.chromium.org/15388002/diff/21001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.cc:42: bool ParseFaviconPath(const std::string&
path,
On 2013/06/14 21:54:46, samarth wrote:
> Yikes! Are there no tests for this?  Please add some unit tests.  It should be
> simple enough.

Done.

https://codereview.chromium.org/15388002/diff/21001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.cc:45: bool* is_icon_url,
On 2013/06/14 21:54:46, samarth wrote:
> Rather than taking in a slew of output params, how about a ParsedFaviconURL
> struct that has the output?

I wasn't happy with it too. Thanks for the suggestion. It really makes the code
more readable!

https://codereview.chromium.org/15388002/diff/21001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.cc:117: // The format of favicon URLs used in
Instant Extended does not support
On 2013/06/14 21:54:46, samarth wrote:
> We don't use it today. What's wrong with leaving support in?
> 
> This function is complex and does a ton of stuff already. Add yet another
switch
> to its behavior just adds more complexity.

Removed the |supports_origin_parameter|.

https://codereview.chromium.org/15388002/diff/21001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.cc:137: // The favicon parameters need to be
returned in order to allow Instant
On 2013/06/14 21:54:46, samarth wrote:
> I find this confusing. I think it would be more natural to return how much of
> the URL was actually consumed. The caller can use that value intelligently.

Done.

https://codereview.chromium.org/15388002/diff/21001/chrome/common/favicon_url...
File chrome/common/favicon_url_parser.h (right):

https://codereview.chromium.org/15388002/diff/21001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.h:15: // file
"chrome/browser/ui/webui/favicon_source.h". Returns true if |path|
On 2013/06/14 21:54:46, samarth wrote:
> Move the definition of the format here.

What do you mean? Do you mean moving the comment that describes favicon
parameters?

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/resources...
chrome/renderer/resources/extensions/searchbox_api.js:271: function
UpdateFaviconUrl(faviconUrl) {
On 2013/06/14 21:54:46, samarth wrote:
> Don't try to parse this URL in javascript.  Instead, just pass in the relevant
> fields (rendererId, mostVisitedId) and construct the URL here.

Done.

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:216: bool
SearchBox::GenerateFaviconURLFromTransientURL(const GURL& transient_url,
On 2013/06/14 21:54:46, samarth wrote:
> Please update the docs in the .h for this.

Done.

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:218: // TODO(pedrosimonetti): DO NOT
SUBMIT
On 2013/06/14 21:54:46, samarth wrote:
> Take out debugging info

Done. I left debugging info in there to make it easier to test while the CL is
being reviewed. That's why I've used "DO NOT SUBMIT". Log messages removed
anyway.

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:222: std::string raw_path =
transient_url.path().substr(1);
On 2013/06/14 21:54:46, samarth wrote:
> Don't just assume: please add checks that the path is non-empty and that the
> first character is /. Probably makes sense to be a DCHECK.

Done.

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:225: int size_in_dip = 16;
On 2013/06/14 21:54:46, samarth wrote:
> This should be set by ParseFaviconPath, no? Don't define a new constant either
> (either initialize to 0/-1 or some common constant).

I'm using the struct now.

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:230: &is_icon_url, &favicon_url,
&size_in_dip, &scale_factor, &params);
On 2013/06/14 21:54:46, samarth wrote:
> Just return false here if success is not true. Or better yet, just wrap this
> call in an if. I don't see the need to rewrite to chrome-search://invalid.

Done.

https://codereview.chromium.org/15388002/diff/21001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:230: &is_icon_url, &favicon_url,
&size_in_dip, &scale_factor, &params);
On 2013/06/14 21:54:46, samarth wrote:
> If you follow my comment from the parse and return how much of the URL string
> was parsed, the rest of this function will be more straightforward.  You just
> need to pass the remainder of the string to the helper function to get a URL
and
> then join the first part of the string with the second part.

Done.

[samarth, 2013-06-20 00:31:29]

A few more comments, but overall it's looking good.  You probably should loop in
favicon OWNERS to get an OK for your general approach.

Thanks,
Samarth

https://codereview.chromium.org/15388002/diff/30001/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/15388002/diff/30001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/favicon_source.cc:77: if (!success) {
nit: move this to right after the Parse call.  url isn't used later.

https://codereview.chromium.org/15388002/diff/30001/chrome/common/favicon_url...
File chrome/common/favicon_url_parser.h (right):

https://codereview.chromium.org/15388002/diff/30001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.h:14: struct ParsedFaviconPath {
Document the fields here, rather than below.

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/resources...
chrome/renderer/resources/extensions/searchbox_api.js:268: item.renderViewId =
null;
Might as well also delete item.rid since that's not part of the public api

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/resources...
chrome/renderer/resources/extensions/searchbox_api.js:273: function
UpdateFaviconUrl(renderViewId, rid) {
CreateFaviconUrl or ConstructFaviconUrl

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:65: // In Instand Extended, the part of
the favicon URL which is supposed to
No need to keep mentioning Instant Extended (especially in this file, which is
all about Instant Extended).  Just say: "The part of ..." and likewise
elsewhere.

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:77: // the restricted_id with the actuall
URL from which the favicon is being
sp. actuall

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:87: // Valid |url| forms:
Update this comment since this is only expected to be used for
chrome-search://thumb

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:93: bool
GetInstantRestrictedIDFromURL(int render_view_id,
Please rename this function and the one above to be parallel.  Something like
GetRestrictedIDFrom{Favicon|Thumb}URL or the like.

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:257: bool success =
internal::ParseRestrictedFaviconUrl(
If the favicon URL parser can't parse this URL, we should just return false here
(which means, we won't try to rewrite the URL).

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:267: *url =
GURL("chrome-search://favicon/");
This should keep favicon_params part of the string.  That way, you get the 2x
version of the default favicon when appropriate.  (I would just construct the
URL in one place and make sure the item_url string is empty in the cases where
we could convert it properly.)

[pedro (no code reviews), 2013-06-20 22:43:45]

https://codereview.chromium.org/15388002/diff/30001/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/15388002/diff/30001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/favicon_source.cc:77: if (!success) {
On 2013/06/20 00:31:29, samarth wrote:
> nit: move this to right after the Parse call.  url isn't used later.

Done.

https://codereview.chromium.org/15388002/diff/30001/chrome/common/favicon_url...
File chrome/common/favicon_url_parser.h (right):

https://codereview.chromium.org/15388002/diff/30001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.h:14: struct ParsedFaviconPath {
On 2013/06/20 00:31:29, samarth wrote:
> Document the fields here, rather than below.

Done.

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/resources...
chrome/renderer/resources/extensions/searchbox_api.js:268: item.renderViewId =
null;
On 2013/06/20 00:31:29, samarth wrote:
> Might as well also delete item.rid since that's not part of the public api

As discussed offline, we actually need the rid, so I'm keeping it.

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/resources...
chrome/renderer/resources/extensions/searchbox_api.js:273: function
UpdateFaviconUrl(renderViewId, rid) {
On 2013/06/20 00:31:29, samarth wrote:
> CreateFaviconUrl or ConstructFaviconUrl

Renamed to GenerateThumbnailURL to keep consistency with the names used in
searchbox.cc

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:65: // In Instand Extended, the part of
the favicon URL which is supposed to
On 2013/06/20 00:31:29, samarth wrote:
> No need to keep mentioning Instant Extended (especially in this file, which is
> all about Instant Extended).  Just say: "The part of ..." and likewise
> elsewhere.

Done.

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:77: // the restricted_id with the actuall
URL from which the favicon is being
On 2013/06/20 00:31:29, samarth wrote:
> sp. actuall

Done.

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:87: // Valid |url| forms:
On 2013/06/20 00:31:29, samarth wrote:
> Update this comment since this is only expected to be used for
> chrome-search://thumb

Done.

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:93: bool
GetInstantRestrictedIDFromURL(int render_view_id,
On 2013/06/20 00:31:29, samarth wrote:
> Please rename this function and the one above to be parallel.  Something like
> GetRestrictedIDFrom{Favicon|Thumb}URL or the like.

Done.

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:257: bool success =
internal::ParseRestrictedFaviconUrl(
On 2013/06/20 00:31:29, samarth wrote:
> If the favicon URL parser can't parse this URL, we should just return false
here
> (which means, we won't try to rewrite the URL).

Done.

https://codereview.chromium.org/15388002/diff/30001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:267: *url =
GURL("chrome-search://favicon/");
On 2013/06/20 00:31:29, samarth wrote:
> This should keep favicon_params part of the string.  That way, you get the 2x
> version of the default favicon when appropriate.  (I would just construct the
> URL in one place and make sure the item_url string is empty in the cases where
> we could convert it properly.)

Done.

[pedro (no code reviews), 2013-06-20 22:44:48]

Hey Evan, could you please take a look at this CL?

[Evan Stade, 2013-06-21 19:08:24]

On 2013/06/20 22:44:48, pedrosimonetti wrote:
> Hey Evan, could you please take a look at this CL?

any particular part?

[pedro (no code reviews), 2013-06-21 20:22:50]

On 2013/06/21 19:08:24, Evan Stade wrote:
> On 2013/06/20 22:44:48, pedrosimonetti wrote:
> > Hey Evan, could you please take a look at this CL?
> 
> any particular part?

I'm moving the favicon parsing logic from chrome/browser to chrome/common, so it
can be used from chrome/renderer. So I'm mostly interested in your review for
this parts:
chrome/browser/favicon/favicon_types.h
chrome/browser/ui/webui/*
chrome/common/*

I'm not sure who to ask for approval in chrome/common directory though. Do you
have approval powers for this directory?

[Evan Stade, 2013-06-24 18:56:23]

no, I don't have OWNER control of chrome/common. Looks like you should look at
chrome/OWNERS for approval of the refactor

https://codereview.chromium.org/15388002/diff/38001/chrome/common/favicon_url...
File chrome/common/favicon_url_parser.cc (right):

https://codereview.chromium.org/15388002/diff/38001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.cc:35: return (path.compare(start_index,
search.size(), search) == 0);
nit: no parens

https://codereview.chromium.org/15388002/diff/38001/chrome/common/favicon_url...
File chrome/common/favicon_url_parser.h (right):

https://codereview.chromium.org/15388002/diff/38001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.h:18: std::string url;
why is this not a GURL?

[pedro (no code reviews), 2013-06-27 21:26:30]

Thanks Evan.

https://codereview.chromium.org/15388002/diff/38001/chrome/common/favicon_url...
File chrome/common/favicon_url_parser.cc (right):

https://codereview.chromium.org/15388002/diff/38001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.cc:35: return (path.compare(start_index,
search.size(), search) == 0);
On 2013/06/24 18:56:23, Evan Stade wrote:
> nit: no parens

Done.

https://codereview.chromium.org/15388002/diff/38001/chrome/common/favicon_url...
File chrome/common/favicon_url_parser.h (right):

https://codereview.chromium.org/15388002/diff/38001/chrome/common/favicon_url...
chrome/common/favicon_url_parser.h:18: std::string url;
On 2013/06/24 18:56:23, Evan Stade wrote:
> why is this not a GURL?

Favicons URLs in Instant Extended do not have the URL part. Instead, they have a
pair of ids (renderer view id, and most visited id), like this:
chrome-search://favicon/size/16@2x/123/456

So, we're not converting that last part of the Favicon URL because it won't be
always in a URL-like format.

[pedro (no code reviews), 2013-06-27 21:36:17]

@James: could you review chrome/common/* changes?

@Steven: could you review chrome/browser/favicon/favicon_types.h?

[James Hawkins, 2013-06-27 21:44:32]

https://codereview.chromium.org/15388002/diff/48001/chrome/common/favicon_typ...
File chrome/common/favicon_types.h (right):

https://codereview.chromium.org/15388002/diff/48001/chrome/common/favicon_typ...
chrome/common/favicon_types.h:10: // Defines the icon types. They are also
stored in icon_type field of favicons
Why is this in chrome/common?

[stevenjb, 2013-06-27 22:06:30]

It seems like we should add chrome/common/favicon/ for these. Even though it's
only 3 files, chrome/common is already pretty huge.

[pedro (no code reviews), 2013-06-27 23:02:47]

@Steven: That's a good idea. I'll move those files to chrome/common/favicon
then.

https://codereview.chromium.org/15388002/diff/48001/chrome/common/favicon_typ...
File chrome/common/favicon_types.h (right):

https://codereview.chromium.org/15388002/diff/48001/chrome/common/favicon_typ...
chrome/common/favicon_types.h:10: // Defines the icon types. They are also
stored in icon_type field of favicons
On 2013/06/27 21:44:32, James Hawkins wrote:
> Why is this in chrome/common?

This used to be in chrome/browser/favicon, but we need to access it from
chrome/renderer/searchbox in order to support hi dpi favicons in Instant
Extended.

Samarth suggested moving the entire chrome/browser/favicon/favicon_types.h file
to chrome/common (or chrome/common/favicon as Steven suggested) so all favicon
related types would be in a single place. What do you think?

[James Hawkins, 2013-06-27 23:04:40]

https://codereview.chromium.org/15388002/diff/48001/chrome/common/favicon_typ...
File chrome/common/favicon_types.h (right):

https://codereview.chromium.org/15388002/diff/48001/chrome/common/favicon_typ...
chrome/common/favicon_types.h:10: // Defines the icon types. They are also
stored in icon_type field of favicons
On 2013/06/27 23:02:47, pedrosimonetti wrote:
> On 2013/06/27 21:44:32, James Hawkins wrote:
> > Why is this in chrome/common?
> 
> This used to be in chrome/browser/favicon, but we need to access it from
> chrome/renderer/searchbox in order to support hi dpi favicons in Instant
> Extended.
> 
> Samarth suggested moving the entire chrome/browser/favicon/favicon_types.h
file
> to chrome/common (or chrome/common/favicon as Steven suggested) so all favicon
> related types would be in a single place. What do you think?

That's fine.

[pedro (no code reviews), 2013-06-28 17:38:56]

Okay, I've moved the common favicon code to chrome/common/favicon. Please take
another look.

Also. I had to change chrome/browser/favicon/DEPS. Is this change correct?

[stevenjb, 2013-06-28 18:23:59]

I didn't look at the searchbox.cc changes (they really ought to be done in a
separate CL) but the favicon changes LGTM.

https://codereview.chromium.org/15388002/diff/57001/chrome/common/favicon/fav...
File chrome/common/favicon/favicon_url_parser.cc (right):

https://codereview.chromium.org/15388002/diff/57001/chrome/common/favicon/fav...
chrome/common/favicon/favicon_url_parser.cc:28: const std::string& search) {
This can just be:

path.compare(start_index, search.length(), search) == 0

since 'search' is never empty, and compare will fail if it reaches the end of
the string.

https://codereview.chromium.org/15388002/diff/57001/chrome/common/favicon/fav...
chrome/common/favicon/favicon_url_parser.cc:45: DCHECK_EQ(16,
gfx::kFaviconSize);
Shouldn't we make this code use gfx::kFaviconSize and define the constants
throughout based on that? Maybe not in this CL, I just don't like seeing hard
coded constants being used.

[pedro (no code reviews), 2013-06-28 22:38:42]

Okay, I've addressed the comments and removed searchbox related code (to be
submitted in another CL).

Please take another look.

https://codereview.chromium.org/15388002/diff/57001/chrome/common/favicon/fav...
File chrome/common/favicon/favicon_url_parser.cc (right):

https://codereview.chromium.org/15388002/diff/57001/chrome/common/favicon/fav...
chrome/common/favicon/favicon_url_parser.cc:28: const std::string& search) {
On 2013/06/28 18:24:00, stevenjb (chromium) wrote:
> This can just be:
> 
> path.compare(start_index, search.length(), search) == 0
> 
> since 'search' is never empty, and compare will fail if it reaches the end of
> the string.

Done.

https://codereview.chromium.org/15388002/diff/57001/chrome/common/favicon/fav...
chrome/common/favicon/favicon_url_parser.cc:45: DCHECK_EQ(16,
gfx::kFaviconSize);
On 2013/06/28 18:24:00, stevenjb (chromium) wrote:
> Shouldn't we make this code use gfx::kFaviconSize and define the constants
> throughout based on that? Maybe not in this CL, I just don't like seeing hard
> coded constants being used.

Done.

[pedro (no code reviews), 2013-06-28 22:40:43]

@sky: could you review changes in ui/gfx/*?

[sky, 2013-07-01 13:47:59]

https://codereview.chromium.org/15388002/diff/62001/ui/gfx/favicon_size.h
File ui/gfx/favicon_size.h (right):

https://codereview.chromium.org/15388002/diff/62001/ui/gfx/favicon_size.h#new...
ui/gfx/favicon_size.h:16: UI_EXPORT extern const int kSizeInDip;
AFAICT all of these are only used in one place. Why do we need them here?
Additionally kSizeInDip is not descriptive. Size of what?

[pedro (no code reviews), 2013-07-01 22:33:29]

https://codereview.chromium.org/15388002/diff/62001/ui/gfx/favicon_size.h
File ui/gfx/favicon_size.h (right):

https://codereview.chromium.org/15388002/diff/62001/ui/gfx/favicon_size.h#new...
ui/gfx/favicon_size.h:16: UI_EXPORT extern const int kSizeInDip;
On 2013/07/01 13:48:00, sky wrote:
> AFAICT all of these are only used in one place. Why do we need them here?
> Additionally kSizeInDip is not descriptive. Size of what?

I moved this constants to where they're being used, and renamed them to
something more descriptive.

[sky, 2013-07-02 00:25:15]

That means you don't need me to review anymore, right?

[pedro (no code reviews), 2013-07-02 16:43:38]

On 2013/07/02 00:25:15, sky wrote:
> That means you don't need me to review anymore, right?

@Sky: right, thanks for the review!

@Evan and @James: could you please take another look?

[James Hawkins, 2013-07-02 17:37:14]

On 2013/07/02 16:43:38, pedrosimonetti wrote:
> On 2013/07/02 00:25:15, sky wrote:
> > That means you don't need me to review anymore, right?
> 
> @Sky: right, thanks for the review!
> 
> @Evan and @James: could you please take another look?

Sorry, which files am I supposed to be reviewing?

[pedro (no code reviews), 2013-07-02 18:12:35]

On 2013/07/02 17:37:14, James Hawkins wrote:
> On 2013/07/02 16:43:38, pedrosimonetti wrote:
> > On 2013/07/02 00:25:15, sky wrote:
> > > That means you don't need me to review anymore, right?
> > 
> > @Sky: right, thanks for the review!
> > 
> > @Evan and @James: could you please take another look?
> 
> Sorry, which files am I supposed to be reviewing?

Sorry for not making it clear.

@James: I need you to review everything except chrome/browser/ui/webui/* (most
of the changes are in chrome/common/* and the rest of the files only changes a
couple of lines in order to adjust the new path for favicon dependencies)

@Evan: chrome/browser/ui/webui/*

[Evan Stade, 2013-07-02 23:10:21]

webui lgtm

[James Hawkins, 2013-07-02 23:23:04]

Where is the unit test for favicon_url_parser.cc?

https://codereview.chromium.org/15388002/diff/69001/chrome/common/favicon/fav...
File chrome/common/favicon/favicon_url_parser.cc (right):

https://codereview.chromium.org/15388002/diff/69001/chrome/common/favicon/fav...
chrome/common/favicon/favicon_url_parser.cc:35: bool HasSubstringAt(const
std::string& path,
This isn't available somewhere in base already?

https://codereview.chromium.org/15388002/diff/69001/chrome/common/favicon/fav...
File chrome/common/favicon/favicon_url_parser.h (right):

https://codereview.chromium.org/15388002/diff/69001/chrome/common/favicon/fav...
chrome/common/favicon/favicon_url_parser.h:16: bool is_icon_url;
nit: Add blank line after membar var and before next documentation comment.

[pedro (no code reviews), 2013-07-03 00:13:35]

On 2013/07/02 23:23:04, James Hawkins wrote:
> Where is the unit test for favicon_url_parser.cc?
> 

James, the unit tests for the favicon parser are implemented in:
chrome/browser/ui/webui/favicon_source_unittest.cc
These tests exercises the non-Instant Extended behavior of favicon URLs.

The tests for the Instant Extended behavior is implemented in another CL
(https://codereview.chromium.org/18344010/), where the high dpi favicon feature
is actually implemented for Instant Extended.

[pedro (no code reviews), 2013-07-03 00:16:55]

https://codereview.chromium.org/15388002/diff/69001/chrome/common/favicon/fav...
File chrome/common/favicon/favicon_url_parser.cc (right):

https://codereview.chromium.org/15388002/diff/69001/chrome/common/favicon/fav...
chrome/common/favicon/favicon_url_parser.cc:35: bool HasSubstringAt(const
std::string& path,
On 2013/07/02 23:23:04, James Hawkins wrote:
> This isn't available somewhere in base already?

Does it have a different name? I would be glad to reuse an existing function but
I couldn't find a replacement for it.

https://codereview.chromium.org/15388002/diff/69001/chrome/common/favicon/fav...
File chrome/common/favicon/favicon_url_parser.h (right):

https://codereview.chromium.org/15388002/diff/69001/chrome/common/favicon/fav...
chrome/common/favicon/favicon_url_parser.h:16: bool is_icon_url;
On 2013/07/02 23:23:04, James Hawkins wrote:
> nit: Add blank line after membar var and before next documentation comment.

Done.

[James Hawkins, 2013-07-03 16:44:44]

https://codereview.chromium.org/15388002/diff/69001/chrome/common/favicon/fav...
File chrome/common/favicon/favicon_url_parser.cc (right):

https://codereview.chromium.org/15388002/diff/69001/chrome/common/favicon/fav...
chrome/common/favicon/favicon_url_parser.cc:35: bool HasSubstringAt(const
std::string& path,
On 2013/07/03 00:16:56, pedrosimonetti wrote:
> On 2013/07/02 23:23:04, James Hawkins wrote:
> > This isn't available somewhere in base already?
> 
> Does it have a different name? I would be glad to reuse an existing function
but
> I couldn't find a replacement for it.

I don't know; I'm asking if you've looked.  base/strings is the place to look.

[pedro (no code reviews), 2013-07-03 17:45:28]

On 2013/07/03 16:44:44, James Hawkins wrote:
>
https://codereview.chromium.org/15388002/diff/69001/chrome/common/favicon/fav...
> File chrome/common/favicon/favicon_url_parser.cc (right):
> 
>
https://codereview.chromium.org/15388002/diff/69001/chrome/common/favicon/fav...
> chrome/common/favicon/favicon_url_parser.cc:35: bool HasSubstringAt(const
> std::string& path,
> On 2013/07/03 00:16:56, pedrosimonetti wrote:
> > On 2013/07/02 23:23:04, James Hawkins wrote:
> > > This isn't available somewhere in base already?
> > 
> > Does it have a different name? I would be glad to reuse an existing function
> but
> > I couldn't find a replacement for it.
> 
> I don't know; I'm asking if you've looked.  base/strings is the place to look.

Yes, I have looked and I couldn't find a similar function in base/strings.

[James Hawkins, 2013-07-03 17:48:51]

On 2013/07/03 00:13:35, pedrosimonetti wrote:
> On 2013/07/02 23:23:04, James Hawkins wrote:
> > Where is the unit test for favicon_url_parser.cc?
> > 
> 
> James, the unit tests for the favicon parser are implemented in:
> chrome/browser/ui/webui/favicon_source_unittest.cc
> These tests exercises the non-Instant Extended behavior of favicon URLs.
> 
> The tests for the Instant Extended behavior is implemented in another CL
> (https://codereview.chromium.org/18344010/), where the high dpi favicon
feature
> is actually implemented for Instant Extended.

ParseFaviconPath is added in this CL; therefore, this CL needs tests for this
method.

[pedro (no code reviews), 2013-07-03 18:32:39]

On 2013/07/03 17:48:51, James Hawkins wrote:
> On 2013/07/03 00:13:35, pedrosimonetti wrote:
> > On 2013/07/02 23:23:04, James Hawkins wrote:
> > > Where is the unit test for favicon_url_parser.cc?
> > > 
> > 
> > James, the unit tests for the favicon parser are implemented in:
> > chrome/browser/ui/webui/favicon_source_unittest.cc
> > These tests exercises the non-Instant Extended behavior of favicon URLs.
> > 
> > The tests for the Instant Extended behavior is implemented in another CL
> > (https://codereview.chromium.org/18344010/), where the high dpi favicon
> feature
> > is actually implemented for Instant Extended.
> 
> ParseFaviconPath is added in this CL; therefore, this CL needs tests for this
> method.

ParseFaviconPath is already being tested in this CL in
chrome/browser/ui/webui/favicon_source_unittest.cc. It tests all code paths in
ParseFaviconPath that are being used.

There's only one code path in ParseFaviconPath that is not being tested right
now (using a pair <view_id>/<most_visited_id> instead of URL), but it is not
being used by the code right now. This code path, required by Instand Extended,
is tested on a separate CL, where this feature (using a pair
<view_id>/<most_visited_id> instead of URL) is being added.

[samarth, 2013-07-03 18:34:05]

I agree with jhawkins: the tests for the thing you're testing should live with
the thing being tested.  So if someone changes the ParsePath function in the
future, they'll know to update the tests along with it.

If the new tests duplicate some of the existing tests, you can remove the old
redundant ones.

Samarth

[pedro (no code reviews), 2013-07-03 21:26:11]

On 2013/07/03 18:34:05, samarth wrote:
> I agree with jhawkins: the tests for the thing you're testing should live with
> the thing being tested.  So if someone changes the ParsePath function in the
> future, they'll know to update the tests along with it.
> 
> If the new tests duplicate some of the existing tests, you can remove the old
> redundant ones.
> 
> Samarth

Thanks Samarth for the clarification. I guess I didn't understand James' last
comment. It does make sense putting the test in the same directory where the
code lives in.

James, please take another look.

[pedro (no code reviews), 2013-07-03 21:28:13]

https://codereview.chromium.org/15388002/diff/69001/chrome/common/favicon/fav...
File chrome/common/favicon/favicon_url_parser.cc (right):

https://codereview.chromium.org/15388002/diff/69001/chrome/common/favicon/fav...
chrome/common/favicon/favicon_url_parser.cc:35: bool HasSubstringAt(const
std::string& path,
On 2013/07/03 16:44:45, James Hawkins wrote:
> On 2013/07/03 00:16:56, pedrosimonetti wrote:
> > On 2013/07/02 23:23:04, James Hawkins wrote:
> > > This isn't available somewhere in base already?
> > 
> > Does it have a different name? I would be glad to reuse an existing function
> but
> > I couldn't find a replacement for it.
> 
> I don't know; I'm asking if you've looked.  base/strings is the place to look.

As I stated in a previous comment, I've looked for it, and there's no similar
function already implemented in base/strings.

[James Hawkins, 2013-07-07 18:14:01]

https://codereview.chromium.org/15388002/diff/91001/chrome/common/favicon/fav...
File chrome/common/favicon/favicon_url_parser_unittest.cc (right):

https://codereview.chromium.org/15388002/diff/91001/chrome/common/favicon/fav...
chrome/common/favicon/favicon_url_parser_unittest.cc:41: // 1) Test parsing path
with no extra parameters.
Each of these points should be a separate test.

[pedro (no code reviews), 2013-07-10 01:27:39]

James, please take another look.

https://codereview.chromium.org/15388002/diff/91001/chrome/common/favicon/fav...
File chrome/common/favicon/favicon_url_parser_unittest.cc (right):

https://codereview.chromium.org/15388002/diff/91001/chrome/common/favicon/fav...
chrome/common/favicon/favicon_url_parser_unittest.cc:41: // 1) Test parsing path
with no extra parameters.
On 2013/07/07 18:14:01, James Hawkins wrote:
> Each of these points should be a separate test.

Done.

[James Hawkins, 2013-07-10 22:50:57]

lgtm

[commit-bot: I haz the power, 2013-07-11 00:33:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/15388002/1...

[commit-bot: I haz the power, 2013-07-11 11:54:46]

Change committed as 211085