Use high resolution icons where possible for streamlined hosted app icons.

chrome/browser/ui/web_applications/hosted_app_tab_helper.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/web_applications/hosted_app_tab_helper.h"
+
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/extensions/crx_installer.h"
+#include "chrome/browser/favicon/favicon_tab_helper.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/web_applications/hosted_app_tab_helper_delegate.h"
+#include "chrome/common/web_application_info.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/common/favicon_url.h"
+
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(HostedAppTabHelper);
+
+HostedAppTabHelper::HostedAppTabHelper(content::WebContents* web_contents)

[tapted, 2013/11/11 08:52:57]

initialize delegate_ to NULL

[calamity, 2013/11/13 06:37:14]

Done.

+    : content::WebContentsObserver(web_contents),
+      fetch_icons_immediately_(false) {
+}
+
+HostedAppTabHelper::~HostedAppTabHelper() {
+}
+
+void HostedAppTabHelper::CreateHostedApp(
+    const WebApplicationInfo& web_app_info) {
+  // If a hosted app creation request is already pending, ignore subsequent
+  // requests.
+  if (web_app_info_)
+    return;
+
+  web_app_info_.reset(new WebApplicationInfo());

[tapted, 2013/11/11 08:52:57]

nit: pretty sure you can still call the copy constructor here. I.e. do 

web_app_info_.reset(new WebApplicationInfo(web_app_info));

rather than the line below

[calamity, 2013/11/13 06:37:14]

Done.

+  *web_app_info_ = web_app_info;
+  // It's possible for |favicon_url_candidates_| to be populated after this is
+  // called. DidUpdateFaviconURL() will resume the app creation process in this
+  // case.
+  if (!favicon_url_candidates_)

[tapted, 2013/11/11 08:52:57]

Could this just be `if (favicon_url_candidates.empty())`, and ditch the
scoped_ptr on it?

[calamity, 2013/11/13 06:37:14]

I wanted it as a scoped_ptr so that its existence would be a signal for whether
DidUpdateFaviconURLs had been called. If there are no favicon URLs, this will
return here incorrectly.

+    return;
+
+  FetchIconsIfNecessaryForCreateHostedApp();
+}
+
+void HostedAppTabHelper::FetchIcons() {
+  for (std::set<GURL>::const_iterator iter = favicon_url_candidates_->begin();
+       iter != favicon_url_candidates_->end(); ++iter) {
+    in_progress_requests_.insert(

[tapted, 2013/11/11 08:52:57]

Maybe make these 7 lines a helper function `DownloadURL(const GURL& url)`?,
since it's repeated below with different formatting.

Also is Unretained safe here? Haven't played much with WebContentsUserData, but
it looks like the lifetime is bound to the WebContents. NetErrorTabHelper uses
weak_factory_.GetWeakPtr(), but there are others that seem content with
Unretained too.

[calamity, 2013/11/13 06:37:14]

Done.

[tapted, 2013/11/13 07:44:53]

Did you check up on the weakptr vs unretained goo?

+        web_contents()->DownloadImage(
+            *iter,
+            true,  // is_favicon
+            0,     // no max size
+            base::Bind(&HostedAppTabHelper::DidDownloadFavicon,
+                       base::Unretained(this))));
+  }
+}
+
+void HostedAppTabHelper::FetchIconsIfNecessaryForCreateHostedApp() {
+  // If there are extra icon URLs in the WebApplicationInfo, we should download

[tapted, 2013/11/11 08:52:57]

nit: remove `we should` from comment

[calamity, 2013/11/13 06:37:14]

Done.

+  // them.
+  for (std::vector<WebApplicationInfo::IconInfo>::const_iterator it =
+           web_app_info_->icons.begin(); it != web_app_info_->icons.end();
+       ++it) {
+    if (it->url.is_valid()) {
+      std::set<GURL>::iterator url_it =
+          favicon_url_candidates_->lower_bound(it->url);

[tapted, 2013/11/11 08:52:57]

I don't think lower_bound is what's needed here.. to keep the insertion hint, it
might work if the next line was `if (url_it != end() && url_it != it->url)`, but
since the url is always in the map after (and we just want to know if it's new),
I'd probably go with

if (it->url.is_valid() && favicon_url_candidates_->insert(it->url).second)
  DownloadUrl(it->url);


(but if that's not what you're trying to do, maybe a comment, since it would be
unexpected for me)

[calamity, 2013/11/13 06:37:14]

Done.

+      if (url_it == favicon_url_candidates_->end()) {
+        favicon_url_candidates_->insert(url_it, it->url);
+        in_progress_requests_.insert(
+            web_contents()->DownloadImage(it->url, true, 0,
+                base::Bind(&HostedAppTabHelper::DidDownloadFavicon,
+                           base::Unretained(this))));
+      }
+    }
+  }
+
+  // If the delegate doesn't need the window icon, we wouldn't have fetched the
+  // icons yet.
+  // If no more downloads are pending, we can proceed directly to creating the
+  // hosted app.
+  if (!fetch_icons_immediately_)
+    FetchIcons();
+  else if (in_progress_requests_.empty())
+    FinishCreateHostedApp();
+}
+
+void HostedAppTabHelper::FinishCreateHostedApp() {
+  web_app_info_->is_bookmark_app = true;
+
+  web_app_info_->app_url = web_app_info_->app_url.is_empty() ?
+      web_contents()->GetURL() : web_app_info_->app_url;
+  web_app_info_->title = web_app_info_->title.empty()

[tapted, 2013/11/11 08:52:57]

nit: the nested ternaries are a bit hard to read, and it looks like you can end
up with `title = title`. Also WebContentsImpl::GetTitle looks kinda complex
(might be good just to call once). Maybe

if (web_app_info_->title.empty())
  web_app_info_->title = web_contents()->GetTitle();
if (web_app_info_->title.empty())
  web_app_info_->title = UTF8ToUTF16(web_app_info_->app_url.spec());
  

Same above, can just be

if (web_app_info_->app_url.empty())
  web_app_info_->app_url = web_contents()->GetURL()

[calamity, 2013/11/13 06:37:14]

Done.

+      ? (web_contents()->GetTitle().empty()
+             ? UTF8ToUTF16(web_app_info_->app_url.spec())
+             : web_contents()->GetTitle())
+      : web_app_info_->title;
+  web_app_info_->urls.push_back(web_app_info_->app_url);
+
+  // Add the downloaded icons.
+  for (gfx::ImageFamily::const_iterator it = icon_family_.begin();
+       it != icon_family_.end(); ++it) {
+    if (!it->IsEmpty()) {
+      WebApplicationInfo::IconInfo icon_info;
+      icon_info.data = it->AsBitmap();
+      icon_info.width = icon_info.data.width();
+      icon_info.height = icon_info.data.height();
+      web_app_info_->icons.push_back(icon_info);
+    }
+  }
+
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+  scoped_refptr<extensions::CrxInstaller> installer(
+      extensions::CrxInstaller::CreateSilent(profile->GetExtensionService()));
+  installer->set_error_on_unsupported_requirements(true);
+  installer->InstallWebApp(*web_app_info_);
+
+  web_app_info_.reset();
+}
+
+void HostedAppTabHelper::DidDownloadFavicon(
+    int id,
+    int http_status_code,
+    const GURL& image_url,
+    const std::vector<SkBitmap>& bitmaps,
+    const std::vector<gfx::Size>& original_bitmap_sizes) {
+
+  std::set<int>::iterator iter = in_progress_requests_.find(id);
+  // Check that this request is one of ours.

[tapted, 2013/11/11 08:52:57]

should this always be true? (DCHECK?)

[calamity, 2013/11/13 06:37:14]

in_progress_requests could have been cleared by a navigate or a favicon url
update.

[tapted, 2013/11/13 07:44:53]

oops - I realised that after writing that comment.

+  if (iter == in_progress_requests_.end())
+    return;
+
+  in_progress_requests_.erase(iter);

[tapted, 2013/11/11 08:52:57]

erase(val) returns the number of things erased. So the above 6 lines could just
be

if (in_progress_requests_.erase(id) == 0)
  return;  // Request canceled by DidUpdateFaviconURL().

[calamity, 2013/11/13 06:37:14]

Done.

+
+  for (std::vector<SkBitmap>::const_iterator it = bitmaps.begin();
+       it != bitmaps.end(); ++it) {
+    icon_family_.Add(gfx::Image::CreateFrom1xBitmap(*it));
+  }
+
+  // Once all requests have been resolved, perform post-download tasks.
+  if (!in_progress_requests_.empty())
+    return;
+
+  if (delegate_)
+    delegate_->OnWindowIconLoaded(web_contents());
+
+  if (web_app_info_)
+    FinishCreateHostedApp();
+}
+
+// content::WebContentsObserver overrides:
+void HostedAppTabHelper::DidNavigateMainFrame(
+    const content::LoadCommittedDetails& details,
+    const content::FrameNavigateParams& params) {
+  // Clear all pending requests.
+  favicon_url_candidates_.reset();
+  in_progress_requests_.clear();
+  web_app_info_.reset();
+  icon_family_.clear();
+}
+
+void HostedAppTabHelper::DidUpdateFaviconURL(
+    int32 page_id,
+    const std::vector<content::FaviconURL>& candidates) {
+  in_progress_requests_.clear();
+  icon_family_.clear();
+  favicon_url_candidates_.reset(new std::set<GURL>());

[tapted, 2013/11/11 08:52:57]

std::set constructor can take two iterators i.e. 

favicon_url_candidates_.reset(
    new std::set<GURL>(candidates.begin(), candidates.end()));

(or use std::set::assign(..) if you ditch the scoped_ptr)

[calamity, 2013/11/13 06:37:14]

This needs to grab the icon_url from each element.

[tapted, 2013/11/13 07:44:53]

Ah, so it does.

Does this needs a comment that (for now?) it is just loading all available
favicons (i.e. of types FAVICON, TOUCH_ICON and TOUCH_COMPRESSED_ICON)?  And..
should you skip INVALID_ICON (assuming that gets set when <waves hands>there's a
URL but it's invalid</waves hands>)

+  for (std::vector<content::FaviconURL>::const_iterator it = candidates.begin();
+       it != candidates.end(); ++it) {
+    favicon_url_candidates_->insert(it->icon_url);
+  }
+
+  if (fetch_icons_immediately_)
+    FetchIcons();
+
+  // If |web_app_info_| is populated, we are in the middle of creating a hosted
+  // app. Resume this process.
+  if (web_app_info_)
+    FetchIconsIfNecessaryForCreateHostedApp();
+}