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

chrome/browser/ui/browser.cc

Index: chrome/browser/ui/browser.cc
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index 91ee18e9f04e1babaa061f2cfb16efaf2f9338ca..78654b248970059e57fe1e7597ed27f4876aa5e9 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -133,6 +133,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/toolbar/toolbar_model_impl.h"
 #include "chrome/browser/ui/unload_controller.h"
+#include "chrome/browser/ui/web_applications/hosted_app_tab_helper.h"
 #include "chrome/browser/ui/web_applications/web_app_ui.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
@@ -523,13 +524,20 @@ bool Browser::is_devtools() const {
 ///////////////////////////////////////////////////////////////////////////////
 // Browser, State Storage and Retrieval for UI:
 
-gfx::Image Browser::GetCurrentPageIcon() const {
+const gfx::Image Browser::GetCurrentPageIcon(int icon_size) const {
   WebContents* web_contents = tab_strip_model_->GetActiveWebContents();
   // |web_contents| can be NULL since GetCurrentPageIcon() is called by the
   // window during the window's creation (before tabs have been added).
-  FaviconTabHelper* favicon_tab_helper =
-      web_contents ? FaviconTabHelper::FromWebContents(web_contents) : NULL;
-  return favicon_tab_helper ? favicon_tab_helper->GetFavicon() : gfx::Image();
+  HostedAppTabHelper* hosted_app_tab_helper =
+      web_contents ? HostedAppTabHelper::FromWebContents(web_contents)
+                   : NULL;
+  if (hosted_app_tab_helper) {
+    const gfx::Image* icon =
+        hosted_app_tab_helper->icon_family().GetBest(icon_size, icon_size);
+    if (icon)
+      return *icon;
+  }
+  return gfx::Image();
 }
 
 string16 Browser::GetWindowTitleForCurrentTab() const {
@@ -1741,6 +1749,13 @@ void Browser::URLStarredChanged(content::WebContents* web_contents,
 }
 
 ///////////////////////////////////////////////////////////////////////////////
+// Browser, HostedAppTabHelperDelegate implementation:
+
+void Browser::OnWindowIconLoaded(content::WebContents* source) {
+  LoadingStateChanged(source);

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

This doesn't feel right. WebContentsImpl only calls this when is_loading changes
true <-> false (and it's a WebContentsDelegate function). Is the goal just to
update the taskbar icon? Maybe you can do that more directly, or else comment
what this is trying to achieve.

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

Yes, updating the loading animation and title bar should be sufficient here.

+}
+
+///////////////////////////////////////////////////////////////////////////////
 // Browser, ZoomObserver implementation:
 
 void Browser::OnZoomChanged(content::WebContents* source,
@@ -2079,6 +2094,13 @@ void Browser::SetAsDelegate(WebContents* web_contents, Browser* delegate) {
       TranslateTabHelper::FromWebContents(web_contents);
   translate_tab_helper->language_state().set_observer(
       delegate ? delegate->language_state_observer_.get() : NULL);
+  HostedAppTabHelper* hosted_app_tab_helper =

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

Is there an overhead to consider for this?

E.g. would it be better to only do this when `is_app()` is true, and to pass the
delegate in when calling CreateHostedApp() for the case when is_app is false?
(assuming I haven't missed something..).

But I'm actually not sure.. there doesn't seem to be much precedence for things
like this. E.g. I thought PasswordManager::CreateForWebContentsAndDelegate(..)
might be one, but it seems special.

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

I don't think so. Any overhead would be in creating the tab helper which is done
in BrowserTabContents and I don't think we can check for is_app() there. Either
way, this isn't necessary unless we are an app window.

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

Yeah - that's what I meant (sorry - should have commented there). E.g. you could
remove it from BrowserTabContents and instead create it on demand. Either here
(when is_app()) or in GetCurrentPageIcon

+      HostedAppTabHelper::FromWebContents(web_contents);
+  hosted_app_tab_helper->set_delegate(delegate);

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

What about SetDelegate(delegate, is_app()) - that would avoid the separate
set_fetch_icons_immediately(..) and avoid littering an already large file with
#ifdefs

[benwells, 2013/11/11 23:15:55]

Why is this limited OS_WIN? This flag should work currently on ChromeOS, and it
would be great if we could also make this available on Linux and Mac.

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

Done.

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

This is for Window Icons which are only used on Windows at the moment. The
feature as a whole will be turned on for ChromeOS.

+#if defined(OS_WIN)
+  // We need the window icon for hosted apps running as a window on Windows.
+  hosted_app_tab_helper->set_fetch_icons_immediately(is_app());
+#endif
 }
 
 void Browser::CloseFrame() {