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..03ee297ae0601c122a213e7e2ab9a6dd5421d6d9 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"
@@ -24,9 +25,16 @@ using extensions::AppWindowRegistry;
 namespace {
 
 std::string GetAppShelfId(AppWindow* app_window) {
+  // Set app_shelf_id if showInShelf parameter is false. If showInShelf
+  // parameter is true the window key value is appended to the app_shelf_id.
   if (app_window->window_type_is_panel())
-    return base::StringPrintf("panel:%d", app_window->session_id().id());
-  return app_window->extension_id();
+    return app_window->show_in_shelf() ?
+        base::StringPrintf("panel:%d", app_window->session_id().id()) +
+        app_window->window_key() :
+        base::StringPrintf("panel:%d", app_window->session_id().id());
+  return app_window->show_in_shelf() ?
+      app_window->extension_id() + app_window->window_key() :
+      app_window->extension_id();

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

This could all be simplified. Also, I believe that session_id predates
window_key and should only be used for panels if window_key is not set, e.g:

std::string shelf_id = app_window->extension_id();
if (app_window->show_in_shelf() || app_window->window_type_is_panel()) {
  if (!app_window->window_key().empty())
    shelf_id += app_window->window_key();
  else
    shelf_id += base::StringToInt(app_window->session_id().id());
}
return shelf_id;

Note: This would also make it OK for window_key() to be empty since session_id
will always be unique per window (I had forgotten about it). We shouldn't allow
items with an empty window_key/id to be pinned, but they could still have their
own icon within a session without specifying 'id' in the create params.

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

Thank you Steven for the feedback, we will include the simpler version of this
in the new patch set. Yes, the unique id will still be needed for pinning
purposes, but we could have windows opened with showInShelf=true without pinning
them (now, for simplicity, as you said in the previous comments, we have
enforced it to always have an 'id' set, either the window will be pinned or
not).

[Andra Paraschiv, 2016/07/11 13:02:56]

Done.

 }
 
 }  // namespace
@@ -121,10 +129,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(app_shelf_id);
   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);
@@ -137,13 +148,18 @@ void ExtensionAppWindowLauncherController::RegisterApp(AppWindow* app_window) {
         new ExtensionAppWindowLauncherItemController(type, app_shelf_id, 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 app_shelf_id (e.g. pinned),
+    // use that shelf item.
+    AppShelfIdToShelfIdMap::iterator app_shelf_id_iter =
+        app_shelf_id_to_shelf_id_map_.find(app_shelf_id);
+    if (app_shelf_id_iter != app_shelf_id_to_shelf_id_map_.end()) {
+      if (owner()->IsPinned(app_shelf_id_iter->second)) {
+        shelf_id = app_shelf_id_iter->second;
+      } else {
+        app_shelf_id_to_shelf_id_map_.erase(app_shelf_id);
+      }
     }
+
     if (shelf_id == 0) {
       shelf_id = owner()->CreateAppLauncherItem(controller, app_id, status);
       // Restore any existing app icon and flag as set.
@@ -155,31 +171,39 @@ void ExtensionAppWindowLauncherController::RegisterApp(AppWindow* app_window) {
     } else {
       owner()->SetItemController(shelf_id, controller);
     }
-    const std::string app_shelf_id = GetAppShelfId(app_window);
+
+    // We need to change the controller associated with app_shelf_id.
     app_controller_map_[app_shelf_id] = controller;
+    app_shelf_id_to_shelf_id_map_[app_shelf_id] = shelf_id;
   }
   owner()->SetItemStatus(shelf_id, status);
   ash::SetShelfIDForWindow(shelf_id, window);
 }
 
 void ExtensionAppWindowLauncherController::UnregisterApp(aura::Window* window) {
-  WindowToAppShelfIdMap::iterator iter1 =
+  WindowToAppShelfIdMap::iterator window_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(window_iter != window_to_app_shelf_id_map_.end());
+  std::string app_shelf_id = window_iter->second;
+  window_to_app_shelf_id_map_.erase(window_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(app_shelf_id);
+  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)) {
+      app_shelf_id_to_shelf_id_map_.erase(app_shelf_id);
+    }
     owner()->CloseLauncherItem(shelf_id);
-    app_controller_map_.erase(iter2);
+    app_controller_map_.erase(app_controller_iter);
   }
 }
 
@@ -194,13 +218,13 @@ bool ExtensionAppWindowLauncherController::IsRegisteredApp(
 AppWindowLauncherItemController*
 ExtensionAppWindowLauncherController::ControllerForWindow(
     aura::Window* window) {
-  WindowToAppShelfIdMap::iterator iter1 =
+  WindowToAppShelfIdMap::iterator window_iter =
       window_to_app_shelf_id_map_.find(window);
-  if (iter1 == window_to_app_shelf_id_map_.end())
+  if (window_iter == 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())
+  AppControllerMap::iterator app_controller_iter =
+      app_controller_map_.find(window_iter->second);
+  if (app_controller_iter == app_controller_map_.end())
     return nullptr;
-  return iter2->second;
+  return app_controller_iter->second;
 }