Enhance chrome.app.window API with better shelf integration

chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc

Index: chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
diff --git a/chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc b/chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
index d3d80af4098e70cf967eb808caea1a26930810fd..8348cdd694eeade7bf39951c005548c168d69d9d 100644
--- a/chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
+++ b/chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
@@ -8,6 +8,7 @@
 #include "ash/shelf/shelf_util.h"
 #include "ash/shell.h"
 #include "ash/wm/window_util.h"
+#include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
@@ -113,7 +114,11 @@ void ExtensionAppWindowLauncherController::RegisterApp(AppWindow* app_window) {
   DCHECK(window_to_app_shelf_id_map_.find(window) ==
          window_to_app_shelf_id_map_.end());
   const std::string app_shelf_id = GetAppShelfId(app_window);
-  window_to_app_shelf_id_map_[window] = app_shelf_id;
+  // Set window key to app shelf id if showInShelf parameter is false.
+  // If showInShelf parameter is true the window key value is used.
+  std::string window_key = app_window->show_in_shelf() ?
+      app_window->window_key() : app_shelf_id;

[Reilly Grant (use Gerrit), 2016/07/06 19:53:00]

app_window->window_key() is controlled by the application so we should probably
prefix it with the app_shelf_id so that an extension can't trick the window
manager into associating its windows with another app. It might make sense then
to just integrate this logic into GetAppShelfId().

[stevenjb, 2016/07/07 22:55:54]

Good catch, thanks!

[Andra Paraschiv, 2016/07/08 07:20:26]

Thank you Reilly for the review and for noticing this, we changed the logic to
append the window_key to the app_shelf_id.

+  window_to_app_shelf_id_map_[window] = window_key;
   window->AddObserver(this);
 
   // Find or create an item controller and launcher item.
@@ -121,10 +126,13 @@ void ExtensionAppWindowLauncherController::RegisterApp(AppWindow* app_window) {
   ash::ShelfItemStatus status = ash::wm::IsActiveWindow(window)
                                     ? ash::STATUS_ACTIVE
                                     : ash::STATUS_RUNNING;
-  AppControllerMap::iterator iter = app_controller_map_.find(app_shelf_id);
+  AppControllerMap::iterator app_controller_iter =
+      app_controller_map_.find(window_key);
   ash::ShelfID shelf_id = 0;
-  if (iter != app_controller_map_.end()) {
-    ExtensionAppWindowLauncherItemController* controller = iter->second;
+
+  if (app_controller_iter != app_controller_map_.end()) {
+    ExtensionAppWindowLauncherItemController* controller =
+        app_controller_iter->second;
     DCHECK(controller->app_id() == app_id);
     shelf_id = controller->shelf_id();
     controller->AddAppWindow(app_window);
@@ -134,16 +142,21 @@ void ExtensionAppWindowLauncherController::RegisterApp(AppWindow* app_window) {
             ? LauncherItemController::TYPE_APP_PANEL
             : LauncherItemController::TYPE_APP;
     ExtensionAppWindowLauncherItemController* controller =
-        new ExtensionAppWindowLauncherItemController(type, app_shelf_id, app_id,
+        new ExtensionAppWindowLauncherItemController(type, window_key, app_id,
                                                      owner());
     controller->AddAppWindow(app_window);
-    // If the app shelf id is not unique, and there is already a shelf
-    // item for this app id (e.g. pinned), use that shelf item.
-    if (app_shelf_id == app_id) {
-      shelf_id =
-          ash::Shell::GetInstance()->GetShelfDelegate()->GetShelfIDForAppID(
-              app_id);
+    // If there is already a shelf id mapped to this window key (e.g. pinned),
+    // use that shelf item.
+    WindowKeyToShelfIdMap::iterator window_key_iter =
+        window_key_to_shelf_id_map_.find(window_key);
+    if (window_key_iter != window_key_to_shelf_id_map_.end()) {
+      if (!owner()->IsPinned(window_key_iter->second)) {

[Reilly Grant (use Gerrit), 2016/07/06 19:53:00]

Reverse this if statement for better readability.

[Andra Paraschiv, 2016/07/08 07:20:26]

Done.

+        window_key_to_shelf_id_map_.erase(window_key);
+      } else {
+        shelf_id = window_key_iter->second;
+      }
     }
+
     if (shelf_id == 0) {
       shelf_id = owner()->CreateAppLauncherItem(controller, app_id, status);
       // Restore any existing app icon and flag as set.
@@ -155,31 +168,39 @@ void ExtensionAppWindowLauncherController::RegisterApp(AppWindow* app_window) {
     } else {
       owner()->SetItemController(shelf_id, controller);
     }
-    const std::string app_shelf_id = GetAppShelfId(app_window);
-    app_controller_map_[app_shelf_id] = controller;
+
+    // We need to change the controller associated with app window shelf id.
+    app_controller_map_[window_key] = controller;
+    window_key_to_shelf_id_map_[window_key] = shelf_id;
   }
   owner()->SetItemStatus(shelf_id, status);
   ash::SetShelfIDForWindow(shelf_id, window);
 }
 
 void ExtensionAppWindowLauncherController::UnregisterApp(aura::Window* window) {
-  WindowToAppShelfIdMap::iterator iter1 =
+  WindowToAppShelfIdMap::iterator app_shelf_id_iter =
       window_to_app_shelf_id_map_.find(window);
-  DCHECK(iter1 != window_to_app_shelf_id_map_.end());
-  std::string app_shelf_id = iter1->second;
-  window_to_app_shelf_id_map_.erase(iter1);
+  DCHECK(app_shelf_id_iter != window_to_app_shelf_id_map_.end());
+  std::string window_key = app_shelf_id_iter->second;

[Reilly Grant (use Gerrit), 2016/07/06 19:53:00]

This isn't window_key, it's app_shelf_id.

[stevenjb, 2016/07/07 22:55:54]

ditto

[Andra Paraschiv, 2016/07/08 07:20:26]

Done.

+  window_to_app_shelf_id_map_.erase(app_shelf_id_iter);
   window->RemoveObserver(this);
 
-  AppControllerMap::iterator iter2 = app_controller_map_.find(app_shelf_id);
-  DCHECK(iter2 != app_controller_map_.end());
-  ExtensionAppWindowLauncherItemController* controller = iter2->second;
+  AppControllerMap::iterator app_controller_iter =
+      app_controller_map_.find(window_key);
+  DCHECK(app_controller_iter != app_controller_map_.end());
+  ExtensionAppWindowLauncherItemController* controller;
+  controller = app_controller_iter->second;
+
   controller->RemoveWindow(controller->GetAppWindow(window));
   if (controller->window_count() == 0) {
-    // If this is the last window associated with the app shelf id, close the
-    // shelf item.
+    // If this is the last window associated with the app window shelf id,
+    // close the shelf item.
     ash::ShelfID shelf_id = controller->shelf_id();
+    if (!owner()->IsPinned(shelf_id)) {
+      window_key_to_shelf_id_map_.erase(window_key);
+    }
     owner()->CloseLauncherItem(shelf_id);
-    app_controller_map_.erase(iter2);
+    app_controller_map_.erase(app_controller_iter);
   }
 }
 
@@ -194,13 +215,13 @@ bool ExtensionAppWindowLauncherController::IsRegisteredApp(
 AppWindowLauncherItemController*
 ExtensionAppWindowLauncherController::ControllerForWindow(
     aura::Window* window) {
-  WindowToAppShelfIdMap::iterator iter1 =
+  WindowToAppShelfIdMap::iterator app_shelf_id_iter =
       window_to_app_shelf_id_map_.find(window);
-  if (iter1 == window_to_app_shelf_id_map_.end())
-    return nullptr;
-  std::string app_shelf_id = iter1->second;
-  AppControllerMap::iterator iter2 = app_controller_map_.find(app_shelf_id);
-  if (iter2 == app_controller_map_.end())
+  if (app_shelf_id_iter == window_to_app_shelf_id_map_.end())
     return nullptr;
-  return iter2->second;
+  AppControllerMap::iterator app_controller_iter =
+      app_controller_map_.find(app_shelf_id_iter->second);
+  if (app_controller_iter != app_controller_map_.end())
+    return app_controller_iter->second;
+  return nullptr;

[Reilly Grant (use Gerrit), 2016/07/06 19:53:00]

For symmetry with the check above I would prefer:

  AppControllerMap::iterator app_controller_iter =
      app_controller_map_.find(app_shelf_id_iter->second);
  if (app_controller_iter == app_controller_map_.end())
    return nullptr;
  return app_controller_iter->second;

[Andra Paraschiv, 2016/07/08 07:20:26]

Done.

 }