Provide assumed favicon for intents if service provider page was never visited.

Provide assumed favicon for intents if page was never visited.

If the service_url has never been visited, the IntentsPicker used to show
a default icon. This change will instead associate the favicon of the
registration page with the service_url page.

BUG=99784
TEST=Start with clean browser install. Run with --enable-web-intents, register a provider from webintents.org. Start picker, it should show WI favicon.


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

[groby-ooo-7-16, 2011-11-10 23:57:31]

brettw: Please review Favicon/History/Thumbnail changes
jhawkins: Please look at intents/browser changes

[groby-ooo-7-16, 2011-11-11 00:33:34]

gbillock: You're the new jhawkins :) (I.e. please review)

[Greg Billock, 2011-11-11 17:47:56]

http://codereview.chromium.org/8469018/diff/1/chrome/browser/favicon/favicon_...
File chrome/browser/favicon/favicon_service.h (right):

http://codereview.chromium.org/8469018/diff/1/chrome/browser/favicon/favicon_...
chrome/browser/favicon/favicon_service.h:85: // Clone a Favicon from an existing
page. Needed if you want to declare
I'm not sure I understand. Does this method assign the favicon from old_page_url
to new_page_url?

Also, _f_avicon.

http://codereview.chromium.org/8469018/diff/1/chrome/browser/history/history.h
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/8469018/diff/1/chrome/browser/history/history....
chrome/browser/history/history.h:670: // provided that other page does not
already have favicons.
Is the purpose for this a cross-domain mapping? That is, wouldn't we load the
right favicon (speculatively, anyway) for a page we'd never seen, if we knew its
domain's favicon?

http://codereview.chromium.org/8469018/diff/1/chrome/browser/history/thumbnai...
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/8469018/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:565: "INSERT INTO icon_mapping
(page_url, icon_id) "
Will sqlite support a fancier conditional insert here? INSERT INTO SELECT ...
WHERE NOT EXISTS ( SELECT ... FROM ...) or something like that? It's been a
while but I seem to recall you can do this with mysql I think, at least.

[groby-ooo-7-16, 2011-11-14 23:53:36]

http://codereview.chromium.org/8469018/diff/1/chrome/browser/favicon/favicon_...
File chrome/browser/favicon/favicon_service.h (right):

http://codereview.chromium.org/8469018/diff/1/chrome/browser/favicon/favicon_...
chrome/browser/favicon/favicon_service.h:85: // Clone a Favicon from an existing
page. Needed if you want to declare
Clarified text, hopefully.

On 2011/11/11 17:47:56, Greg Billock wrote:
> I'm not sure I understand. Does this method assign the favicon from
old_page_url
> to new_page_url?
> 
> Also, _f_avicon.

http://codereview.chromium.org/8469018/diff/1/chrome/browser/history/history.h
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/8469018/diff/1/chrome/browser/history/history....
chrome/browser/history/history.h:670: // provided that other page does not
already have favicons.
Actually, we don't know the domain's favicon. We only know favicons that have
been explicitly associated with a URL in the ThumbnailDatabase.

If no icon is recorded for a given URL, the default icon (the gray globe) is
shown. That's the whole reason for this patch :)

On 2011/11/11 17:47:56, Greg Billock wrote:
> Is the purpose for this a cross-domain mapping? That is, wouldn't we load the
> right favicon (speculatively, anyway) for a page we'd never seen, if we knew
its
> domain's favicon?

http://codereview.chromium.org/8469018/diff/1/chrome/browser/history/thumbnai...
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/8469018/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:565: "INSERT INTO icon_mapping
(page_url, icon_id) "
On 2011/11/11 17:47:56, Greg Billock wrote:
> Will sqlite support a fancier conditional insert here? INSERT INTO SELECT ...
> WHERE NOT EXISTS ( SELECT ... FROM ...) or something like that? It's been a
> while but I seem to recall you can do this with mysql I think, at least.

yes, fancier statements are supported. Here's what the query would look like:
insert into icon_mapping (page_url, icon_id) select 'foo',icon_id from
icon_mapping where page_url='http://webintents.org/' and not exists (select
icon_id from icon_mapping where page_url='foo');

But

- The query becomes hideously long. Two shorter queries are IMHO easier to
read/reveal the intent.
- The usefulness of the query plan depends on the exact wording of the query.
SQLite doesn't have a decent plan optimizer. 

So leaving it this way for now unless you strongly object.

[brettw, 2011-11-15 00:02:42]

http://codereview.chromium.org/8469018/diff/8001/chrome/browser/history/thumb...
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/8469018/diff/8001/chrome/browser/history/thumb...
chrome/browser/history/thumbnail_database.cc:551: "SELECT COUNT(*) FROM
icon_mapping "
I'm not doing real reviews since I'm on leave, so you'll have to get another
history owner to review this. But don't do count() to see if something exist,
since it actually has to find all possible results. Select * and then see if
there's a result.

[groby-ooo-7-16, 2011-11-16 00:31:39]

sky: since brettw is on leave, mind taking a look?

http://codereview.chromium.org/8469018/diff/8001/chrome/browser/history/thumb...
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/8469018/diff/8001/chrome/browser/history/thumb...
chrome/browser/history/thumbnail_database.cc:551: "SELECT COUNT(*) FROM
icon_mapping "
On 2011/11/15 00:02:42, brettw wrote:
> I'm not doing real reviews since I'm on leave, so you'll have to get another
> history owner to review this. But don't do count() to see if something exist,
> since it actually has to find all possible results. Select * and then see if
> there's a result.

Done.

[sky, 2011-11-16 17:26:00]

http://codereview.chromium.org/8469018/diff/13001/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_service.h (right):

http://codereview.chromium.org/8469018/diff/13001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_service.h:85: // Clone a favicon from an existing
page. This associates the favicon from
Clarify this clones all icons, not just the favicon.

http://codereview.chromium.org/8469018/diff/13001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_service.h:90: void CloneFavicon(const GURL&
old_page_url, const GURL& new_page_url);
This is worrisome. There's nothing stopping someone from doing something like
CloneFavicon('evil.com', 'good.com'), which we wouldn't want. Could we restrict
this to the same domain? so that you only get to change the path?

[groby-ooo-7-16, 2011-11-17 00:37:55]

http://codereview.chromium.org/8469018/diff/13001/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_service.h (right):

http://codereview.chromium.org/8469018/diff/13001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_service.h:85: // Clone a favicon from an existing
page. This associates the favicon from
On 2011/11/16 17:26:00, sky wrote:
> Clarify this clones all icons, not just the favicon.

Done.

http://codereview.chromium.org/8469018/diff/13001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_service.h:90: void CloneFavicon(const GURL&
old_page_url, const GURL& new_page_url);
On 2011/11/16 17:26:00, sky wrote:
> This is worrisome. There's nothing stopping someone from doing something like
> CloneFavicon('evil.com', 'good.com'), which we wouldn't want. Could we
restrict
> this to the same domain? so that you only get to change the path?

I'm confused - how is that any more of a security risk than e.g. allowing a call
to SetFavicon? This is chrome-internal code - if an attacker can call that
directly, security is compromised anyways?

(Added anyways, just in case ;)

[sky, 2011-11-17 04:49:01]

LGTM

http://codereview.chromium.org/8469018/diff/13001/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_service.h (right):

http://codereview.chromium.org/8469018/diff/13001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_service.h:90: void CloneFavicon(const GURL&
old_page_url, const GURL& new_page_url);
On 2011/11/17 00:37:55, groby wrote:
> On 2011/11/16 17:26:00, sky wrote:
> > This is worrisome. There's nothing stopping someone from doing something
like
> > CloneFavicon('evil.com', 'good.com'), which we wouldn't want. Could we
> restrict
> > this to the same domain? so that you only get to change the path?
> 
> I'm confused - how is that any more of a security risk than e.g. allowing a
call
> to SetFavicon? This is chrome-internal code - if an attacker can call that
> directly, security is compromised anyways?
> 
> (Added anyways, just in case ;)

I wasn't worried about the attack risk (as you say, this is chrome code), but
more so of someone accidentally using the API for a case we don't need to
support nor want to support.

http://codereview.chromium.org/8469018/diff/18001/chrome/browser/history/hist...
File chrome/browser/history/history_backend_unittest.cc (right):

http://codereview.chromium.org/8469018/diff/18001/chrome/browser/history/hist...
chrome/browser/history/history_backend_unittest.cc:1143: 
nit: remove one line.

http://codereview.chromium.org/8469018/diff/18001/chrome/browser/history/thum...
File chrome/browser/history/thumbnail_database_unittest.cc (right):

http://codereview.chromium.org/8469018/diff/18001/chrome/browser/history/thum...
chrome/browser/history/thumbnail_database_unittest.cc:446: EXPECT_NE(id1, 0);
assertions should have the format expected, actual. So, the 0 should be first
for these (you have it right for the non-NE cases).

[groby-ooo-7-16, 2011-11-18 01:11:19]

http://codereview.chromium.org/8469018/diff/18001/chrome/browser/history/hist...
File chrome/browser/history/history_backend_unittest.cc (right):

http://codereview.chromium.org/8469018/diff/18001/chrome/browser/history/hist...
chrome/browser/history/history_backend_unittest.cc:1143: 
On 2011/11/17 04:49:01, sky wrote:
> nit: remove one line.

Done.

http://codereview.chromium.org/8469018/diff/18001/chrome/browser/history/thum...
File chrome/browser/history/thumbnail_database_unittest.cc (right):

http://codereview.chromium.org/8469018/diff/18001/chrome/browser/history/thum...
chrome/browser/history/thumbnail_database_unittest.cc:446: EXPECT_NE(id1, 0);
On 2011/11/17 04:49:01, sky wrote:
> assertions should have the format expected, actual. So, the 0 should be first
> for these (you have it right for the non-NE cases).

Done.

[commit-bot: I haz the power, 2011-11-18 04:07:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/8469018/22001

[commit-bot: I haz the power, 2011-11-18 05:50:18]

Change committed as 110662