Enhance chrome.app.window API with better shelf integration

Enhance chrome.app.window API with the possibility of creating a window that
will show up separately in the shelf, tied to its own icon.

Added a new property, showInShelf, to CreateWindowOptions. Default value for
the property is false.

Based on https://codereview.chromium.org/1811523002Co-Authored-By: Paul Sapunaru <paul.sapunaru@intel.com>
Co-Authored-By: Andra Paraschiv <andra.paraschiv@intel.com>

BUG=610299
TEST=interactive_ui_tests
Implement a simple extension that creates windows with the property set
to true. Observe that the newly created windows are added as separate icons in
the shelf.

Review-Url: https://codereview.chromium.org/1914993002
Cr-Commit-Position: refs/heads/master@{#405978}

AUTHORS

@@ -48,6 +48,7 @@ Amruth Raj <ckqr36@motorola.com>
 Anand Ratn <anand.ratn@samsung.com>
 Anastasios Cassiotis <tom.cassiotis@gmail.com>
 Ancil George <ancilgeorge@samsung.com>
+Andra Paraschiv <andra.paraschiv@intel.com>
 Andrei Parvu <andrei.prv@gmail.com>
 Andrei Parvu <parvu@adobe.com>
 Andrew Brampton <me@bramp.net>
@@ -645,6 +646,7 @@ U. Artie Eoff <ullysses.a.eoff@intel.com>
 Umar Hansa <umar.hansa@gmail.com>
 Upendra Gowda <upendrag.gowda@gmail.com>
 Vaibhav Agrawal <vaibhav1.a@samsung.com>
+Valentin Ilie <valentin.ilie@intel.com>
 Vamshikrishna Yellenki <vamshi@motorola.com>
 Vani Hegde <vani.hegde@samsung.com>
 Vartul Katiyar <vartul.k@samsung.com>

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

@@ -1532,6 +1532,117 @@ IN_PROC_BROWSER_TEST_F(LauncherPlatformAppBrowserTest, WindowAttentionStatus) {
   EXPECT_EQ(ash::STATUS_ACTIVE, item.status);
 }
 
+IN_PROC_BROWSER_TEST_F(LauncherPlatformAppBrowserTest,
+                       ShowInShelfWindowsWithWindowKeySet) {
+  ash::ShelfModel* shelf_model = ash::Shell::GetInstance()->shelf_model();
+
+  // Add a window with shelf True, close it
+  int item_count = shelf_model->item_count();
+  const Extension* extension = LoadAndLaunchPlatformApp("launch", "Launched");
+  AppWindow::CreateParams params;
+
+  params.show_in_shelf = true;
+  params.window_key = "window1";
+  AppWindow* window1 =
+      CreateAppWindowFromParams(browser()->profile(), extension, params);
+  // There should be only 1 item added to the shelf.
+  EXPECT_EQ(item_count + 1, shelf_model->item_count());
+  CloseAppWindow(window1);
+  EXPECT_EQ(item_count, shelf_model->item_count());
+
+  // Add a window with false, following one with true
+  item_count = shelf_model->item_count();
+  extension = LoadAndLaunchPlatformApp("launch", "Launched");
+
+  params.show_in_shelf = false;
+  params.window_key = "window1";
+  window1 = CreateAppWindowFromParams(browser()->profile(), extension, params);
+  EXPECT_EQ(item_count + 1, shelf_model->item_count());
+  params.show_in_shelf = true;
+  params.window_key = "window2";
+  AppWindow* window2 =
+      CreateAppWindowFromParams(browser()->profile(), extension, params);
+  // There should be 2 items added to the shelf: although window1 has
+  // show_in_shelf set to false, it's the first window created so its icon must
+  // show up in shelf.
+  EXPECT_EQ(item_count + 2, shelf_model->item_count());
+  CloseAppWindow(window1);
+  EXPECT_EQ(item_count + 1, shelf_model->item_count());
+  CloseAppWindow(window2);
+  EXPECT_EQ(item_count, shelf_model->item_count());
+
+  // Open just one window with false
+  item_count = shelf_model->item_count();
+  extension = LoadAndLaunchPlatformApp("launch", "Launched");
+
+  params.show_in_shelf = false;
+  params.window_key = "window1";
+  window1 = CreateAppWindowFromParams(browser()->profile(), extension, params);
+  // There should be 1 item added to the shelf: although show_in_shelf is false,
+  // this is the first window created.
+  EXPECT_EQ(item_count + 1, shelf_model->item_count());
+  CloseAppWindow(window1);
+  EXPECT_EQ(item_count, shelf_model->item_count());
+
+  // Add a window with true, following one with false
+  item_count = shelf_model->item_count();
+  extension = LoadAndLaunchPlatformApp("launch", "Launched");
+
+  params.show_in_shelf = true;
+  params.window_key = "window1";
+  window1 = CreateAppWindowFromParams(browser()->profile(), extension, params);
+  EXPECT_EQ(item_count + 1, shelf_model->item_count());  // main window
+  params.show_in_shelf = false;
+  params.window_key = "window2";
+  window2 = CreateAppWindowFromParams(browser()->profile(), extension, params);
+  EXPECT_EQ(item_count + 2, shelf_model->item_count());
+  CloseAppWindow(window1);
+  // There should be 1 item added to the shelf as the second window
+  // is set to show_in_shelf false
+  EXPECT_EQ(item_count + 1, shelf_model->item_count());
+  CloseAppWindow(window2);
+  EXPECT_EQ(item_count, shelf_model->item_count());
+
+  // Test closing windows in different order
+  item_count = shelf_model->item_count();
+  extension = LoadAndLaunchPlatformApp("launch", "Launched");
+
+  params.show_in_shelf = false;
+  params.window_key = "window1";
+  window1 = CreateAppWindowFromParams(browser()->profile(), extension, params);
+  EXPECT_EQ(item_count + 1, shelf_model->item_count());
+  params.show_in_shelf = false;
+  params.window_key = "window2";
+  window2 = CreateAppWindowFromParams(browser()->profile(), extension, params);
+  EXPECT_EQ(item_count + 1, shelf_model->item_count());
+  params.show_in_shelf = true;
+  params.window_key = "window3";
+  AppWindow* window3 =
+      CreateAppWindowFromParams(browser()->profile(), extension, params);
+  EXPECT_EQ(item_count + 2, shelf_model->item_count());
+  params.show_in_shelf = true;
+  params.window_key = "window4";
+  AppWindow* window4 =
+      CreateAppWindowFromParams(browser()->profile(), extension, params);
+  // There should be 3 items added to the shelf.
+  EXPECT_EQ(item_count + 3, shelf_model->item_count());
+  // Any window close order should be valid
+  CloseAppWindow(window4);
+  // Closed window4 that was shown in shelf. item_count would decrease
+  EXPECT_EQ(item_count + 2, shelf_model->item_count());
+  CloseAppWindow(window1);
+  // Closed window1 which was grouped together with window2 so item_count
+  // would not decrease
+  EXPECT_EQ(item_count + 2, shelf_model->item_count());
+  CloseAppWindow(window3);
+  // Closed window3 that was shown in shelf. item_count would decrease
+  EXPECT_EQ(item_count + 1, shelf_model->item_count());
+  CloseAppWindow(window2);
+  // Closed window2 - there is no other window in that group and item_count
+  // would decrease
+  EXPECT_EQ(item_count, shelf_model->item_count());
+}
+
 // Checks that the browser Alt "tabbing" is properly done.
 IN_PROC_BROWSER_TEST_F(ShelfAppBrowserTestNoDefaultBrowser,
                        AltNumberBrowserTabbing) {

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,18 @@ using extensions::AppWindowRegistry;
 namespace {
 
 std::string GetAppShelfId(AppWindow* app_window) {
-  if (app_window->window_type_is_panel())
-    return base::StringPrintf("panel:%d", app_window->session_id().id());
-  return app_window->extension_id();
+  // Set app_shelf_id default value to extension_id. If showInShelf parameter
+  // is true or the window type is panel and the window key is not empty, its
+  // value is appended to the app_shelf_id. Otherwise, if the window key is
+  // empty, the session_id is used.
+  std::string app_shelf_id = app_window->extension_id();
+  if (app_window->show_in_shelf() || app_window->window_type_is_panel()) {
+    if (!app_window->window_key().empty())
+      app_shelf_id += app_window->window_key();
+    else
+      app_shelf_id += base::StringPrintf("%d", app_window->session_id().id());
+  }
+  return app_shelf_id;
 }
 
 }  // namespace
@@ -121,10 +131,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 +150,35 @@ 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) {
+    // 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);
+      }
+    } else if (app_shelf_id == app_id) {
+      // show_in_shelf in false and not a panel
       shelf_id =
           ash::Shell::GetInstance()->GetShelfDelegate()->GetShelfIDForAppID(
               app_id);
+      // Check if the shelf_id corresponds to an already opened
+      // showInShelf=true window that has the same app_id. The current
+      // showInShelf=false window should not fold under this shelf item,
+      // so the shelf_id is set to 0 to get a new shelf_id.
+      auto&& id_map = app_shelf_id_to_shelf_id_map_;
+      if (std::find_if(
+              id_map.begin(), id_map.end(),
+              [shelf_id](const AppShelfIdToShelfIdMap::value_type& pair) {
+                return pair.second == shelf_id;
+              }) != id_map.end()) {
+        shelf_id = 0;
+      }
     }
+
     if (shelf_id == 0) {
       shelf_id = owner()->CreateAppLauncherItem(controller, app_id, status);
       // Restore any existing app icon and flag as set.
@@ -155,31 +190,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 +237,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;
 }

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

@@ -9,6 +9,7 @@
 #include <map>
 #include <string>
 
+#include "ash/shelf/shelf_util.h"
 #include "base/macros.h"
 #include "chrome/browser/ui/ash/launcher/app_window_launcher_controller.h"
 #include "extensions/browser/app_window/app_window_registry.h"
@@ -67,6 +68,7 @@ class ExtensionAppWindowLauncherController
   using AppControllerMap =
       std::map<std::string, ExtensionAppWindowLauncherItemController*>;
   using WindowToAppShelfIdMap = std::map<aura::Window*, std::string>;
+  using AppShelfIdToShelfIdMap = std::map<std::string, ash::ShelfID>;
 
   // A set of unowned AppWindowRegistry pointers for loaded users.
   // Note that this will only be used with multiple users in the side by side
@@ -79,6 +81,9 @@ class ExtensionAppWindowLauncherController
   // Allows us to get from an aura::Window to the app shelf id.
   WindowToAppShelfIdMap window_to_app_shelf_id_map_;
 
+  // Map of app shelf id to shelf id.
+  AppShelfIdToShelfIdMap app_shelf_id_to_shelf_id_map_;
+
   DISALLOW_COPY_AND_ASSIGN(ExtensionAppWindowLauncherController);
 };
 

extensions/browser/api/app_window/app_window_api.cc

@@ -75,6 +75,8 @@ const char kImeWindowMustBeImeWindowOrPanel[] =
     "IME extensions must create ime window ( with \"ime: true\" and "
     "\"frame: 'none'\") or panel window (with \"type: panel\").";
 #endif
+const char kShowInShelfWindowKeyNotSet[] =
+    "The \"showInShelf\" option requires the \"id\" option to be set.";
 }  // namespace app_window_constants
 
 const char kNoneFrameOption[] = "none";
@@ -321,6 +323,15 @@ bool AppWindowCreateFunction::RunAsync() {
           *options->visible_on_all_workspaces;
     }
 
+    if (options->show_in_shelf.get()) {
+      create_params.show_in_shelf = *options->show_in_shelf.get();
+
+      if (create_params.show_in_shelf && create_params.window_key.empty()) {
+        error_ = app_window_constants::kShowInShelfWindowKeyNotSet;
+        return false;
+      }
+    }
+
     if (options->type != app_window::WINDOW_TYPE_PANEL) {
       switch (options->state) {
         case app_window::STATE_NONE:

extensions/browser/app_window/app_window.cc

@@ -173,8 +173,8 @@ AppWindow::CreateParams::CreateParams()
       resizable(true),
       focused(true),
       always_on_top(false),
-      visible_on_all_workspaces(false) {
-}
+      visible_on_all_workspaces(false),
+      show_in_shelf(false) {}
 
 AppWindow::CreateParams::CreateParams(const CreateParams& other) = default;
 
@@ -255,6 +255,7 @@ AppWindow::AppWindow(BrowserContext* context,
       cached_always_on_top_(false),
       requested_alpha_enabled_(false),
       is_ime_window_(false),
+      show_in_shelf_(false),
       image_loader_ptr_factory_(this) {
   ExtensionsBrowserClient* client = ExtensionsBrowserClient::Get();
   CHECK(!client->IsGuestSession(context) || context->IsOffTheRecord())
@@ -295,8 +296,8 @@ void AppWindow::Init(const GURL& url,
     new_params.always_on_top = false;
 
   requested_alpha_enabled_ = new_params.alpha_enabled;
-
   is_ime_window_ = params.is_ime_window;
+  show_in_shelf_ = params.show_in_shelf;
 
   AppWindowClient* app_window_client = AppWindowClient::Get();
   native_app_window_.reset(

extensions/browser/app_window/app_window.h

@@ -191,6 +191,11 @@ class AppWindow : public content::WebContentsDelegate,
     // If true, the window will be visible on all workspaces. Defaults to false.
     bool visible_on_all_workspaces;
 
+    // If true, the window will have its own shelf icon. Otherwise the window
+    // will be grouped in the shelf with other windows that are associated with
+    // the app. Defaults to false.
+    bool show_in_shelf;
+
     // The API enables developers to specify content or window bounds. This
     // function combines them into a single, constrained window size.
     gfx::Rect GetInitialWindowBounds(const gfx::Insets& frame_insets) const;
@@ -368,6 +373,8 @@ class AppWindow : public content::WebContentsDelegate,
   // remove this TODO.
   bool is_ime_window() const { return is_ime_window_; }
 
+  bool show_in_shelf() const { return show_in_shelf_; }
+
   void SetAppWindowContentsForTesting(
       std::unique_ptr<AppWindowContents> contents) {
     app_window_contents_ = std::move(contents);
@@ -570,6 +577,9 @@ class AppWindow : public content::WebContentsDelegate,
   // Whether |is_ime_window| was set in the CreateParams.
   bool is_ime_window_;
 
+  // Whether |show_in_shelf| was set in the CreateParams.
+  bool show_in_shelf_;
+
   // PlzNavigate: this is called when the first navigation is ready to commit.
   base::Closure on_first_commit_callback_;
 

extensions/browser/app_window/app_window_interactive_uitest.cc

@@ -186,4 +186,17 @@ IN_PROC_BROWSER_TEST_F(AppWindowTest, MAYBE_DisableAlwaysOnTopInFullscreen) {
   CloseAppWindow(window);
 }
 
+// Tests a window created with showInShelf property enabled is indeed marked
+// as shown in shelf in AppWindow.
+IN_PROC_BROWSER_TEST_F(AppWindowTest, InitShowInShelf) {
+  AppWindow* window =
+      CreateTestAppWindow("{ \"showInShelf\": true , \"id\": \"window\" }");
+  ASSERT_TRUE(window);
+
+  // Ensure that the window created is marked as shown in shelf.
+  EXPECT_TRUE(window->show_in_shelf());
+
+  CloseAppWindow(window);
+}
+
 }  // namespace extensions

extensions/common/api/app_window.idl

@@ -225,6 +225,12 @@ namespace app.window {
     // Requires the <code>app.window.ime</code> API permission.
     [nodoc] boolean? ime;
 
+    // If true, the window will have its own shelf icon. Otherwise the window
+    // will be grouped in the shelf with other windows that are associated with
+    // the app. Defaults to false. If showInShelf is set to true you need to
+    // specify an id for the window.
+    boolean? showInShelf;
+
     // Frame type: <code>none</code> or <code>chrome</code> (defaults to
     // <code>chrome</code>). For <code>none</code>, the
     // <code>-webkit-app-region</code> CSS property can be used to apply