Split FaviconHelper in two: ImageLoadingHelper and FaviconHelper

Split FaviconHelper in two: ImageLoadingHelper and FaviconHelper.

This is the first step of fixing http://crbug.com/196769.

The problem is that there are two types of users of Content::DownloadFavicon() - the favicon consumers and regular icons and images consumers. Recently, the otherwise-generic image download+decoding implementation regressed because it was assumed to be only used for favicon loading. 

The proposed fix is to add a parameter to the method (enum, FAVICON/IMAGE) and rename it from DownloadFavicon to DownloadImage, to make sure the name corresponds to the impl/usage.

This is rename-only part, no additional parameter yet. The FaviconHelper class was split in 2, with new ImageLoadingHelper dealing with images. 


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

[Dmitry Titov, 2013-03-19 02:15:20]

John, I think you may be the best reviewer here... If not, please advice.

[Dmitry Titov, 2013-03-19 18:19:38]

caitkp@ - could you take a look at FaviconHelper changes
brettw@ - could you please do OWNERS look?

[jam, 2013-03-19 20:12:21]

(saw this go by so I have a few mostly nits). brettw can do the review or i can,
up to you

https://codereview.chromium.org/12780024/diff/20001/content/common/icon_messa...
File content/common/icon_messages.h (right):

https://codereview.chromium.org/12780024/diff/20001/content/common/icon_messa...
content/common/icon_messages.h:29: std::vector<content::FaviconURL> /* urls of
the favicon */)
seems too much to have one messages file for one message. especially if we move
the favicon file in content\renderer into renderviewimpl and that this message
is dispatched in WebContentsImpl, this should just move to view_messages

https://codereview.chromium.org/12780024/diff/20001/content/public/browser/we...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/12780024/diff/20001/content/public/browser/we...
content/public/browser/web_contents.h:423: // Sends a request to download the
given favicon |url| and returns the unique
nit: update comment

https://codereview.chromium.org/12780024/diff/20001/content/renderer/favicon_...
File content/renderer/favicon_helper.h (right):

https://codereview.chromium.org/12780024/diff/20001/content/renderer/favicon_...
content/renderer/favicon_helper.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
it seems that this file is now really small and these two methods should just
move to RenderViewImpl. It was originally split off because this favicon code
was in src\chrome, but then we moved it to content after reconsideration.

https://codereview.chromium.org/12780024/diff/20001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/12780024/diff/20001/content/renderer/render_v...
content/renderer/render_view_impl.h:1448: ImageLoadingHelper*
image_loading_helper_;
no need to keep a pointer in this class since it's not being used. just create
it like other helpers and it'll delete itself when the RV is gone

[Dmitry Titov, 2013-03-20 01:30:21]

Thanks! Updated, PTAL.

https://codereview.chromium.org/12780024/diff/20001/content/common/icon_messa...
File content/common/icon_messages.h (right):

https://codereview.chromium.org/12780024/diff/20001/content/common/icon_messa...
content/common/icon_messages.h:29: std::vector<content::FaviconURL> /* urls of
the favicon */)
On 2013/03/19 20:12:21, jam wrote:
> seems too much to have one messages file for one message. especially if we
move
> the favicon file in content\renderer into renderviewimpl and that this message
> is dispatched in WebContentsImpl, this should just move to view_messages

moved to view_messages, removed FaviconHelper class.

https://codereview.chromium.org/12780024/diff/20001/content/public/browser/we...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/12780024/diff/20001/content/public/browser/we...
content/public/browser/web_contents.h:423: // Sends a request to download the
given favicon |url| and returns the unique
On 2013/03/19 20:12:21, jam wrote:
> nit: update comment

Done.

https://codereview.chromium.org/12780024/diff/20001/content/renderer/favicon_...
File content/renderer/favicon_helper.h (right):

https://codereview.chromium.org/12780024/diff/20001/content/renderer/favicon_...
content/renderer/favicon_helper.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2013/03/19 20:12:21, jam wrote:
> it seems that this file is now really small and these two methods should just
> move to RenderViewImpl. It was originally split off because this favicon code
> was in src\chrome, but then we moved it to content after reconsideration.

Done, the code moved to RVImpl, removed the files.

https://codereview.chromium.org/12780024/diff/20001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/12780024/diff/20001/content/renderer/render_v...
content/renderer/render_view_impl.h:1448: ImageLoadingHelper*
image_loading_helper_;
On 2013/03/19 20:12:21, jam wrote:
> no need to keep a pointer in this class since it's not being used. just create
> it like other helpers and it'll delete itself when the RV is gone

Done.

[jam, 2013-03-20 23:25:19]

qq: it's a bit hard to review the new file since you didn't use svn copy. can
you do that instead?

[brettw, 2013-03-20 23:25:56]

lgtm

https://codereview.chromium.org/12780024/diff/32001/content/renderer/image_lo...
File content/renderer/image_loading_helper.cc (right):

https://codereview.chromium.org/12780024/diff/32001/content/renderer/image_lo...
content/renderer/image_loading_helper.cc:42: if (image_url.SchemeIs("data")) {
Can you use kDataScheme from content/public/common/url_constants.h?

https://codereview.chromium.org/12780024/diff/32001/content/renderer/image_lo...
File content/renderer/image_loading_helper.h (right):

https://codereview.chromium.org/12780024/diff/32001/content/renderer/image_lo...
content/renderer/image_loading_helper.h:23: // This class deals with favicon
downloading.
Is this comment still up-to-date?

https://codereview.chromium.org/12780024/diff/32001/content/renderer/image_lo...
content/renderer/image_loading_helper.h:59: ImageResourceFetcherList;
4 space-indents for wrapped lines.

https://codereview.chromium.org/12780024/diff/32001/content/renderer/image_lo...
content/renderer/image_loading_helper.h:67: } // namespace content
2 spaces before //

[jam, 2013-03-21 01:14:51]

some nits

https://codereview.chromium.org/12780024/diff/32001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/12780024/diff/32001/content/common/view_messa...
content/common/view_messages.h:2296: std::vector<content::FaviconURL> /* urls of
the favicon */)
nit: convention is that the comment contains the parameter name in the
dispatching method

https://codereview.chromium.org/12780024/diff/32001/content/public/common/fav...
File content/public/common/favicon_url.h (right):

https://codereview.chromium.org/12780024/diff/32001/content/public/common/fav...
content/public/common/favicon_url.h:28: static IconType
ToFaviconType(WebKit::WebIconURL::Type type);
this is only called in render_view_impl.cc. so no need to put this method in the
public api. keep it as a static method in render_view_impl.cc

https://codereview.chromium.org/12780024/diff/32001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/12780024/diff/32001/content/renderer/render_v...
content/renderer/render_view_impl.h:173: struct FaviconURL;
nit: order

[Dmitry Titov, 2013-03-27 22:41:13]

I've used "git mv" this time, but it didn't upload as D file and A+ file,
instead D and A, not sure why... 

Done with all the comments and nits. jam@ and caitkp@, it's ready for another
look.

https://codereview.chromium.org/12780024/diff/32001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/12780024/diff/32001/content/common/view_messa...
content/common/view_messages.h:2296: std::vector<content::FaviconURL> /* urls of
the favicon */)
On 2013/03/21 01:14:51, jam wrote:
> nit: convention is that the comment contains the parameter name in the
> dispatching method

Done.

https://codereview.chromium.org/12780024/diff/32001/content/public/common/fav...
File content/public/common/favicon_url.h (right):

https://codereview.chromium.org/12780024/diff/32001/content/public/common/fav...
content/public/common/favicon_url.h:28: static IconType
ToFaviconType(WebKit::WebIconURL::Type type);
On 2013/03/21 01:14:51, jam wrote:
> this is only called in render_view_impl.cc. so no need to put this method in
the
> public api. keep it as a static method in render_view_impl.cc

Done.

https://codereview.chromium.org/12780024/diff/32001/content/renderer/image_lo...
File content/renderer/image_loading_helper.cc (right):

https://codereview.chromium.org/12780024/diff/32001/content/renderer/image_lo...
content/renderer/image_loading_helper.cc:42: if (image_url.SchemeIs("data")) {
On 2013/03/20 23:25:56, brettw wrote:
> Can you use kDataScheme from content/public/common/url_constants.h?

Done.

https://codereview.chromium.org/12780024/diff/32001/content/renderer/image_lo...
File content/renderer/image_loading_helper.h (right):

https://codereview.chromium.org/12780024/diff/32001/content/renderer/image_lo...
content/renderer/image_loading_helper.h:23: // This class deals with favicon
downloading.
On 2013/03/20 23:25:56, brettw wrote:
> Is this comment still up-to-date?

Done.

https://codereview.chromium.org/12780024/diff/32001/content/renderer/image_lo...
content/renderer/image_loading_helper.h:59: ImageResourceFetcherList;
On 2013/03/20 23:25:56, brettw wrote:
> 4 space-indents for wrapped lines.

Done.

https://codereview.chromium.org/12780024/diff/32001/content/renderer/image_lo...
content/renderer/image_loading_helper.h:67: } // namespace content
On 2013/03/20 23:25:56, brettw wrote:
> 2 spaces before //

Done.

https://codereview.chromium.org/12780024/diff/32001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/12780024/diff/32001/content/renderer/render_v...
content/renderer/render_view_impl.h:173: struct FaviconURL;
On 2013/03/21 01:14:51, jam wrote:
> nit: order

Done.

[jam, 2013-03-27 23:24:42]

On 2013/03/27 22:41:13, Dmitry Titov wrote:
> I've used "git mv" this time, but it didn't upload as D file and A+ file,
> instead D and A, not sure why... 

I'm not that familiar with git. I know there are different thresholds that can
be tweaked.

Please try to do fix this. If you can't do it in git, then with svn. I don't
think we should lose history.

[Dmitry Titov, 2013-03-27 23:39:29]

Aga, "git cl upload --similarity=20" worked! 
"similarity" sets the percent for files to be identical to be considered copies.

[Cait (Slow), 2013-03-28 19:10:17]

LGTM (with nits)

https://codereview.chromium.org/12780024/diff/47001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_util.cc (right):

https://codereview.chromium.org/12780024/diff/47001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_util.cc:11: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebIconURL.h"
needed?

https://codereview.chromium.org/12780024/diff/47001/content/common/image_mess...
File content/common/image_messages.h (right):

https://codereview.chromium.org/12780024/diff/47001/content/common/image_mess...
content/common/image_messages.h:22: bool /* is favicon (turn off ciikies) */,
nit: typo ciikies --> cookies

https://codereview.chromium.org/12780024/diff/47001/content/common/image_mess...
content/common/image_messages.h:34: 
nit: remove empty line

https://codereview.chromium.org/12780024/diff/47001/content/public/browser/we...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/12780024/diff/47001/content/public/browser/we...
content/public/browser/web_contents.h:419: // be called with the bitmaps
received from the renderer. If [is_favicon|,
nit: [is_favicon| --> |is_favicon|
maybe also add "is true" for clarity

https://codereview.chromium.org/12780024/diff/47001/content/public/browser/we...
content/public/browser/web_contents.h:420: // the cookeis are not sent and not
accepted during download. Note that
nit: cookeis--> cookies

https://codereview.chromium.org/12780024/diff/47001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/12780024/diff/47001/content/renderer/render_v...
content/renderer/render_view_impl.h:1188: void SendUpdateFaviconURL(const
std::vector<FaviconURL>& urls);
Please add documentation for these 2 methods.

[Dmitry Titov, 2013-03-28 22:52:40]

Thanks Cait!

https://codereview.chromium.org/12780024/diff/47001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_util.cc (right):

https://codereview.chromium.org/12780024/diff/47001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_util.cc:11: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebIconURL.h"
On 2013/03/28 19:10:18, caitkp wrote:
> needed?

Done.

https://codereview.chromium.org/12780024/diff/47001/content/common/image_mess...
File content/common/image_messages.h (right):

https://codereview.chromium.org/12780024/diff/47001/content/common/image_mess...
content/common/image_messages.h:22: bool /* is favicon (turn off ciikies) */,
On 2013/03/28 19:10:18, caitkp wrote:
> nit: typo ciikies --> cookies

Done.

https://codereview.chromium.org/12780024/diff/47001/content/common/image_mess...
content/common/image_messages.h:34: 
On 2013/03/28 19:10:18, caitkp wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/12780024/diff/47001/content/public/browser/we...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/12780024/diff/47001/content/public/browser/we...
content/public/browser/web_contents.h:419: // be called with the bitmaps
received from the renderer. If [is_favicon|,
On 2013/03/28 19:10:18, caitkp wrote:
> nit: [is_favicon| --> |is_favicon|
> maybe also add "is true" for clarity

Done.

https://codereview.chromium.org/12780024/diff/47001/content/public/browser/we...
content/public/browser/web_contents.h:420: // the cookeis are not sent and not
accepted during download. Note that
On 2013/03/28 19:10:18, caitkp wrote:
> nit: cookeis--> cookies

Done.

https://codereview.chromium.org/12780024/diff/47001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/12780024/diff/47001/content/renderer/render_v...
content/renderer/render_view_impl.h:1188: void SendUpdateFaviconURL(const
std::vector<FaviconURL>& urls);
On 2013/03/28 19:10:18, caitkp wrote:
> Please add documentation for these 2 methods.

Done.

[Dmitry Titov, 2013-03-28 23:00:43]

Julien, contents/common/OWNERS says you can look at *_message.h files.

Here, I move 3 messages from icon_messages.h into view_messages.h and
image_messages.h, just refactoring. 

Could you take a look?

[jln (very slow on Chromium), 2013-03-28 23:33:39]

On 2013/03/28 23:00:43, Dmitry Titov wrote:
> Julien, contents/common/OWNERS says you can look at *_message.h files.
> 
> Here, I move 3 messages from icon_messages.h into view_messages.h and
> image_messages.h, just refactoring. 
> 
> Could you take a look?

*_messages lgtm

[jam, 2013-03-29 00:26:53]

lgtm

[Dmitry Titov, 2013-03-29 00:50:13]

TBR Jonathan, for an OWNER look at the android_webview file in this patch.
It's an API renaming change.

Thanks!

[commit-bot: I haz the power, 2013-03-29 00:52:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dimich@chromium.org/12780024/56001

[commit-bot: I haz the power, 2013-03-29 00:52:26]

Presubmit check for 12780024-56001 failed and returned exit status 1.

INFO:root:Found 23 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/views/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    android_webview/browser/icon_helper.cc

Presubmit checks took 2.6s to calculate.

[joth, 2013-03-29 00:58:12]

lgtm

[joth, 2013-03-29 00:58:15]

lgtm

lgtm

[commit-bot: I haz the power, 2013-03-29 01:00:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dimich@chromium.org/12780024/56001

[commit-bot: I haz the power, 2013-03-29 05:32:05]

Change committed as 191304