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_util.h"
 #include "ash/shell.h"
 #include "ash/wm/window_util.h"
 #include "base/strings/stringprintf.h"
@@ skipping 104 matching lines @@
   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);
   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();
     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.
+    // 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();
       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);
-    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);
 }
 
 void ExtensionAppWindowLauncherController::UnregisterApp(aura::Window* window) {
   WindowToAppShelfIdMap::iterator iter1 =
       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);
   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);
   }
 }
 
 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 =
       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);

[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;
 }