Normalizes URLs requested for favicons in the password manager dialog.

Normalizes URLs requested for favicons in the password manager dialog.

BUG=117635
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132707

[Evan Stade, 2012-04-14 00:32:43]

https://chromiumcodereview.appspot.com/10079024/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://chromiumcodereview.appspot.com/10079024/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/favicon_source.cc:61: if (path.size() > 10 &&
path.substr(0, 10) == "normalize/") {
why isn't this normalization just the default?

https://chromiumcodereview.appspot.com/10079024/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/favicon_source.cc:66: if (originalUrl.find("://") ==
std::string::npos)
probably safest to use GURL for this (it has complicated canonicalization
routines). Something like:

if (GURL(originalUrl).scheme().empty())

[Kyle Horimoto, 2012-04-16 19:08:15]

https://chromiumcodereview.appspot.com/10079024/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://chromiumcodereview.appspot.com/10079024/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/favicon_source.cc:61: if (path.size() > 10 &&
path.substr(0, 10) == "normalize/") {
On 2012/04/14 00:32:43, Evan Stade wrote:
> why isn't this normalization just the default?

Essentially, any page can have its own favicon (e.g., example.com/a and
example.com/b could have different favicons). Thus, doing this for every favicon
request could result in incorrect favicons. This is a special case because it is
managing passwords for an entire domain (i.e., we store the password for the
domain, not individually for each page on the domain).

https://chromiumcodereview.appspot.com/10079024/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/favicon_source.cc:66: if (originalUrl.find("://") ==
std::string::npos)
On 2012/04/14 00:32:43, Evan Stade wrote:
> probably safest to use GURL for this (it has complicated canonicalization
> routines). Something like:
> 
> if (GURL(originalUrl).scheme().empty())

Done.

[Evan Stade, 2012-04-16 20:45:18]

http://codereview.chromium.org/10079024/diff/1/chrome/browser/ui/webui/favico...
File chrome/browser/ui/webui/favicon_source.cc (right):

http://codereview.chromium.org/10079024/diff/1/chrome/browser/ui/webui/favico...
chrome/browser/ui/webui/favicon_source.cc:61: if (path.size() > 10 &&
path.substr(0, 10) == "normalize/") {
On 2012/04/16 19:08:15, Kyle Horimoto wrote:
> On 2012/04/14 00:32:43, Evan Stade wrote:
> > why isn't this normalization just the default?
> 
> Essentially, any page can have its own favicon (e.g., example.com/a and
> example.com/b could have different favicons). Thus, doing this for every
favicon
> request could result in incorrect favicons. This is a special case because it
is
> managing passwords for an entire domain (i.e., we store the password for the
> domain, not individually for each page on the domain).

in this case I would not call it "normalize", but "origin" or something. Also a
brief form of your response would be good to add as a comment.

[Kyle Horimoto, 2012-04-17 21:37:39]

http://codereview.chromium.org/10079024/diff/1/chrome/browser/ui/webui/favico...
File chrome/browser/ui/webui/favicon_source.cc (right):

http://codereview.chromium.org/10079024/diff/1/chrome/browser/ui/webui/favico...
chrome/browser/ui/webui/favicon_source.cc:61: if (path.size() > 10 &&
path.substr(0, 10) == "normalize/") {
On 2012/04/16 20:45:18, Evan Stade wrote:
> On 2012/04/16 19:08:15, Kyle Horimoto wrote:
> > On 2012/04/14 00:32:43, Evan Stade wrote:
> > > why isn't this normalization just the default?
> > 
> > Essentially, any page can have its own favicon (e.g., example.com/a and
> > example.com/b could have different favicons). Thus, doing this for every
> favicon
> > request could result in incorrect favicons. This is a special case because
it
> is
> > managing passwords for an entire domain (i.e., we store the password for the
> > domain, not individually for each page on the domain).
> 
> in this case I would not call it "normalize", but "origin" or something. Also
a
> brief form of your response would be good to add as a comment.

Done.

[Evan Stade, 2012-04-17 21:44:43]

lgtm with nit

http://codereview.chromium.org/10079024/diff/7001/chrome/browser/ui/webui/fav...
File chrome/browser/ui/webui/favicon_source.cc (right):

http://codereview.chromium.org/10079024/diff/7001/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:62: // The password manager, however,
manages passwords for entire domains.
this comment should describe the behavior, and a comment in
password_manager_list.js should describe why it's using it.

[Kyle Horimoto, 2012-04-17 22:13:37]

http://codereview.chromium.org/10079024/diff/7001/chrome/browser/ui/webui/fav...
File chrome/browser/ui/webui/favicon_source.cc (right):

http://codereview.chromium.org/10079024/diff/7001/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:62: // The password manager, however,
manages passwords for entire domains.
On 2012/04/17 21:44:44, Evan Stade wrote:
> this comment should describe the behavior, and a comment in
> password_manager_list.js should describe why it's using it.

Done.

[commit-bot: I haz the power, 2012-04-17 22:53:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10079024/7003

[commit-bot: I haz the power, 2012-04-18 00:29:55]

Change committed as 132707