Fix Notifications Icon loading.

Fix Notifications Icon loading.

The recent change to loading favicons regressed the loading regular icons that used the same codepath (https://chromiumcodereview.appspot.com/12218057)

The fix is to add a parameter to WebContent::DownloadFavicon(...) to differentiate the loading of favicon and regular images.

There is a separate patch, for renaming DownloadFavicon -> DownloadImage (https://codereview.chromium.org/12780024/)

TBR=palmer@chromium.org
BUG=196769
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189349

[joth, 2013-03-19 22:44:31]

a_w/.. lgtm

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

https://codereview.chromium.org/12945007/diff/1/content/public/browser/web_co...
content/public/browser/web_contents.h:427: // |image_size| is a hint for images
with multiple sizes. The downloaded image
 |image_type| ...

[brettw, 2013-03-19 22:50:13]

I'd like the enum to be a bit more unique, maybe like DOWNLOAD_IMAGE_* to match
the enum type name?

John can hopefully suggest a better place to put the actual enum declaration.

https://codereview.chromium.org/12945007/diff/1/content/public/common/content...
File content/public/common/content_constants.h (right):

https://codereview.chromium.org/12945007/diff/1/content/public/common/content...
content/public/common/content_constants.h:63: DOWNLOAD_OTHER,    // Regular
image loading/decoding.
I think this should be indented 2 spaces.

[Cait (Slow), 2013-03-20 00:04:56]

lgtm from a favicon loading pov. I agree with Brett that a more descriptive name
for the enum would be good though.

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

https://codereview.chromium.org/12945007/diff/1/content/public/browser/web_co...
content/public/browser/web_contents.h:429: // the image is returned.
please add a note explaining the purpose of the DownloadImageType param here/

[jam, 2013-03-20 00:07:57]

On 2013/03/19 22:50:13, brettw wrote:
> I'd like the enum to be a bit more unique, maybe like DOWNLOAD_IMAGE_* to
match
> the enum type name?
> 
> John can hopefully suggest a better place to put the actual enum declaration.

I understand sometimes people like to use enum for two-valued states because
it's more readable. For Content API, we put each enum in a separate file. Since
this will only be two states, can we just skip an enum and have a boolean
argument to DownloadFavicon? i.e.


// |is_favicon| is true if the image being downloaded is a favicon, or false if
it's just an image. The difference is favicon loading doesn't use cookies.
virtual int DownloadFavicon(const GURL& url,
                            bool is_favicon,
                            int image_size,
                            const FaviconDownloadCallback& callback) = 0;

[Dmitry Titov, 2013-03-20 01:10:07]

I followed John's advice and replaces enum with a bool. Worked fine and looks
simpler :-)

Also addressed other comments as well!

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

https://codereview.chromium.org/12945007/diff/1/content/public/browser/web_co...
content/public/browser/web_contents.h:427: // |image_size| is a hint for images
with multiple sizes. The downloaded image
On 2013/03/19 22:44:31, joth wrote:
>  |image_type| ...

Done.

https://codereview.chromium.org/12945007/diff/1/content/public/browser/web_co...
content/public/browser/web_contents.h:429: // the image is returned.
On 2013/03/20 00:04:56, caitkp wrote:
> please add a note explaining the purpose of the DownloadImageType param here/

Done.

https://codereview.chromium.org/12945007/diff/1/content/public/common/content...
File content/public/common/content_constants.h (right):

https://codereview.chromium.org/12945007/diff/1/content/public/common/content...
content/public/common/content_constants.h:63: DOWNLOAD_OTHER,    // Regular
image loading/decoding.
On 2013/03/19 22:50:14, brettw wrote:
> I think this should be indented 2 spaces.

Removed the enum per John's suggestion.

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

John or Brett (only need one of you) - PTAL?

[jam, 2013-03-20 02:44:41]

lgtm

https://codereview.chromium.org/12945007/diff/12001/android_webview/browser/i...
File android_webview/browser/icon_helper.cc (right):

https://codereview.chromium.org/12945007/diff/12001/android_webview/browser/i...
android_webview/browser/icon_helper.cc:63: true,  // favicon
ditto

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_tab_helper.cc:137: true,  // favicon
ditto

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.cc:311: false, 
// not a favicon
ditto

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc (right):

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc:112:
web_contents_->DownloadFavicon(*iter, true /* favicon */, 0,
ditto

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/shell_window.cc:362: true,  // favicon
ditto

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/ui/metro_p...
File chrome/browser/ui/metro_pin_tab_helper_win.cc (right):

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/ui/metro_p...
chrome/browser/ui/metro_pin_tab_helper_win.cc:404: true,  // favicon
ditto

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/ash/balloon_view_ash.cc (right):

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/balloon_view_ash.cc:93: false,  // not a favicon
ditto

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/ui/views/c...
chrome/browser/ui/views/create_application_shortcut_view.cc:495: true,  //
favicon
ditto

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/ui/web_app...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://codereview.chromium.org/12945007/diff/12001/chrome/browser/ui/web_app...
chrome/browser/ui/web_applications/web_app_ui.cc:175: true,  // favicon
nit: it seems if someone wants to see what this is, they can look at the header

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

https://codereview.chromium.org/12945007/diff/12001/content/renderer/favicon_...
content/renderer/favicon_helper.h:55: // not send and not accepted during
download.
nit: s/send/sent

[Dmitry Titov, 2013-03-20 04:51:57]

Removed comments, fixed typo. Added TBR=palmer@ for icon_messages.h change.
Landing.

[commit-bot: I haz the power, 2013-03-20 04:52:16]

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

[commit-bot: I haz the power, 2013-03-20 06:32:53]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-20 06:38:18]

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

[commit-bot: I haz the power, 2013-03-20 14:08:10]

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

[commit-bot: I haz the power, 2013-03-20 17:12:10]

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

Patch:       chrome/browser/ui/extensions/shell_window.cc
Index: chrome/browser/ui/extensions/shell_window.cc
diff --git a/chrome/browser/ui/extensions/shell_window.cc
b/chrome/browser/ui/extensions/shell_window.cc
index
ade284ac01765dc9b3b300e66f1b4c9b9f007877..b0efa69e94a6d48392ff7f29ff4165c96475ced5
100644
--- a/chrome/browser/ui/extensions/shell_window.cc
+++ b/chrome/browser/ui/extensions/shell_window.cc
@@ -358,7 +358,9 @@ string16 ShellWindow::GetTitle() const {
 
 void ShellWindow::SetAppIconUrl(const GURL& url) {
   app_icon_url_ = url;
-  web_contents()->DownloadFavicon(url, kPreferredIconSize,
+  web_contents()->DownloadFavicon(url,
+                                  true,
+                                  kPreferredIconSize,
                                   base::Bind(&ShellWindow::DidDownloadFavicon,
                                              weak_ptr_factory_.GetWeakPtr()));
 }

[Dmitry Titov, 2013-03-20 18:27:43]

Committed patchset #4 manually as r189349 (presubmit successful).

[palmer, 2013-03-20 19:08:43]

LGTM