Get web app icon via FavIconHelper and auto repair/update

Get web app icon via FavIconHelper and auto repair/update

- Expose a DownloadImage method from FavIconHelper to download/decode
  image for an image url;
- Expose FavIconHelper from TabContents;
- Update CreateApplicationShortcutView to use the exposed DownloadImage
  method to get web app icon instead of do it via URLFetcher/PNGCodec;
- Check and update web app icon and shortcuts when chrome is lauched as
  app for OS_WIN;
- Code cleanup:
  - Move a bunch of FavIconHelper methods that are not used externally
    to private;
  - Remove an unused cancelable_consumer_ from TabContents;

BUG=8539
TEST=Verify issue 8539 is fixed. And create a web page with a non PNG shortcut icon and verify it shows up in create application shortcut dialog.

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

[Miranda Callahan, 2009-12-09 23:41:35]

This is quite a patch -- great work!  See comments; mostly nits, but a few
substantial questions as well.

http://codereview.chromium.org/482003/diff/1/2
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/482003/diff/1/2#newcode352
chrome/browser/browser.cc:352: // shortcuts when web app info is available.
I had to read this sentence a couple of times to understand what it means, and
I'm still not sure; perhaps a slight rewording makes it clearer (or not, in case
I haven't understood what's going on correctly):

Set UPDATE_SHORTCUT as the pending web app action. This action is picked up in
LoadingStateChanged to schedule a call to GetApplicationInfo, which updates
shortcuts when web app info is available.

http://codereview.chromium.org/482003/diff/1/2#newcode2009
chrome/browser/browser.cc:2009: return;
What happens to the UPDATE_SHORTCUT pending action if we find no entry and
return here?  Will it get called again later?  Or is there then a danger that we
might fail the DCHECK up in line 1184 later on?

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

http://codereview.chromium.org/482003/diff/1/3#newcode831
chrome/browser/browser.h:831: UPDATE_SHORTCUT   // Update icon for app shortcut
Please put periods at the end of each comment (as above in
CancelDownloadConfirmationState).

http://codereview.chromium.org/482003/diff/1/3#newcode833
chrome/browser/browser.h:833: WebAppAction pending_web_app_action_;
Please add a comment above this line explaining the pending_web_app_action_
variable.

http://codereview.chromium.org/482003/diff/1/4
File chrome/browser/fav_icon_helper.cc (right):

http://codereview.chromium.org/482003/diff/1/4#newcode270
chrome/browser/fav_icon_helper.cc:270: return download_id;  // Download request
failed.
Since you're always returning download_id, could you remove these two lines, and
enclose the next three lines in 
"if (download_id) { ... }" instead?  But also: is this return value ever used?

http://codereview.chromium.org/482003/diff/1/5
File chrome/browser/fav_icon_helper.h (right):

http://codereview.chromium.org/482003/diff/1/5#newcode77
chrome/browser/fav_icon_helper.h:77: // When download completes, |callback| is
called with the download_id,
Don't start a new line here, just continue the paragraph.

http://codereview.chromium.org/482003/diff/1/5#newcode79
chrome/browser/fav_icon_helper.h:79: // downloaded image.
Not sure what "whether" means with regard to "downloaded image"; do you mean "a
boolean flag to indicate whether the download succeeded and we have downloaded a
valid image"?

http://codereview.chromium.org/482003/diff/1/5#newcode147
chrome/browser/fav_icon_helper.h:147: const GURL& icon_url,
Do what you did above in ScheduleDownload, and put all the arguments that will
fit on the same line (move up icon_url, I think).

http://codereview.chromium.org/482003/diff/1/8
File chrome/browser/views/create_application_shortcut_view.cc (right):

http://codereview.chromium.org/482003/diff/1/8#newcode213
chrome/browser/views/create_application_shortcut_view.cc:213: owner_ = NULL;
Do we leak in this case, since we explicitly "delete this" in the "Run" case?

http://codereview.chromium.org/482003/diff/1/8#newcode245
chrome/browser/views/create_application_shortcut_view.cc:245:
shortcut_info_.description,
can these three lines be made into two?

http://codereview.chromium.org/482003/diff/1/9
File chrome/browser/views/create_application_shortcut_view.h (right):

http://codereview.chromium.org/482003/diff/1/9#newcode80
chrome/browser/views/create_application_shortcut_view.h:80: // Target url of the
app shortcut.
Is this still a url? Should comment be changed?

http://codereview.chromium.org/482003/diff/1/9#newcode87
chrome/browser/views/create_application_shortcut_view.h:87: class
IconDownloadCallbackFunctor;
I think this class forward declaration should go up around line 26 with the
other forward declarations...?

http://codereview.chromium.org/482003/diff/1/10
File chrome/browser/web_applications/web_app.cc (right):

http://codereview.chromium.org/482003/diff/1/10#newcode26
chrome/browser/web_applications/web_app.cc:26: #include <ShellAPI.h>
I think this include needs to be up above the project-specific files, with a
separate #if defined(OS_WIN).

http://codereview.chromium.org/482003/diff/1/10#newcode103
chrome/browser/web_applications/web_app.cc:103: // Predicator for soring image
from largest to smallest.
// Predicate for sorting ...

http://codereview.chromium.org/482003/diff/1/10#newcode164
chrome/browser/web_applications/web_app.cc:164: // a better way to achieve this.
Do you think this is disruptive enough that a bug should be filed on it?

http://codereview.chromium.org/482003/diff/1/10#newcode667
chrome/browser/web_applications/web_app.cc:667: DCHECK(info);  // Must provider
a valid info.
"Must provide"

http://codereview.chromium.org/482003/diff/1/11
File chrome/browser/web_applications/web_app.h (right):

http://codereview.chromium.org/482003/diff/1/11#newcode55
chrome/browser/web_applications/web_app.h:55: // shortcut and would only update
(re-create) them if they exits.
nit: "shortcut and only updates (recreates) them if they exit."

[xiyuan, 2009-12-10 01:00:27]

CL updated based on your comments. Please take another look. Thanks.

http://codereview.chromium.org/482003/diff/1/2
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/482003/diff/1/2#newcode352
chrome/browser/browser.cc:352: // shortcuts when web app info is available.
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> I had to read this sentence a couple of times to understand what it means, and
> I'm still not sure; perhaps a slight rewording makes it clearer (or not, in
case
> I haven't understood what's going on correctly):
> 
> Set UPDATE_SHORTCUT as the pending web app action. This action is picked up in
> LoadingStateChanged to schedule a call to GetApplicationInfo, which updates
> shortcuts when web app info is available.

Updated and hopefully this time it is more clear. Sorry for my bad English. :)

http://codereview.chromium.org/482003/diff/1/2#newcode2009
chrome/browser/browser.cc:2009: return;
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> What happens to the UPDATE_SHORTCUT pending action if we find no entry and
> return here?  Will it get called again later?  Or is there then a danger that
we
> might fail the DCHECK up in line 1184 later on?
Good point. When this happens, the pending_web_app_action_ is not cleared and
the next page load would trigger the logic and we might end up updating the
wrong shortcuts.

Updated the code to reset pending_web_app_action_ to NONE and fire a NOTREACHED
assert.

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

http://codereview.chromium.org/482003/diff/1/3#newcode831
chrome/browser/browser.h:831: UPDATE_SHORTCUT   // Update icon for app shortcut
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> Please put periods at the end of each comment (as above in
> CancelDownloadConfirmationState).

Done.

http://codereview.chromium.org/482003/diff/1/3#newcode833
chrome/browser/browser.h:833: WebAppAction pending_web_app_action_;
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> Please add a comment above this line explaining the pending_web_app_action_
> variable.

Done.

http://codereview.chromium.org/482003/diff/1/4
File chrome/browser/fav_icon_helper.cc (right):

http://codereview.chromium.org/482003/diff/1/4#newcode270
chrome/browser/fav_icon_helper.cc:270: return download_id;  // Download request
failed.
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> Since you're always returning download_id, could you remove these two lines,
and
> enclose the next three lines in 
> "if (download_id) { ... }" instead?  But also: is this return value ever used?


Done.

The download id is for caller to track its image download requests and passed in
as first param of the image download callback.

It is not used right now in both CreateApplicationShortcutView and
UpdateShortcutWorker because they only have one pending image download at a
time. But if caller wants to have multiple downloads at the same time, then it
needs some way to identify them. We could also use image url but think download
id is cheaper to use.

http://codereview.chromium.org/482003/diff/1/5
File chrome/browser/fav_icon_helper.h (right):

http://codereview.chromium.org/482003/diff/1/5#newcode77
chrome/browser/fav_icon_helper.h:77: // When download completes, |callback| is
called with the download_id,
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> Don't start a new line here, just continue the paragraph.

Done.

http://codereview.chromium.org/482003/diff/1/5#newcode79
chrome/browser/fav_icon_helper.h:79: // downloaded image.
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> Not sure what "whether" means with regard to "downloaded image"; do you mean
"a
> boolean flag to indicate whether the download succeeded and we have downloaded
a
> valid image"?

the "and downloaded image" is the third param of the callback. Rephrased as
follows:

When download completes, |callback| is called with the three params: the
download_id, a boolean flag to indicate whether the download succeeds and a
SkBitmap as the downloaded image.

http://codereview.chromium.org/482003/diff/1/5#newcode147
chrome/browser/fav_icon_helper.h:147: const GURL& icon_url,
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> Do what you did above in ScheduleDownload, and put all the arguments that will
> fit on the same line (move up icon_url, I think).

Done. All three params fit in one line.

http://codereview.chromium.org/482003/diff/1/8
File chrome/browser/views/create_application_shortcut_view.cc (right):

http://codereview.chromium.org/482003/diff/1/8#newcode213
chrome/browser/views/create_application_shortcut_view.cc:213: owner_ = NULL;
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> Do we leak in this case, since we explicitly "delete this" in the "Run" case?

This part is a bit tricky. IconDownloadCallbackFunctor is used for icon download
callback. Its Run method is always called when download finishes or fails. The
Cancel method decouples CreateApplicationShortcutView with the pending icon
download when the view is dismissed/released before the icon download finishes.
Thus, when callback happens, it knows the owner view is gone and just release
itself.

http://codereview.chromium.org/482003/diff/1/8#newcode245
chrome/browser/views/create_application_shortcut_view.cc:245:
shortcut_info_.description,
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> can these three lines be made into two?

Done.

http://codereview.chromium.org/482003/diff/1/9
File chrome/browser/views/create_application_shortcut_view.h (right):

http://codereview.chromium.org/482003/diff/1/9#newcode80
chrome/browser/views/create_application_shortcut_view.h:80: // Target url of the
app shortcut.
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> Is this still a url? Should comment be changed?

Done.

http://codereview.chromium.org/482003/diff/1/9#newcode87
chrome/browser/views/create_application_shortcut_view.h:87: class
IconDownloadCallbackFunctor;
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> I think this class forward declaration should go up around line 26 with the
> other forward declarations...?
This IconDownloadCallbackFunctor is intended to use only with this class and
putting it here would ensure that.

http://codereview.chromium.org/482003/diff/1/10
File chrome/browser/web_applications/web_app.cc (right):

http://codereview.chromium.org/482003/diff/1/10#newcode26
chrome/browser/web_applications/web_app.cc:26: #include <ShellAPI.h>
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> I think this include needs to be up above the project-specific files, with a
> separate #if defined(OS_WIN).

Done.

http://codereview.chromium.org/482003/diff/1/10#newcode103
chrome/browser/web_applications/web_app.cc:103: // Predicator for soring image
from largest to smallest.
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> // Predicate for sorting ...

Done.

http://codereview.chromium.org/482003/diff/1/10#newcode164
chrome/browser/web_applications/web_app.cc:164: // a better way to achieve this.
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> Do you think this is disruptive enough that a bug should be filed on it?
It should be okay since we are only doing it when necessary (icon missing or
changed on server).

http://codereview.chromium.org/482003/diff/1/10#newcode667
chrome/browser/web_applications/web_app.cc:667: DCHECK(info);  // Must provider
a valid info.
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> "Must provide"

Done.

http://codereview.chromium.org/482003/diff/1/11
File chrome/browser/web_applications/web_app.h (right):

http://codereview.chromium.org/482003/diff/1/11#newcode55
chrome/browser/web_applications/web_app.h:55: // shortcut and would only update
(re-create) them if they exits.
On 2009/12/09 23:41:35, Miranda Callahan wrote:
> nit: "shortcut and only updates (recreates) them if they exit."

Done.

[Miranda Callahan, 2009-12-10 01:07:47]

LGTM, great work.