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 05cd17360af12941b2e28fad9ac4ba5c77d18ba7..ae29d635ab9300af0444a1ba61f58e744a258413 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
@@ -122,7 +122,8 @@ void ExtensionAppWindowLauncherController::RegisterApp(AppWindow* app_window) {
                                     : ash::STATUS_RUNNING;
   AppControllerMap::iterator iter = app_controller_map_.find(app_shelf_id);
   ash::ShelfID shelf_id = 0;
-  if (iter != app_controller_map_.end()) {
+
+  if (!app_window->show_in_shelf() && iter != app_controller_map_.end()) {
     ExtensionAppWindowLauncherItemController* controller = iter->second;
     DCHECK(controller->app_id() == app_id);
     shelf_id = controller->shelf_id();
@@ -137,10 +138,11 @@ void ExtensionAppWindowLauncherController::RegisterApp(AppWindow* app_window) {
                                                      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.
+    // item for this app id (e.g. pinned), use that shelf item, except for the
+    // case when the showInShelf parameter is true.
     if (app_shelf_id == app_id)
       shelf_id = owner()->GetShelfIDForAppID(app_id);
-    if (shelf_id == 0) {
+    if (shelf_id == 0 || app_window->show_in_shelf()) {
       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();
@@ -151,8 +153,15 @@ 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 the app if there is no
+    // controller currently tied to the app shelf id (first window).
+    if (!app_controller_map_[app_shelf_id]) {
+      app_controller_map_[app_shelf_id] = controller;

[stevenjb, 2016/04/26 16:30:00]

Use an iterator instead of doing the map lookup twice.

[Andra Paraschiv, 2016/04/28 09:49:03]

The code remained the same. When using an iterator the end of the map is reached
after calling the find method "app_controller_map_->find(app_shelf_id)".

[stevenjb, 2016/05/03 20:39:48]

Actually, [] will insert a new element (so this effectively inserts a default
(null) element then inserts |controller| over it), but frankly we shouldn't be
relying on that anyway. Instead use:

if (!ContainsKey(app_controller_map_, app_shelf_id))

(ContainsKey is a helper in stl_util.h).

[Andra Paraschiv, 2016/05/04 11:59:09]

Done.

+    } else if (app_window->show_in_shelf()) {
+      window_controller_map_[window] = controller;

[stevenjb, 2016/04/26 16:30:00]

This will only contain entries for secondary windows. We should name the member
to reflect that and explain that in the header comment.

Also, this logic seems incomplete. If the first window has show_in_shelf = true,
and another window is added with show_in_shelf = false, no new icon will be
created, which seems incorrect.

[Andra Paraschiv, 2016/04/28 09:49:03]

Renamed the member to secondary_window_controller_map. Regarding the logic we
should clarify it (e.g. windows with show_in_shelf=true/false have or not their
icon in the shelf depending on the order in which are opened) before modifying
the code.

+    }
   }
   owner()->SetItemStatus(shelf_id, status);
   ash::SetShelfIDForWindow(shelf_id, window);
@@ -167,15 +176,25 @@ void ExtensionAppWindowLauncherController::UnregisterApp(aura::Window* window) {
   window->RemoveObserver(this);
 
   AppControllerMap::iterator iter2 = app_controller_map_.find(app_shelf_id);
-  DCHECK(iter2 != app_controller_map_.end());
-  ExtensionAppWindowLauncherItemController* controller = iter2->second;
+  WindowControllerMap::iterator iter3 = window_controller_map_.find(window);

[stevenjb, 2016/04/26 16:30:00]

'iter2' wasn't a good name and adding 'iter3' makes it worse. Can we rename
these something like 'shelf_id_iter', 'app_controller_iter', and
'secondary_window_iter'?

[Andra Paraschiv, 2016/04/28 09:49:03]

Done.

+  DCHECK(iter2 != app_controller_map_.end() ||
+         iter3 != window_controller_map_.end());
+  ExtensionAppWindowLauncherItemController* controller;
+  if (iter3 != window_controller_map_.end())
+    controller = iter3->second;
+  else
+    controller = iter2->second;
+
   controller->RemoveWindowForNativeWindow(window);
   if (controller->window_count() == 0) {
     // If this is the last window associated with the app shelf id, close the
     // shelf item.
     ash::ShelfID shelf_id = controller->shelf_id();
     owner()->CloseLauncherItem(shelf_id);
-    app_controller_map_.erase(iter2);
+    if (iter3 != window_controller_map_.end())
+      window_controller_map_.erase(iter3);
+    else
+      app_controller_map_.erase(iter2);
   }
 }
 
@@ -196,7 +215,10 @@ ExtensionAppWindowLauncherController::ControllerForWindow(
     return nullptr;
   std::string app_shelf_id = iter1->second;
   AppControllerMap::iterator iter2 = app_controller_map_.find(app_shelf_id);

[stevenjb, 2016/04/26 16:30:00]

In this case we don't need iter2 unless iter3 == end. Move this line below 220
and rename the iterators.

[Andra Paraschiv, 2016/04/28 09:49:03]

Done.

-  if (iter2 == app_controller_map_.end())
-    return nullptr;
-  return iter2->second;
+  WindowControllerMap::iterator iter3 = window_controller_map_.find(window);
+  if (iter3 != window_controller_map_.end())
+    return iter3->second;
+  if (iter2 != app_controller_map_.end())
+    return iter2->second;
+  return nullptr;
 }