Extract renderer-side favicon downloading code into separate helper class

Extract renderer-side favicon downloading code into separate helper class

This is the renderer-side companion to http://codereview.chromium.org/11195010/

BUG=160995
TEST=no visible change

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

[Cait (Slow), 2012-10-24 14:30:10]

sky: PTAL -- this change is so that  the favicon downloading code can be reused
by WebView.

Thanks!

[joth, 2012-10-24 22:02:23]

minor suggestions (but as the is in existing code that's just being moved, may
be as well  not to do these changes in the same patch).

thanks!

http://codereview.chromium.org/11232068/diff/1007/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/11232068/diff/1007/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:280: FaviconHelper* favicon = 
 new FaviconHelper(render_view);
whitespace nit

http://codereview.chromium.org/11232068/diff/1007/chrome/renderer/favicon_hel...
File chrome/renderer/favicon_helper.cc (right):

http://codereview.chromium.org/11232068/diff/1007/chrome/renderer/favicon_hel...
chrome/renderer/favicon_helper.cc:105: for (iter = image_fetchers_.begin(); iter
!= image_fetchers_.end(); ++iter) {
fwiw, if image_fetchers_ is ScopedVector I think we can just use std::find() on
it here.

also, feels like it should DCHECK that the fetcher was found? else we'd
double-delete the fetcher if it every gave us 2 callbacks.

http://codereview.chromium.org/11232068/diff/1007/chrome/renderer/favicon_hel...
File chrome/renderer/favicon_helper.h (right):

http://codereview.chromium.org/11232068/diff/1007/chrome/renderer/favicon_hel...
chrome/renderer/favicon_helper.h:36: private:
unindent one space nit

http://codereview.chromium.org/11232068/diff/1007/chrome/renderer/favicon_hel...
chrome/renderer/favicon_helper.h:64:
linked_ptr<webkit_glue::MultiResolutionImageResourceFetcher> >
can this be ScopedVector<webkit_glue::MultiResolutionImageResourceFetcher> ?
I saw a thread recently preferring this.

[Cait (Slow), 2012-10-25 20:29:23]

http://codereview.chromium.org/11232068/diff/1007/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/11232068/diff/1007/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:280: FaviconHelper* favicon = 
 new FaviconHelper(render_view);
On 2012/10/24 22:02:23, joth wrote:
> whitespace nit

Done.

http://codereview.chromium.org/11232068/diff/1007/chrome/renderer/favicon_hel...
File chrome/renderer/favicon_helper.cc (right):

http://codereview.chromium.org/11232068/diff/1007/chrome/renderer/favicon_hel...
chrome/renderer/favicon_helper.cc:105: for (iter = image_fetchers_.begin(); iter
!= image_fetchers_.end(); ++iter) {
On 2012/10/24 22:02:23, joth wrote:
> fwiw, if image_fetchers_ is ScopedVector I think we can just use std::find()
on
> it here.
> 
> also, feels like it should DCHECK that the fetcher was found? else we'd
> double-delete the fetcher if it every gave us 2 callbacks.

Done.

http://codereview.chromium.org/11232068/diff/1007/chrome/renderer/favicon_hel...
File chrome/renderer/favicon_helper.h (right):

http://codereview.chromium.org/11232068/diff/1007/chrome/renderer/favicon_hel...
chrome/renderer/favicon_helper.h:36: private:
On 2012/10/24 22:02:23, joth wrote:
> unindent one space nit

Done.

http://codereview.chromium.org/11232068/diff/1007/chrome/renderer/favicon_hel...
chrome/renderer/favicon_helper.h:64:
linked_ptr<webkit_glue::MultiResolutionImageResourceFetcher> >
On 2012/10/24 22:02:23, joth wrote:
> can this be ScopedVector<webkit_glue::MultiResolutionImageResourceFetcher> ?
> I saw a thread recently preferring this.

Done.

[joth, 2012-10-30 14:41:45]

On non-nit suggestion here - I think we can go a bit further with the separation
from ChromeRenderViewObserver, which will make the extracted helper far more
convenient (less duplication) for us to use in webview.

Thanks!

https://codereview.chromium.org/11232068/diff/5002/chrome/renderer/chrome_ren...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/11232068/diff/5002/chrome/renderer/chrome_ren...
chrome/renderer/chrome_render_view_observer.cc:679: routing_id(),
render_view()->GetPageId(), urls);
rather than have this observer call into favicon observer to have that one do
the work, I think this whole piece from line 666 downwards could move into a
DidStopLoading override in FaviconHelper ?

https://codereview.chromium.org/11232068/diff/5002/chrome/renderer/chrome_ren...
chrome/renderer/chrome_render_view_observer.cc:698: routing_id(),
render_view()->GetPageId(), urls);
likewise, this entire method (line 684 - 698) could move into
FaviconHelper::DidChangeIcon ?

together these two changes will mean favicon_helper_ doesn't need to be passed
into this class at all, giving a much clearer separation of the two.

https://codereview.chromium.org/11232068/diff/5002/chrome/renderer/favicon_he...
File chrome/renderer/favicon_helper.h (right):

https://codereview.chromium.org/11232068/diff/5002/chrome/renderer/favicon_he...
chrome/renderer/favicon_helper.h:25: // There is one FaviconHelper per
RenderView.
perhaps also mention these instances are owned by the RenderView.

https://codereview.chromium.org/11232068/diff/5002/chrome/renderer/favicon_he...
chrome/renderer/favicon_helper.h:30: virtual ~FaviconHelper();
d'tor can be private. (It gets deleted via OnDestruct)

[Cait (Slow), 2012-10-30 20:07:33]

https://codereview.chromium.org/11232068/diff/5002/chrome/renderer/chrome_ren...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/11232068/diff/5002/chrome/renderer/chrome_ren...
chrome/renderer/chrome_render_view_observer.cc:679: routing_id(),
render_view()->GetPageId(), urls);
On 2012/10/30 14:41:45, joth wrote:
> rather than have this observer call into favicon observer to have that one do
> the work, I think this whole piece from line 666 downwards could move into a
> DidStopLoading override in FaviconHelper ?

Done.

https://codereview.chromium.org/11232068/diff/5002/chrome/renderer/chrome_ren...
chrome/renderer/chrome_render_view_observer.cc:698: routing_id(),
render_view()->GetPageId(), urls);
On 2012/10/30 14:41:45, joth wrote:
> likewise, this entire method (line 684 - 698) could move into
> FaviconHelper::DidChangeIcon ?
> 
> together these two changes will mean favicon_helper_ doesn't need to be passed
> into this class at all, giving a much clearer separation of the two.

Done.

https://codereview.chromium.org/11232068/diff/5002/chrome/renderer/favicon_he...
File chrome/renderer/favicon_helper.h (right):

https://codereview.chromium.org/11232068/diff/5002/chrome/renderer/favicon_he...
chrome/renderer/favicon_helper.h:25: // There is one FaviconHelper per
RenderView.
On 2012/10/30 14:41:45, joth wrote:
> perhaps also mention these instances are owned by the RenderView.

Done.

https://codereview.chromium.org/11232068/diff/5002/chrome/renderer/favicon_he...
chrome/renderer/favicon_helper.h:30: virtual ~FaviconHelper();
On 2012/10/30 14:41:45, joth wrote:
> d'tor can be private. (It gets deleted via OnDestruct)

Done.

[joth, 2012-10-30 20:22:57]

nice.
lgtm for our needs, but just one question...

https://codereview.chromium.org/11232068/diff/13001/chrome/renderer/favicon_h...
File chrome/renderer/favicon_helper.cc (right):

https://codereview.chromium.org/11232068/diff/13001/chrome/renderer/favicon_h...
chrome/renderer/favicon_helper.cc:81: routing_id(), id, image_url, false,
image_size, images));
I'm now looking at this wondering why we don't have an early  'return' here?

if data_image_failed == true, then then code below will go ahead and attempt to
download the favicon. (even though it's a data: URL so that makes no sense).
further if DownloadFavicon() fails we'll send a duplicate error response.


this would also allow |data_image_failed| to be pushed into the inner scope.
(or, just remove it as you can use data_image.empty() directly).

https://codereview.chromium.org/11232068/diff/13001/chrome/renderer/favicon_h...
File chrome/renderer/favicon_helper.h (right):

https://codereview.chromium.org/11232068/diff/13001/chrome/renderer/favicon_h...
chrome/renderer/favicon_helper.h:10: 
nit: remove \n here and line 26

[Cait (Slow), 2012-10-30 21:39:40]

https://codereview.chromium.org/11232068/diff/13001/chrome/renderer/favicon_h...
File chrome/renderer/favicon_helper.cc (right):

https://codereview.chromium.org/11232068/diff/13001/chrome/renderer/favicon_h...
chrome/renderer/favicon_helper.cc:81: routing_id(), id, image_url, false,
image_size, images));
On 2012/10/30 20:22:57, joth wrote:
> I'm now looking at this wondering why we don't have an early  'return' here?
> 
> if data_image_failed == true, then then code below will go ahead and attempt
to
> download the favicon. (even though it's a data: URL so that makes no sense).
> further if DownloadFavicon() fails we'll send a duplicate error response.
> 
> 
> this would also allow |data_image_failed| to be pushed into the inner scope.
> (or, just remove it as you can use data_image.empty() directly).
> 

Done.

https://codereview.chromium.org/11232068/diff/13001/chrome/renderer/favicon_h...
File chrome/renderer/favicon_helper.h (right):

https://codereview.chromium.org/11232068/diff/13001/chrome/renderer/favicon_h...
chrome/renderer/favicon_helper.h:10: 
On 2012/10/30 20:22:57, joth wrote:
> nit: remove \n here and line 26

Done.

[joth, 2012-10-30 22:14:02]

https://codereview.chromium.org/11232068/diff/18001/chrome/renderer/favicon_h...
File chrome/renderer/favicon_helper.cc (right):

https://codereview.chromium.org/11232068/diff/18001/chrome/renderer/favicon_h...
chrome/renderer/favicon_helper.cc:81: }
On 2012/10/30 21:39:40, caitkp wrote:
> On 2012/10/30 20:22:57, joth wrote:
> > I'm now looking at this wondering why we don't have an early  'return' here?
> > 
> > if data_image_failed == true, then then code below will go ahead and attempt
> to
> > download the favicon. (even though it's a data: URL so that makes no sense).
> > further if DownloadFavicon() fails we'll send a duplicate error response.
> > 
> > 
> > this would also allow |data_image_failed| to be pushed into the inner scope.
> > (or, just remove it as you can use data_image.empty() directly).
> > 
> 
> Done.

errr Sorry, I think I've introduced a new issue now (the triple negative is
doing my head in)...

so if scheme == "data" but the decode fails (data_image is empty) we'll still
fall-through here and try to download the data URL 

I think I find it easier if written like this (just one  Send call; early return
implies async completion), but maybe there's a cleaner option again?


std::vector<SkBitmap> result_images;
if (image_url.SchemeIs("data")) {
  data_image = imageFromDataUrl(url);
  if (!data_image.empty())
    result_images.push_back(data_image);
} else {
  if (DownloadFavicon(id, image_url, image_size)) {
    // Will complete asynchronously via FaviconHelper::DidDownloadFavicon
    return;
  }
}

Send(new IconHostMsg_DidDownloadFavicon(
     routing_id(), id, image_url, true, image_size,
     result_images);

[Cait (Slow), 2012-11-02 19:14:38]

https://codereview.chromium.org/11232068/diff/18001/chrome/renderer/favicon_h...
File chrome/renderer/favicon_helper.cc (right):

https://codereview.chromium.org/11232068/diff/18001/chrome/renderer/favicon_h...
chrome/renderer/favicon_helper.cc:81: }
Agree. This is easier to follow.

On 2012/10/30 22:14:02, joth wrote:
> On 2012/10/30 21:39:40, caitkp wrote:
> > On 2012/10/30 20:22:57, joth wrote:
> > > I'm now looking at this wondering why we don't have an early  'return'
here?
> > > 
> > > if data_image_failed == true, then then code below will go ahead and
attempt
> > to
> > > download the favicon. (even though it's a data: URL so that makes no
sense).
> > > further if DownloadFavicon() fails we'll send a duplicate error response.
> > > 
> > > 
> > > this would also allow |data_image_failed| to be pushed into the inner
scope.
> > > (or, just remove it as you can use data_image.empty() directly).
> > > 
> > 
> > Done.
> 
> errr Sorry, I think I've introduced a new issue now (the triple negative is
> doing my head in)...
> 
> so if scheme == "data" but the decode fails (data_image is empty) we'll still
> fall-through here and try to download the data URL 
> 
> I think I find it easier if written like this (just one  Send call; early
return
> implies async completion), but maybe there's a cleaner option again?
> 
> 
> std::vector<SkBitmap> result_images;
> if (image_url.SchemeIs("data")) {
>   data_image = imageFromDataUrl(url);
>   if (!data_image.empty())
>     result_images.push_back(data_image);
> } else {
>   if (DownloadFavicon(id, image_url, image_size)) {
>     // Will complete asynchronously via FaviconHelper::DidDownloadFavicon
>     return;
>   }
> }
> 
> Send(new IconHostMsg_DidDownloadFavicon(
>      routing_id(), id, image_url, true, image_size,
>      result_images);
> 
>

[Cait (Slow), 2012-11-09 17:05:11]

[+joi, for content/components insight]

Is the plan for this class to move into the content/components/ dir
(http://codereview.chromium.org/11273119/), or can it land here and be moved in
a future CL?

If I understand the relationship between chrome and content correctly, I think
there are several chrome/common deps which need to be broken (or moved) before
this code can be moved.

Thanks

[joth, 2012-11-09 17:24:47]

IMO this patch is a good step to land as it is, and will make the follow-up move
much simpler

(My understanding is this will be moved to content/renderer -- i.e. not be a
'component' at all but live fully inside the content API. But that's definitely
best handled as a separate step)

https://codereview.chromium.org/11232068/diff/20001/chrome/renderer/favicon_h...
File chrome/renderer/favicon_helper.cc (right):

https://codereview.chromium.org/11232068/diff/20001/chrome/renderer/favicon_h...
chrome/renderer/favicon_helper.cc:76: data_image = imageFromDataUrl(url);
|data_image| has not been declared

[Jói, 2012-11-09 18:37:15]

Agreed it should be a separate step.  Jonathan is right (or at least,
it matches my understanding) that this would move to content/ itself,
not to content/components.

Cheers,
Jói


On Fri, Nov 9, 2012 at 5:24 PM,  <joth@chromium.org> wrote:
> IMO this patch is a good step to land as it is, and will make the follow-up
> move
> much simpler
>
> (My understanding is this will be moved to content/renderer -- i.e. not be a
> 'component' at all but live fully inside the content API. But that's
> definitely
> best handled as a separate step)
>
>
>
>
https://codereview.chromium.org/11232068/diff/20001/chrome/renderer/favicon_h...
> File chrome/renderer/favicon_helper.cc (right):
>
>
https://codereview.chromium.org/11232068/diff/20001/chrome/renderer/favicon_h...
> chrome/renderer/favicon_helper.cc:76: data_image =
> imageFromDataUrl(url);
> |data_image| has not been declared
>
> https://codereview.chromium.org/11232068/

[Cait (Slow), 2012-11-14 20:25:55]

ping: sky: This CL should be ready for owners' approval now -- thanks!

https://codereview.chromium.org/11232068/diff/20001/chrome/renderer/favicon_h...
File chrome/renderer/favicon_helper.cc (right):

https://codereview.chromium.org/11232068/diff/20001/chrome/renderer/favicon_h...
chrome/renderer/favicon_helper.cc:76: data_image = imageFromDataUrl(url);
On 2012/11/09 17:24:47, joth wrote:
> |data_image| has not been declared

Done.

[sky, 2012-11-15 01:22:17]

LGTM with the following addressed

https://codereview.chromium.org/11232068/diff/24002/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/11232068/diff/24002/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:305: new
FaviconHelper(render_view);
Add a comment here that FaviconHelper deletes itself when render_view is
destroyed.

https://codereview.chromium.org/11232068/diff/24002/chrome/renderer/favicon_h...
File chrome/renderer/favicon_helper.cc (right):

https://codereview.chromium.org/11232068/diff/24002/chrome/renderer/favicon_h...
chrome/renderer/favicon_helper.cc:50: FaviconHelper::~FaviconHelper() {
Position should match header (many of the methods in here don't match).

https://codereview.chromium.org/11232068/diff/24002/chrome/renderer/favicon_h...
chrome/renderer/favicon_helper.cc:56: if (!urls.empty()) {
nit: no {}

[Cait (Slow), 2012-11-15 16:25:26]

https://codereview.chromium.org/11232068/diff/24002/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/11232068/diff/24002/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:305: new
FaviconHelper(render_view);
On 2012/11/15 01:22:17, sky wrote:
> Add a comment here that FaviconHelper deletes itself when render_view is
> destroyed.

Done.

https://codereview.chromium.org/11232068/diff/24002/chrome/renderer/favicon_h...
File chrome/renderer/favicon_helper.cc (right):

https://codereview.chromium.org/11232068/diff/24002/chrome/renderer/favicon_h...
chrome/renderer/favicon_helper.cc:50: FaviconHelper::~FaviconHelper() {
On 2012/11/15 01:22:17, sky wrote:
> Position should match header (many of the methods in here don't match).

Done.

https://codereview.chromium.org/11232068/diff/24002/chrome/renderer/favicon_h...
chrome/renderer/favicon_helper.cc:56: if (!urls.empty()) {
On 2012/11/15 01:22:17, sky wrote:
> nit: no {}

Done.

[commit-bot: I haz the power, 2012-11-15 16:26:25]

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

[commit-bot: I haz the power, 2012-11-15 18:25:00]

Change committed as 167965