Replace IDR window property use with gfx::ImageSkia icons.

Chrome shouldn't rely on ash to load images by IDR. Consolidate codepaths/properties for window icons. NOTE: Regresses Settings & Task Manager shelf icons in mash. (this will be resolved once we're using aura::Windows in mash) BUG=NONE TEST=No Settings or Task Manager window/shelf icon regressions. R=jamescook@chromium.org TBR=tsepez@chromium.org Review-Url: https://codereview.chromium.org/2514473003 Cr-Commit-Position: refs/heads/master@{#433082}
hylschwy · Nov 18, 2016 · d3ddd7a · d3ddd7a
1 parent 3dede1e
commit d3ddd7a
Show file tree

Hide file tree

Showing 13 changed files with 23 additions and 76 deletions.
diff --git a/ash/aura/wm_window_aura.cc b/ash/aura/wm_window_aura.cc
@@ -333,9 +333,6 @@ int WmWindowAura::GetIntProperty(WmWindowProperty key) {
   if (key == WmWindowProperty::MODAL_TYPE)
     return window_->GetProperty(aura::client::kModalKey);
 
-  if (key == WmWindowProperty::SHELF_ICON_RESOURCE_ID)
-    return window_->GetProperty(kShelfIconResourceIdKey);
-
   if (key == WmWindowProperty::SHELF_ID)
     return window_->GetProperty(kShelfIDKey);
 
@@ -350,10 +347,6 @@ int WmWindowAura::GetIntProperty(WmWindowProperty key) {
 }
 
 void WmWindowAura::SetIntProperty(WmWindowProperty key, int value) {
-  if (key == WmWindowProperty::SHELF_ICON_RESOURCE_ID) {
-    window_->SetProperty(kShelfIconResourceIdKey, value);
-    return;
-  }
   if (key == WmWindowProperty::SHELF_ID) {
     window_->SetProperty(kShelfIDKey, value);
     return;
@@ -884,8 +877,6 @@ void WmWindowAura::OnWindowPropertyChanged(aura::Window* window,
     wm_property = WmWindowProperty::EXCLUDE_FROM_MRU;
   } else if (key == aura::client::kModalKey) {
     wm_property = WmWindowProperty::MODAL_TYPE;
-  } else if (key == kShelfIconResourceIdKey) {
-    wm_property = WmWindowProperty::SHELF_ICON_RESOURCE_ID;
   } else if (key == kShelfIDKey) {
     wm_property = WmWindowProperty::SHELF_ID;
   } else if (key == kShelfItemTypeKey) {

diff --git a/ash/common/shelf/shelf_constants.h b/ash/common/shelf/shelf_constants.h
@@ -28,9 +28,6 @@ enum ShelfConstant {
   SHELF_INSETS_FOR_AUTO_HIDE
 };
 
-// Invalid image resource id used for shelf items.
-const int kInvalidImageResourceID = -1;
-
 // We reserve a small area on the edge of the workspace area to ensure that
 // the resize handle at the edge of the window can be hit.
 extern const int kWorkspaceAreaVisibleInset;

diff --git a/ash/common/shelf/shelf_window_watcher.cc b/ash/common/shelf/shelf_window_watcher.cc
@@ -15,11 +15,8 @@
 #include "ash/common/wm_window.h"
 #include "ash/common/wm_window_property.h"
 #include "ash/public/cpp/shell_window_ids.h"
-#include "ui/base/resource/resource_bundle.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
-#include "ui/gfx/image/image_skia.h"
-#include "ui/resources/grit/ui_resources.h"
 
 namespace ash {
 namespace {
@@ -38,15 +35,9 @@ void UpdateShelfItemForWindow(ShelfItem* item, WmWindow* window) {
   item->app_id = window->GetStringProperty(WmWindowProperty::APP_ID);
 
   // Prefer app icons over window icons, they're typically larger.
-  gfx::ImageSkia image = window->GetAppIcon();
-  if (image.isNull())
-    image = window->GetWindowIcon();
-  if (image.isNull()) {
-    int icon = window->GetIntProperty(WmWindowProperty::SHELF_ICON_RESOURCE_ID);
-    if (icon != kInvalidImageResourceID)
-      image = *ResourceBundle::GetSharedInstance().GetImageSkiaNamed(icon);
-  }
-  item->image = image;
+  item->image = window->GetAppIcon();
+  if (item->image.isNull())
+    item->image = window->GetWindowIcon();
 }
 
 }  // namespace
@@ -90,7 +81,6 @@ void ShelfWindowWatcher::UserWindowObserver::OnWindowPropertyChanged(
       property == WmWindowProperty::APP_ID ||
       property == WmWindowProperty::DRAW_ATTENTION ||
       property == WmWindowProperty::SHELF_ITEM_TYPE ||
-      property == WmWindowProperty::SHELF_ICON_RESOURCE_ID ||
       property == WmWindowProperty::WINDOW_ICON) {
     window_watcher_->OnUserWindowPropertyChanged(window);
   }

diff --git a/ash/common/shelf/shelf_window_watcher_unittest.cc b/ash/common/shelf/shelf_window_watcher_unittest.cc
@@ -105,10 +105,6 @@ TEST_F(ShelfWindowWatcherTest, CreateAndRemoveShelfItemProperties) {
   // Clearing twice doesn't do anything.
   window2->SetIntProperty(WmWindowProperty::SHELF_ITEM_TYPE, TYPE_UNDEFINED);
   EXPECT_EQ(1, model_->item_count());
-
-  // Setting an icon id (without a valid item type) does not add a shelf item.
-  window2->SetIntProperty(WmWindowProperty::SHELF_ICON_RESOURCE_ID, 1234);
-  EXPECT_EQ(1, model_->item_count());
 }
 
 TEST_F(ShelfWindowWatcherTest, ActivateWindow) {

diff --git a/ash/common/wm_window_property.h b/ash/common/wm_window_property.h
@@ -30,9 +30,6 @@ enum class WmWindowProperty {
   // Type int, but cast to ui::ModalType.
   MODAL_TYPE,
 
-  // Type int.
-  SHELF_ICON_RESOURCE_ID,
-
   // Type int.
   SHELF_ID,
 

diff --git a/ash/mus/bridge/wm_window_mus.cc b/ash/mus/bridge/wm_window_mus.cc
@@ -107,8 +107,6 @@ WmWindowProperty WmWindowPropertyFromUI(const std::string& ui_window_key) {
     return WmWindowProperty::ALWAYS_ON_TOP;
   if (ui_window_key == ui::mojom::WindowManager::kExcludeFromMru_Property)
     return WmWindowProperty::EXCLUDE_FROM_MRU;
-  if (ui_window_key == ui::mojom::WindowManager::kShelfIconResourceId_Property)
-    return WmWindowProperty::SHELF_ICON_RESOURCE_ID;
   if (ui_window_key == ui::mojom::WindowManager::kShelfItemType_Property)
     return WmWindowProperty::SHELF_ITEM_TYPE;
   return WmWindowProperty::INVALID_PROPERTY;
@@ -425,18 +423,6 @@ int WmWindowMus::GetIntProperty(WmWindowProperty key) {
     return static_cast<int>(ui::MODAL_TYPE_NONE);
   }
 
-  if (key == WmWindowProperty::SHELF_ICON_RESOURCE_ID) {
-    if (window_->HasSharedProperty(
-            ui::mojom::WindowManager::kShelfIconResourceId_Property)) {
-      return window_->GetSharedProperty<int>(
-          ui::mojom::WindowManager::kShelfIconResourceId_Property);
-    }
-    // Mash provides a default shelf icon image.
-    // TODO(msw): Support icon resource ids and bitmaps:
-    // mojo::Array<uint8_t> app_icon = GetWindowAppIcon(window_);
-    return IDR_DEFAULT_FAVICON;
-  }
-
   if (key == WmWindowProperty::SHELF_ID) {
     if (window_->HasSharedProperty(
             ui::mojom::WindowManager::kShelfId_Property)) {
@@ -469,12 +455,6 @@ int WmWindowMus::GetIntProperty(WmWindowProperty key) {
 }
 
 void WmWindowMus::SetIntProperty(WmWindowProperty key, int value) {
-  if (key == WmWindowProperty::SHELF_ICON_RESOURCE_ID) {
-    window_->SetSharedProperty<int>(
-        ui::mojom::WindowManager::kShelfIconResourceId_Property, value);
-    return;
-  }
-
   if (key == WmWindowProperty::SHELF_ID) {
     window_->SetSharedProperty<int>(ui::mojom::WindowManager::kShelfId_Property,
                                     value);

diff --git a/ash/mus/window_manager.cc b/ash/mus/window_manager.cc
@@ -320,7 +320,6 @@ bool WindowManager::OnWmSetProperty(
   return name == ui::mojom::WindowManager::kShowState_Property ||
          name == ui::mojom::WindowManager::kPreferredSize_Property ||
          name == ui::mojom::WindowManager::kResizeBehavior_Property ||
-         name == ui::mojom::WindowManager::kShelfIconResourceId_Property ||
          name == ui::mojom::WindowManager::kShelfItemType_Property ||
          name == ui::mojom::WindowManager::kWindowAppIcon_Property ||
          name == ui::mojom::WindowManager::kWindowTitle_Property;

diff --git a/ash/wm/window_properties.cc b/ash/wm/window_properties.cc
@@ -19,10 +19,6 @@ DEFINE_WINDOW_PROPERTY_KEY(ui::WindowShowState,
                            kRestoreShowStateOverrideKey,
                            ui::SHOW_STATE_DEFAULT);
 
-DEFINE_WINDOW_PROPERTY_KEY(int,
-                           kShelfIconResourceIdKey,
-                           kInvalidImageResourceID);
-
 DEFINE_WINDOW_PROPERTY_KEY(ShelfID, kShelfIDKey, kInvalidShelfID);
 
 DEFINE_WINDOW_PROPERTY_KEY(int, kShelfItemTypeKey, TYPE_UNDEFINED);

diff --git a/ash/wm/window_properties.h b/ash/wm/window_properties.h
@@ -43,10 +43,6 @@ ASH_EXPORT extern const aura::WindowProperty<gfx::Rect*>* const
 ASH_EXPORT extern const aura::WindowProperty<ui::WindowShowState>* const
     kRestoreShowStateOverrideKey;
 
-// A property key to store the icon resource id for a window's shelf item.
-ASH_EXPORT extern const aura::WindowProperty<int>* const
-    kShelfIconResourceIdKey;
-
 // A property key to store the id for a window's shelf item.
 ASH_EXPORT extern const aura::WindowProperty<ShelfID>* const kShelfIDKey;
 

diff --git a/chrome/browser/ui/ash/launcher/settings_window_observer.cc b/chrome/browser/ui/ash/launcher/settings_window_observer.cc
@@ -17,10 +17,13 @@
 #include "services/ui/public/cpp/window.h"
 #include "services/ui/public/cpp/window_property.h"
 #include "services/ui/public/interfaces/window_manager.mojom.h"
+#include "ui/aura/client/aura_constants.h"
 #include "ui/aura/mus/mus_util.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_property.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/image/image_skia.h"
 
 namespace {
 
@@ -84,8 +87,10 @@ void SettingsWindowObserver::OnNewSettingsWindow(Browser* settings_browser) {
                           l10n_util::GetStringUTF16(IDS_SETTINGS_TITLE));
   property_util::SetIntProperty(window, ash::kShelfItemTypeKey,
                                 ash::TYPE_DIALOG);
-  property_util::SetIntProperty(window, ash::kShelfIconResourceIdKey,
-                                IDR_ASH_SHELF_ICON_SETTINGS);
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  gfx::ImageSkia* icon = rb.GetImageSkiaNamed(IDR_ASH_SHELF_ICON_SETTINGS);
+  // The new gfx::ImageSkia instance is owned by the window itself.
+  window->SetProperty(aura::client::kWindowIconKey, new gfx::ImageSkia(*icon));
 
   if (chrome::IsRunningInMash())
     ui_window_tracker_->Add(aura::GetMusWindow(window));

diff --git a/chrome/browser/ui/ash/property_util.cc b/chrome/browser/ui/ash/property_util.cc
@@ -17,8 +17,6 @@ namespace {
 // Get the corresponding ui::Window property key for an aura::Window key.
 template <typename T>
 const char* GetMusProperty(const aura::WindowProperty<T>* aura_key) {
-  if (aura_key == ash::kShelfIconResourceIdKey)
-    return ui::mojom::WindowManager::kShelfIconResourceId_Property;
   if (aura_key == ash::kShelfItemTypeKey)
     return ui::mojom::WindowManager::kShelfItemType_Property;
   NOTREACHED();

diff --git a/chrome/browser/ui/views/task_manager_view.cc b/chrome/browser/ui/views/task_manager_view.cc
@@ -41,6 +41,9 @@
 #include "ash/wm/window_util.h"                   // nogncheck
 #include "chrome/browser/ui/ash/ash_util.h"       // nogncheck
 #include "chrome/browser/ui/ash/property_util.h"  // nogncheck
+#include "ui/aura/client/aura_constants.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/image/image_skia.h"
 #endif  // defined(USE_ASH)
 
 #if defined(OS_WIN)
@@ -73,14 +76,14 @@ task_manager::TaskManagerTableModel* TaskManagerView::Show(Browser* browser) {
 
   g_task_manager_view = new TaskManagerView();
 
-  gfx::NativeWindow window =
+  gfx::NativeWindow context =
       browser ? browser->window()->GetNativeWindow() : nullptr;
 #if defined(USE_ASH)
-  if (!chrome::IsRunningInMash() && !window)
-    window = ash::wm::GetActiveWindow();
+  if (!chrome::IsRunningInMash() && !context)
+    context = ash::wm::GetActiveWindow();
 #endif
 
-  DialogDelegate::CreateDialogWidget(g_task_manager_view, window, nullptr);
+  DialogDelegate::CreateDialogWidget(g_task_manager_view, context, nullptr);
   g_task_manager_view->InitAlwaysOnTopState();
 
 #if defined(OS_WIN)
@@ -104,12 +107,13 @@ task_manager::TaskManagerTableModel* TaskManagerView::Show(Browser* browser) {
     focus_manager->SetFocusedView(g_task_manager_view->tab_table_);
 
 #if defined(USE_ASH)
-  aura::Window* aura_window =
-      g_task_manager_view->GetWidget()->GetNativeWindow();
-  property_util::SetIntProperty(aura_window, ash::kShelfItemTypeKey,
+  aura::Window* window = g_task_manager_view->GetWidget()->GetNativeWindow();
+  property_util::SetIntProperty(window, ash::kShelfItemTypeKey,
                                 ash::TYPE_DIALOG);
-  property_util::SetIntProperty(aura_window, ash::kShelfIconResourceIdKey,
-                                IDR_ASH_SHELF_ICON_TASK_MANAGER);
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  gfx::ImageSkia* icon = rb.GetImageSkiaNamed(IDR_ASH_SHELF_ICON_TASK_MANAGER);
+  // The new gfx::ImageSkia instance is owned by the window itself.
+  window->SetProperty(aura::client::kWindowIconKey, new gfx::ImageSkia(*icon));
 #endif
   return g_task_manager_view->table_model_.get();
 }

diff --git a/services/ui/public/interfaces/window_manager.mojom b/services/ui/public/interfaces/window_manager.mojom
@@ -58,8 +58,6 @@ interface WindowManager {
   const string kRestoreBounds_Property = "prop:restore-bounds";
   // Shadow style for the window. Type: mojom::ShadowStyle.
   const string kShadowStyle_Property = "prop:shadow-style";
-  // The image resource ID to be used as the shelf item's icon. Type: int
-  const string kShelfIconResourceId_Property = "prop:shelf-icon-resource-id";
   // The ID of the shelf item corresponding to this window. Type: int
   const string kShelfId_Property = "prop:shelf-id";
   // The type of item to be shown on the shelf for this window. Type: int