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"
 
@@ skipping 95 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()) {
+  std::string window_key = app_window->window_key();
+
+  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.
-    if (app_shelf_id == app_id) {
+    // item for this app id (e.g. pinned), use that shelf item, except for the
+    // case when the showInShelf parameter is true.

[stevenjb, 2016/06/14 18:33:21]

This is confusing. We should have a comment in the new if() clause describing
its behavior, and the old comment (updated) in the else() clause.

[Andra Paraschiv, 2016/06/28 07:04:46]

Done.

+    if (!window_key.empty() &&
+        ContainsKey(window_id_to_shelf_id_map_, window_key)) {

[stevenjb, 2016/06/14 18:33:21]

This should be window_key_to_shelf_id_map_.

[Andra Paraschiv, 2016/06/28 07:04:46]

Done.

+      shelf_id = window_id_to_shelf_id_map_[window_key];

[stevenjb, 2016/06/14 18:33:21]

We are doing two lookups. We should get an iterator to
window_key_to_shelf_id_map_ instead of using COntainsKey and use that here.

[Andra Paraschiv, 2016/06/28 07:04:46]

Done.

+      if (!owner()->IsPinned(shelf_id)) {

[stevenjb, 2016/06/14 18:33:21]

I'm not following what this is supposed to do. Why are we setting shelf_id to 0
of there is an existing unpinned item with the same key?

[Andra Paraschiv, 2016/06/28 07:04:46]

If a pinned window is closed, then unpinned and opened again we should use a new
shelf id, not the old one. We delete from the map the shelf id corresponding to
this window key.

+        window_id_to_shelf_id_map_.erase(window_key);
+        shelf_id = 0;
+      }
+    }
+    else if (app_shelf_id == app_id && !app_window->show_in_shelf() &&

[stevenjb, 2016/06/14 18:33:21]

else on previous line

[Andra Paraschiv, 2016/06/28 07:04:46]

Done.

+             secondary_window_controller_map_.empty() &&
+             window_id_to_shelf_id_map_.empty()) {

[stevenjb, 2016/06/14 18:33:21]

Why test for an empty map? Couldn't a different app could have entries?

[Andra Paraschiv, 2016/06/28 07:04:46]

Yes, you are right, thank you for this notice. The new patch set doesn't include
these conditions anymore.

       shelf_id =
           ash::Shell::GetInstance()->GetShelfDelegate()->GetShelfIDForAppID(
               app_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());

[stevenjb, 2016/06/14 18:33:21]

Currently the extra window icons will still have the app icon image, right?

I think we need to have a separate CL that allows chrome.app.window.create to
specify the icon first, so that secondary icons can have a distinct icon.

[Andra Paraschiv, 2016/06/21 09:04:13]

Yes, extra window icons still have the app icon image, we need to add the icon
parameter to the window create options.

         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 (!ContainsKey(app_controller_map_, app_shelf_id) &&
+        !app_window->show_in_shelf()) {
+      app_controller_map_[app_shelf_id] = controller;
+    } else if (app_window->show_in_shelf()) {
+      secondary_window_controller_map_[window] = controller;
+    }
+    if (!window_key.empty()) {
+      window_id_to_shelf_id_map_[window_key] = shelf_id;
+    }
   }

[stevenjb, 2016/06/14 18:33:21]

Having 3 different maps in this code now is super confusing.

Could we do something like this:

window_key = !app_window->window_key().empty() ? window_key =
app_window->window_key() : app_id;

Then just have one map of window_key -> shelf item?

[Andra Paraschiv, 2016/06/21 09:04:13]

We could use the window key to map to the shelf item for windows with
showInShelf parameter set to true or false.

Can we use app_shelf_id (as it is now in the default behavior for
showInShelf=false) for all windows with showInShelf=false and the window_key
value for windows with showInShelf=true?

There could be two possible solutions:

1. We use the window_key value for all the windows that have it set. If the
window_key is not set, we use the app_shelf_id value. 

window_key = !app_window->window_key().empty() ? app_window->window_key() :
app_shelf_id;

In this case e.g. a window with showInShelf=true and one with showInShelf=false
could not have the window_key set. We use the app_shelf_id value as the
window_key for both. Each of them will have a different shelf_id and we will use
the same app_shelf_id map key for 2 different shelf_id values. 

2. All showInShelf=true windows should have a window_key set and we use this
value for the map. For the showInShelf=false windows we always use the
app_shelf_id regardless of the value of their window_key.

window_key = app_shelf_id;
if (!app_window->window_key().empty() && app_window->showInShelf())
  window_key = app_window->window_key();

   owner()->SetItemStatus(shelf_id, status);
   ash::SetShelfIDForWindow(shelf_id, window);
 }
 
 void ExtensionAppWindowLauncherController::UnregisterApp(aura::Window* window) {
-  WindowToAppShelfIdMap::iterator iter1 =
+  WindowToAppShelfIdMap::iterator 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(shelf_id_iter != window_to_app_shelf_id_map_.end());
+  std::string app_shelf_id = shelf_id_iter->second;
+  window_to_app_shelf_id_map_.erase(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(app_shelf_id);
+  WindowControllerMap::iterator secondary_window_iter =
+      secondary_window_controller_map_.find(window);
+  DCHECK(app_controller_iter != app_controller_map_.end() ||
+         secondary_window_iter != secondary_window_controller_map_.end());

[stevenjb, 2016/06/14 18:33:21]

I think we should be able to combine these maps. window_to_app_shelf_id_map_
should have the correct id for the window, whether it is a secondary window or
not.

[Andra Paraschiv, 2016/06/28 07:04:46]

We combined the app_controller_map and the secondary_window_map and use the
window_key value for the window_to_app_shelf_id_map.

+  ExtensionAppWindowLauncherItemController* controller;
+  if (secondary_window_iter != secondary_window_controller_map_.end())
+    controller = secondary_window_iter->second;
+  else
+    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.
     ash::ShelfID shelf_id = controller->shelf_id();
+    if (!owner()->IsPinned(shelf_id)) {
+      for (WindowIdToShelfIdMap::const_iterator i =
+           window_id_to_shelf_id_map_.begin();
+           i != window_id_to_shelf_id_map_.end(); ++i) {
+        if (shelf_id == i->second) {
+          window_id_to_shelf_id_map_.erase(i);
+          break;
+        }
+      }

[stevenjb, 2016/06/14 18:33:21]

I think we ma have an stl util helper to do this if we still need it.

[Andra Paraschiv, 2016/06/28 07:04:46]

We should delete the shelf_id from the map for unpinned windows that are closed.

+    }
     owner()->CloseLauncherItem(shelf_id);
-    app_controller_map_.erase(iter2);
+    if (secondary_window_iter != secondary_window_controller_map_.end())
+      secondary_window_controller_map_.erase(secondary_window_iter);
+    else
+      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 shelf_id_iter =
       window_to_app_shelf_id_map_.find(window);
-  if (iter1 == window_to_app_shelf_id_map_.end())
+  if (shelf_id_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())
-    return nullptr;
-  return iter2->second;
+  WindowControllerMap::iterator secondary_window_iter =
+      secondary_window_controller_map_.find(window);
+  if (secondary_window_iter != secondary_window_controller_map_.end())
+    return secondary_window_iter->second;
+  AppControllerMap::iterator app_controller_iter =
+      app_controller_map_.find(shelf_id_iter->second);
+  if (app_controller_iter != app_controller_map_.end())
+    return app_controller_iter->second;
+  return nullptr;
 }