Enhance chrome.app.window API with better shelf integration

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

 // Copyright 2016 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/ash/launcher/extension_app_window_launcher_controller.h"
 
 #include "ash/shelf/shelf_delegate.h"
 #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"
 #include "chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_window_manager.h"
 #include "extensions/browser/app_window/app_window.h"
 #include "extensions/browser/app_window/native_app_window.h"
 #include "extensions/common/extension.h"
 #include "ui/aura/window_event_dispatcher.h"
 
 using extensions::AppWindow;
 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
 
 ExtensionAppWindowLauncherController::ExtensionAppWindowLauncherController(
     ChromeLauncherController* owner)
     : AppWindowLauncherController(owner) {
   AppWindowRegistry* registry = AppWindowRegistry::Get(owner->GetProfile());
   registry_.insert(registry);
   registry->AddObserver(this);
@@ skipping 74 matching lines @@
          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;
   window->AddObserver(this);
 
   // Find or create an item controller and launcher item.
   std::string app_id = app_window->extension_id();
   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);
   } else {
     LauncherItemController::Type type =
         app_window->window_type_is_panel()
             ? LauncherItemController::TYPE_APP_PANEL
             : LauncherItemController::TYPE_APP;
     ExtensionAppWindowLauncherItemController* controller =
         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.
       const gfx::Image& app_icon = app_window->app_icon();
       if (!app_icon.IsEmpty()) {
         owner()->SetLauncherItemImage(shelf_id, app_icon.AsImageSkia());
         controller->set_image_set_by_controller(true);
       }
     } 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);
   }
 }
 
 bool ExtensionAppWindowLauncherController::IsRegisteredApp(
     aura::Window* window) {
   return window_to_app_shelf_id_map_.find(window) !=
          window_to_app_shelf_id_map_.end();
 }
 
 // Private Methods
 
 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;
 }