move favicon download code from chrome/ into content/

move favicon download code from chrome/ into content/

-chrome/renderer/favicon_helper.(h|cc) ->content/renderer/...
-chrome/common/icon_messages.h -> content/common/icon_messages.h
-favicon_download_helper.(h|cc) API incorporated into web_contents 
-chrome/browser/favicon/favicon_download_helper_delegate.h -> content/public/browser/favicon_download_delegate.h

Depends on: 
https://codereview.chromium.org/11416179/
https://codereview.chromium.org/11421051/

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

[Cait (Slow), 2012-11-27 00:14:42]

Hi John,

This CL moves the remaining favicon download code into content/ and incorporates
DownloadFavicon() into the WebContents API. 

I kept the FaviconDownloadDelegate as a separate API, as  incorporating it into
the WebContentsDelegate API will require non-trivial changes in many cases (the
WCD is set  in a different place than the favicon request is made from), and I
would prefer to tackle these individually.

Thanks,
Cait



PTAL at the changes in:
-content/browser/web_contents/
-content/public/browser/
-content/common (IPC message definitions)

Thanks,
Cait

[jam, 2012-11-27 01:16:30]

I'm assuming you don't want me to look at
https://codereview.chromium.org/11416179/

https://codereview.chromium.org/11411180/diff/1/chrome/browser/favicon/favico...
File chrome/browser/favicon/favicon_download_helper.cc (right):

https://codereview.chromium.org/11411180/diff/1/chrome/browser/favicon/favico...
chrome/browser/favicon/favicon_download_helper.cc:34: host->Send(new
IconMsg_DownloadFavicon(host->GetRoutingID(), id, url,
is this file wrong? i.e. icon_messages.h is now in content

https://codereview.chromium.org/11411180/diff/1/chrome/browser/favicon/favico...
File chrome/browser/favicon/favicon_download_helper.h (right):

https://codereview.chromium.org/11411180/diff/1/chrome/browser/favicon/favico...
chrome/browser/favicon/favicon_download_helper.h:38: //
IconHostMsg_DidDownloadFavicon IPC message will be sent and passed on to
nit: don't describe the implementation details, especially from another module,
in the method's comment

https://codereview.chromium.org/11411180/diff/1/chrome/browser/favicon/favico...
File chrome/browser/favicon/favicon_download_helper_delegate.h (right):

https://codereview.chromium.org/11411180/diff/1/chrome/browser/favicon/favico...
chrome/browser/favicon/favicon_download_helper_delegate.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
shouldn't this be a delete?

https://codereview.chromium.org/11411180/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/11411180/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_contents.h:385: 
nit: undo

https://codereview.chromium.org/11411180/diff/1/chrome/browser/ui/web_applica...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://codereview.chromium.org/11411180/diff/1/chrome/browser/ui/web_applica...
chrome/browser/ui/web_applications/web_app_ui.cc:63: //
content::WebContentsDelegate override.
nit: content::FaviconDownloadDelegate

https://codereview.chromium.org/11411180/diff/1/chrome/browser/ui/web_applica...
chrome/browser/ui/web_applications/web_app_ui.cc:172: DeleteMe();  // We are
done if underlying WebContents is gone.
this can't happen anymore right?

https://codereview.chromium.org/11411180/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11411180/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:153: static int
current_favicon_download_id = 0;
nit: prefix with "g_" to make it clear that this is a static

also, usually we name this "next" instead of "current"

https://codereview.chromium.org/11411180/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:1956: DCHECK(i ==
favicon_download_ids_.end());
this check seems unnecessary given that you have a counter that you control the
key

https://codereview.chromium.org/11411180/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:2398: // Currently WebContents
notifies us of ANY downloads so that it is
I'm not sure I understand this comment. This is an IPC coming from
RenderViewHost. why would it send this message for a favicon that is unknown? 

you should have a check like you do to guard against malicious renderers, but
the comment seems off

https://codereview.chromium.org/11411180/diff/1/content/public/browser/web_co...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/11411180/diff/1/content/public/browser/web_co...
content/public/browser/web_contents.h:405: virtual int DownloadFavicon(const
GURL& url, int image_size) = 0;
nit: comment is out of date (re IPC)

seems that a simpler API is to have this function takes a
FaviconDownloadDelegate as a parameter? then that way you wouldn't need
RegisterFaviconDelegate. this would also avoid the issue of a delegate being
overwritten if multiple classes fetch favicons from the same webcontents

https://codereview.chromium.org/11411180/diff/1/content/public/browser/web_co...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/11411180/diff/1/content/public/browser/web_co...
content/public/browser/web_contents_delegate.h:41: struct FaviconURL;
this isn't used here?

https://codereview.chromium.org/11411180/diff/1/content/renderer/favicon_help...
File content/renderer/favicon_helper.cc (right):

https://codereview.chromium.org/11411180/diff/1/content/renderer/favicon_help...
content/renderer/favicon_helper.cc:26: using content::FaviconURL;
nit: get rid of this

https://codereview.chromium.org/11411180/diff/1/content/renderer/favicon_help...
content/renderer/favicon_helper.cc:35: bool TouchEnabled() {
nit: put the anonymous namespace inside the content namespace, and also don't
indent the function

https://codereview.chromium.org/11411180/diff/1/content/renderer/favicon_help...
File content/renderer/favicon_helper.h (right):

https://codereview.chromium.org/11411180/diff/1/content/renderer/favicon_help...
content/renderer/favicon_helper.h:65: const std::vector<content::FaviconURL>&
urls);
nit: get rid of "content::"

https://codereview.chromium.org/11411180/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11411180/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.h:1407: FaviconHelper* favicon_helper_;
not needed, remove

[Cait (Slow), 2012-11-28 00:11:45]

https://chromiumcodereview.appspot.com/11411180/diff/1/chrome/browser/favicon...
File chrome/browser/favicon/favicon_download_helper.cc (right):

https://chromiumcodereview.appspot.com/11411180/diff/1/chrome/browser/favicon...
chrome/browser/favicon/favicon_download_helper.cc:34: host->Send(new
IconMsg_DownloadFavicon(host->GetRoutingID(), id, url,
On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> is this file wrong? i.e. icon_messages.h is now in content

Done.

https://chromiumcodereview.appspot.com/11411180/diff/1/chrome/browser/favicon...
File chrome/browser/favicon/favicon_download_helper.h (right):

https://chromiumcodereview.appspot.com/11411180/diff/1/chrome/browser/favicon...
chrome/browser/favicon/favicon_download_helper.h:38: //
IconHostMsg_DidDownloadFavicon IPC message will be sent and passed on to
On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> nit: don't describe the implementation details, especially from another
module,
> in the method's comment

Done.

https://chromiumcodereview.appspot.com/11411180/diff/1/chrome/browser/favicon...
File chrome/browser/favicon/favicon_download_helper_delegate.h (right):

https://chromiumcodereview.appspot.com/11411180/diff/1/chrome/browser/favicon...
chrome/browser/favicon/favicon_download_helper_delegate.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> shouldn't this be a delete?

Done.

https://chromiumcodereview.appspot.com/11411180/diff/1/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.h (right):

https://chromiumcodereview.appspot.com/11411180/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.h:385: 
On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> nit: undo

Done.

https://chromiumcodereview.appspot.com/11411180/diff/1/chrome/browser/ui/web_...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://chromiumcodereview.appspot.com/11411180/diff/1/chrome/browser/ui/web_...
chrome/browser/ui/web_applications/web_app_ui.cc:63: //
content::WebContentsDelegate override.
On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> nit: content::FaviconDownloadDelegate

Done.

https://chromiumcodereview.appspot.com/11411180/diff/1/chrome/browser/ui/web_...
chrome/browser/ui/web_applications/web_app_ui.cc:172: DeleteMe();  // We are
done if underlying WebContents is gone.
On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> this can't happen anymore right?

Done.

https://chromiumcodereview.appspot.com/11411180/diff/1/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/11411180/diff/1/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:153: static int
current_favicon_download_id = 0;
On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> nit: prefix with "g_" to make it clear that this is a static
> 
> also, usually we name this "next" instead of "current"

Done.

https://chromiumcodereview.appspot.com/11411180/diff/1/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1956: DCHECK(i ==
favicon_download_ids_.end());
On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> this check seems unnecessary given that you have a counter that you control
the
> key

Done.

https://chromiumcodereview.appspot.com/11411180/diff/1/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2398: // Currently WebContents
notifies us of ANY downloads so that it is
On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> I'm not sure I understand this comment. This is an IPC coming from
> RenderViewHost. why would it send this message for a favicon that is unknown? 
> 
> you should have a check like you do to guard against malicious renderers, but
> the comment seems off

Done.

https://chromiumcodereview.appspot.com/11411180/diff/1/content/public/browser...
File content/public/browser/web_contents.h (right):

https://chromiumcodereview.appspot.com/11411180/diff/1/content/public/browser...
content/public/browser/web_contents.h:405: virtual int DownloadFavicon(const
GURL& url, int image_size) = 0;
Fixed the comment. Unfortunately, the delegate has to be set prior to the
download request being made in order to receive the new faviconURL from the
renderer (this is the purpose of the UpdateFaviconURL method on the delegate).

On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> nit: comment is out of date (re IPC)
> 
> seems that a simpler API is to have this function takes a
> FaviconDownloadDelegate as a parameter? then that way you wouldn't need
> RegisterFaviconDelegate. this would also avoid the issue of a delegate being
> overwritten if multiple classes fetch favicons from the same webcontents

https://chromiumcodereview.appspot.com/11411180/diff/1/content/public/browser...
File content/public/browser/web_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/11411180/diff/1/content/public/browser...
content/public/browser/web_contents_delegate.h:41: struct FaviconURL;
On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> this isn't used here?

Done.

https://chromiumcodereview.appspot.com/11411180/diff/1/content/renderer/favic...
File content/renderer/favicon_helper.cc (right):

https://chromiumcodereview.appspot.com/11411180/diff/1/content/renderer/favic...
content/renderer/favicon_helper.cc:26: using content::FaviconURL;
On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> nit: get rid of this

Done.

https://chromiumcodereview.appspot.com/11411180/diff/1/content/renderer/favic...
content/renderer/favicon_helper.cc:35: bool TouchEnabled() {
On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> nit: put the anonymous namespace inside the content namespace, and also don't
> indent the function

Done.

https://chromiumcodereview.appspot.com/11411180/diff/1/content/renderer/favic...
File content/renderer/favicon_helper.h (right):

https://chromiumcodereview.appspot.com/11411180/diff/1/content/renderer/favic...
content/renderer/favicon_helper.h:65: const std::vector<content::FaviconURL>&
urls);
On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> nit: get rid of "content::"

Done.

https://chromiumcodereview.appspot.com/11411180/diff/1/content/renderer/rende...
File content/renderer/render_view_impl.h (right):

https://chromiumcodereview.appspot.com/11411180/diff/1/content/renderer/rende...
content/renderer/render_view_impl.h:1407: FaviconHelper* favicon_helper_;
On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> not needed, remove
Should the renderer-side favicon class (content/renderer/favicon_helper.cc) be
initialized elsewhere?

[jam, 2012-11-28 04:08:16]

https://chromiumcodereview.appspot.com/11411180/diff/1/content/public/browser...
File content/public/browser/web_contents.h (right):

https://chromiumcodereview.appspot.com/11411180/diff/1/content/public/browser...
content/public/browser/web_contents.h:405: virtual int DownloadFavicon(const
GURL& url, int image_size) = 0;
On 2012/11/28 00:11:45, caitkp wrote:
> Fixed the comment. Unfortunately, the delegate has to be set prior to the
> download request being made in order to receive the new faviconURL from the
> renderer (this is the purpose of the UpdateFaviconURL method on the delegate).
> 
> On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> > nit: comment is out of date (re IPC)
> > 
> > seems that a simpler API is to have this function takes a
> > FaviconDownloadDelegate as a parameter? then that way you wouldn't need
> > RegisterFaviconDelegate. this would also avoid the issue of a delegate being
> > overwritten if multiple classes fetch favicons from the same webcontents
> 

can the updated url be saved in WebContents until a download is requested and
then given to the delegate then? my concern is that it will be error prone to
have a way to set a delegate which can be overwridden by other objects

[Cait (Slow), 2012-11-29 21:59:18]

On 2012/11/28 04:08:16, John Abd-El-Malek wrote:
>
https://chromiumcodereview.appspot.com/11411180/diff/1/content/public/browser...
> File content/public/browser/web_contents.h (right):
> 
>
https://chromiumcodereview.appspot.com/11411180/diff/1/content/public/browser...
> content/public/browser/web_contents.h:405: virtual int DownloadFavicon(const
> GURL& url, int image_size) = 0;
> On 2012/11/28 00:11:45, caitkp wrote:
> > Fixed the comment. Unfortunately, the delegate has to be set prior to the
> > download request being made in order to receive the new faviconURL from the
> > renderer (this is the purpose of the UpdateFaviconURL method on the
delegate).
> > 
> > On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> > > nit: comment is out of date (re IPC)
> > > 
> > > seems that a simpler API is to have this function takes a
> > > FaviconDownloadDelegate as a parameter? then that way you wouldn't need
> > > RegisterFaviconDelegate. this would also avoid the issue of a delegate
being
> > > overwritten if multiple classes fetch favicons from the same webcontents
> > 
> 
> can the updated url be saved in WebContents until a download is requested and
> then given to the delegate then? my concern is that it will be error prone to
> have a way to set a delegate which can be overwridden by other objects

What about allowing multiple favicon delegates to be set for one WebContents (it
might make more sense to call them observers in this case)? Since they (the
delegates/observers) all share the same WebContents, any faviconURL update that
effects one should effect them all.

[jam, 2012-11-30 23:04:32]

(sorry for the delay, I only just saw this now. in the future please feel free
to ping me if I don't respond in a few hours)

On 2012/11/29 21:59:18, caitkp wrote:
> On 2012/11/28 04:08:16, John Abd-El-Malek wrote:
> >
>
https://chromiumcodereview.appspot.com/11411180/diff/1/content/public/browser...
> > File content/public/browser/web_contents.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/11411180/diff/1/content/public/browser...
> > content/public/browser/web_contents.h:405: virtual int DownloadFavicon(const
> > GURL& url, int image_size) = 0;
> > On 2012/11/28 00:11:45, caitkp wrote:
> > > Fixed the comment. Unfortunately, the delegate has to be set prior to the
> > > download request being made in order to receive the new faviconURL from
the
> > > renderer (this is the purpose of the UpdateFaviconURL method on the
> delegate).
> > > 
> > > On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> > > > nit: comment is out of date (re IPC)
> > > > 
> > > > seems that a simpler API is to have this function takes a
> > > > FaviconDownloadDelegate as a parameter? then that way you wouldn't need
> > > > RegisterFaviconDelegate. this would also avoid the issue of a delegate
> being
> > > > overwritten if multiple classes fetch favicons from the same webcontents
> > > 
> > 
> > can the updated url be saved in WebContents until a download is requested
and
> > then given to the delegate then? my concern is that it will be error prone
to
> > have a way to set a delegate which can be overwridden by other objects
> 
> What about allowing multiple favicon delegates to be set for one WebContents
(it
> might make more sense to call them observers in this case)? Since they (the
> delegates/observers) all share the same WebContents, any faviconURL update
that
> effects one should effect them all.

in general, a 'delegate' is usually one per object. how about a small variation
on this idea though:

-UpdateFaviconURL would be a method on WebContentsObserver
-remove FaviconDownloadDelegate since it would only have one method remaining
(DidDownloadFavicon), and instead make DownloadFavicon take a callback?

[Cait (Slow), 2012-12-03 21:20:37]

On 2012/11/30 23:04:32, John Abd-El-Malek wrote:
> (sorry for the delay, I only just saw this now. in the future please feel free
> to ping me if I don't respond in a few hours)
> 
> On 2012/11/29 21:59:18, caitkp wrote:
> > On 2012/11/28 04:08:16, John Abd-El-Malek wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/11411180/diff/1/content/public/browser...
> > > File content/public/browser/web_contents.h (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/11411180/diff/1/content/public/browser...
> > > content/public/browser/web_contents.h:405: virtual int
DownloadFavicon(const
> > > GURL& url, int image_size) = 0;
> > > On 2012/11/28 00:11:45, caitkp wrote:
> > > > Fixed the comment. Unfortunately, the delegate has to be set prior to
the
> > > > download request being made in order to receive the new faviconURL from
> the
> > > > renderer (this is the purpose of the UpdateFaviconURL method on the
> > delegate).
> > > > 
> > > > On 2012/11/27 01:16:31, John Abd-El-Malek wrote:
> > > > > nit: comment is out of date (re IPC)
> > > > > 
> > > > > seems that a simpler API is to have this function takes a
> > > > > FaviconDownloadDelegate as a parameter? then that way you wouldn't
need
> > > > > RegisterFaviconDelegate. this would also avoid the issue of a delegate
> > being
> > > > > overwritten if multiple classes fetch favicons from the same
webcontents
> > > > 
> > > 
> > > can the updated url be saved in WebContents until a download is requested
> and
> > > then given to the delegate then? my concern is that it will be error prone
> to
> > > have a way to set a delegate which can be overwridden by other objects
> > 
> > What about allowing multiple favicon delegates to be set for one WebContents
> (it
> > might make more sense to call them observers in this case)? Since they (the
> > delegates/observers) all share the same WebContents, any faviconURL update
> that
> > effects one should effect them all.
> 
> in general, a 'delegate' is usually one per object. how about a small
variation
> on this idea though:
> 
> -UpdateFaviconURL would be a method on WebContentsObserver
> -remove FaviconDownloadDelegate since it would only have one method remaining
> (DidDownloadFavicon), and instead make DownloadFavicon take a callback?

PTAL at patch set 3 (specifically web_contents and web_contents_observer) I
modified DownloadFavicon() to take a callback and made UpdateFaviconUrl a method
on WCO.

[jam, 2012-12-03 21:59:39]

lgtm, thanks this looks much nicer

I tried to look at all the places where base::Unretained was used to double
check that it's safe. there were two places (both in ash) that I wasn't sure
about. might be worthwhile to ping someone familiar with that code to see if
that assumption holds, or if a weak factory needs to be used

https://chromiumcodereview.appspot.com/11411180/diff/2004/chrome/browser/ui/a...
File chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc (right):

https://chromiumcodereview.appspot.com/11411180/diff/2004/chrome/browser/ui/a...
chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc:12: #include
"content/public/common/favicon_url.h"
nit: this is in the header, so don't add it here as well

https://chromiumcodereview.appspot.com/11411180/diff/2004/chrome/browser/ui/a...
chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc:114:
base::Unretained(this)));
ditto re lifetime

https://chromiumcodereview.appspot.com/11411180/diff/2004/chrome/browser/ui/v...
File chrome/browser/ui/views/ash/balloon_view_ash.cc (right):

https://chromiumcodereview.appspot.com/11411180/diff/2004/chrome/browser/ui/v...
chrome/browser/ui/views/ash/balloon_view_ash.cc:22: #include
"googleurl/src/gurl.h"
this is pretty redundant to include, by definition web_contents.h,
site_instance.h etc include this. i realize the script probably complained, but
i think it's better to ignore this warning

https://chromiumcodereview.appspot.com/11411180/diff/2004/chrome/browser/ui/v...
chrome/browser/ui/views/ash/balloon_view_ash.cc:25: #include
"third_party/skia/include/core/SkBitmap.h"
ditto

https://chromiumcodereview.appspot.com/11411180/diff/2004/chrome/browser/ui/v...
chrome/browser/ui/views/ash/balloon_view_ash.cc:129: base::Unretained(this)));
are you sure that this object will outlive web_contents?

https://chromiumcodereview.appspot.com/11411180/diff/2004/chrome/browser/ui/w...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://chromiumcodereview.appspot.com/11411180/diff/2004/chrome/browser/ui/w...
chrome/browser/ui/web_applications/web_app_ui.cc:162:
unprocessed_icons_.back().height),
nit:spacing

https://chromiumcodereview.appspot.com/11411180/diff/2004/content/browser/web...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/11411180/diff/2004/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:72: #include
"content/public/common/favicon_url.h"
nit: redundant since by definition icon_messages.h includes this

https://chromiumcodereview.appspot.com/11411180/diff/2004/content/renderer/fa...
File content/renderer/favicon_helper.cc (right):

https://chromiumcodereview.appspot.com/11411180/diff/2004/content/renderer/fa...
content/renderer/favicon_helper.cc:11: #include
"content/public/common/favicon_url.h"
nit: redundant since by definition icon_messages.h includes this

https://chromiumcodereview.appspot.com/11411180/diff/2004/content/renderer/fa...
File content/renderer/favicon_helper.h (right):

https://chromiumcodereview.appspot.com/11411180/diff/2004/content/renderer/fa...
content/renderer/favicon_helper.h:70: virtual void
DidChangeIcon(WebKit::WebFrame* frame,
this was only added to RenderViewObserver because the favicon code was outside
content. now that this is in inside content, you can take it out of the
interface, and have RenderViewImpl call this method on FaviconHelper directly

[Cait (Slow), 2012-12-04 20:57:35]

https://codereview.chromium.org/11411180/diff/2004/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc (right):

https://codereview.chromium.org/11411180/diff/2004/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc:12: #include
"content/public/common/favicon_url.h"
On 2012/12/03 21:59:39, John Abd-El-Malek wrote:
> nit: this is in the header, so don't add it here as well

Done.

https://codereview.chromium.org/11411180/diff/2004/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/balloon_view_ash.cc (right):

https://codereview.chromium.org/11411180/diff/2004/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/balloon_view_ash.cc:22: #include
"googleurl/src/gurl.h"
On 2012/12/03 21:59:39, John Abd-El-Malek wrote:
> this is pretty redundant to include, by definition web_contents.h,
> site_instance.h etc include this. i realize the script probably complained,
but
> i think it's better to ignore this warning

Done.

https://codereview.chromium.org/11411180/diff/2004/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/balloon_view_ash.cc:25: #include
"third_party/skia/include/core/SkBitmap.h"
On 2012/12/03 21:59:39, John Abd-El-Malek wrote:
> ditto

Done.

https://codereview.chromium.org/11411180/diff/2004/chrome/browser/ui/web_appl...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://codereview.chromium.org/11411180/diff/2004/chrome/browser/ui/web_appl...
chrome/browser/ui/web_applications/web_app_ui.cc:162:
unprocessed_icons_.back().height),
On 2012/12/03 21:59:39, John Abd-El-Malek wrote:
> nit:spacing

Done.

https://codereview.chromium.org/11411180/diff/2004/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11411180/diff/2004/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:72: #include
"content/public/common/favicon_url.h"
On 2012/12/03 21:59:39, John Abd-El-Malek wrote:
> nit: redundant since by definition icon_messages.h includes this

Done.

https://codereview.chromium.org/11411180/diff/2004/content/renderer/favicon_h...
File content/renderer/favicon_helper.cc (right):

https://codereview.chromium.org/11411180/diff/2004/content/renderer/favicon_h...
content/renderer/favicon_helper.cc:11: #include
"content/public/common/favicon_url.h"
On 2012/12/03 21:59:39, John Abd-El-Malek wrote:
> nit: redundant since by definition icon_messages.h includes this

Done.

https://codereview.chromium.org/11411180/diff/2004/content/renderer/favicon_h...
File content/renderer/favicon_helper.h (right):

https://codereview.chromium.org/11411180/diff/2004/content/renderer/favicon_h...
content/renderer/favicon_helper.h:70: virtual void
DidChangeIcon(WebKit::WebFrame* frame,
On 2012/12/03 21:59:39, John Abd-El-Malek wrote:
> this was only added to RenderViewObserver because the favicon code was outside
> content. now that this is in inside content, you can take it out of the
> interface, and have RenderViewImpl call this method on FaviconHelper directly

Done.

[Cait (Slow), 2012-12-05 19:10:19]

Hi Cris,

PTAL at content/common/icon_messages.h. This is the same code that previously
lived in chrome/common/icon_messages.h. It is being moved because all icon
downloading functionality is being moved into content/. No new messages are
added.

Thanks,
Cait

[Cris Neckar, 2012-12-05 19:50:39]

IPC changes lgtm

[commit-bot: I haz the power, 2012-12-05 20:56:14]

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

[commit-bot: I haz the power, 2012-12-05 20:56:24]

Failed to apply patch for chrome/browser/favicon/favicon_handler_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/favicon/favicon_handler_unittest.cc
  Hunk #1 FAILED at 6.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/favicon/favicon_handler_unittest.cc.rej

Patch:       chrome/browser/favicon/favicon_handler_unittest.cc
Index: chrome/browser/favicon/favicon_handler_unittest.cc
diff --git a/chrome/browser/favicon/favicon_handler_unittest.cc
b/chrome/browser/favicon/favicon_handler_unittest.cc
index
bfa4250dc9b1d584abc5738e67280d000b7f06f4..4fc770314e203f64c64858fb2b67fb10ce290b32
100644
--- a/chrome/browser/favicon/favicon_handler_unittest.cc
+++ b/chrome/browser/favicon/favicon_handler_unittest.cc
@@ -6,17 +6,18 @@
 #include "chrome/browser/favicon/favicon_handler.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "content/public/browser/favicon_status.h"
 #include "content/public/browser/invalidate_type.h"
 #include "content/public/browser/navigation_entry.h"
-#include "content/public/browser/favicon_status.h"
 #include "content/public/browser/web_contents.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/favicon_size.h"
 #include "ui/gfx/image/image.h"
-#include "third_party/skia/include/core/SkBitmap.h"
 
 class TestFaviconHandler;
 
+using content::FaviconURL;
 using content::NavigationEntry;
 using content::WebContents;

[commit-bot: I haz the power, 2012-12-05 21:07:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/11411180/24002

[commit-bot: I haz the power, 2012-12-05 21:08:00]

Failed to apply patch for chrome/browser/favicon/favicon_handler_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/favicon/favicon_handler_unittest.cc
  Hunk #1 FAILED at 6.
  Hunk #2 FAILED at 280.
  2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/favicon/favicon_handler_unittest.cc.rej

Patch:       chrome/browser/favicon/favicon_handler_unittest.cc
Index: chrome/browser/favicon/favicon_handler_unittest.cc
diff --git a/chrome/browser/favicon/favicon_handler_unittest.cc
b/chrome/browser/favicon/favicon_handler_unittest.cc
index
bfa4250dc9b1d584abc5738e67280d000b7f06f4..4c29cef7dabca7cb0acf172845f3f2e74afc47cb
100644
--- a/chrome/browser/favicon/favicon_handler_unittest.cc
+++ b/chrome/browser/favicon/favicon_handler_unittest.cc
@@ -6,17 +6,18 @@
 #include "chrome/browser/favicon/favicon_handler.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "content/public/browser/favicon_status.h"
 #include "content/public/browser/invalidate_type.h"
 #include "content/public/browser/navigation_entry.h"
-#include "content/public/browser/favicon_status.h"
 #include "content/public/browser/web_contents.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/favicon_size.h"
 #include "ui/gfx/image/image.h"
-#include "third_party/skia/include/core/SkBitmap.h"
 
 class TestFaviconHandler;
 
+using content::FaviconURL;
 using content::NavigationEntry;
 using content::WebContents;
 
@@ -280,9 +281,7 @@ class TestFaviconHandler : public FaviconHandler {
                                   const GURL& icon_url,
                                   history::IconType icon_type,
                                   const gfx::Image& image) OVERRIDE {
-    std::vector<unsigned char> bitmap_data;
-    if (!image.IsEmpty())
-      bitmap_data = *image.ToImagePNG();
+    std::vector<unsigned char> bitmap_data = image.As1xPNGBytes()->data();
     history_handler_.reset(new HistoryRequestHandler(
         page_url, icon_url, icon_type, bitmap_data,
         FaviconService::FaviconResultsCallback()));

[commit-bot: I haz the power, 2012-12-05 23:09:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/11411180/20002

[commit-bot: I haz the power, 2012-12-05 23:10:10]

Presubmit check for 11411180-20002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Your #include order seems to be broken. Send mail to
marja@chromium.org if this is not the case.
      content/renderer/favicon_helper.cc:15

Presubmit checks took 5.8s to calculate.

[commit-bot: I haz the power, 2012-12-05 23:14:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/11411180/16055

[commit-bot: I haz the power, 2012-12-06 06:13:55]

Change committed as 171411