Refine extension app window registration code.

Call RegisterApp from OnAppWindowAdded (for single- and multi-profile).
Remove workarounds and comments for configuration differences.
Fix a test that was expecting broken/contradictory behavior...

Bug: 730876, 557406
Test: No behavior changes with extension window shelf icons.
Change-Id: Ifbfd64a6f107f0f651c8cc2c4d6bfa362f50c04c
Reviewed-on: https://chromium-review.googlesource.com/569260Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486545}

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

@@ -717,22 +717,29 @@ IN_PROC_BROWSER_TEST_F(LauncherPlatformAppBrowserTest,
   EXPECT_FALSE(window1->GetBaseWindow()->IsMaximized());
 
   // Creating a second window of the same type should change the behavior so
-  // that a click does not change the activation state.
+  // that a click on the shelf item does not change the activation state.
   AppWindow* window1a = CreateAppWindow(browser()->profile(), extension1);
+  EXPECT_TRUE(window1->GetNativeWindow()->IsVisible());
   EXPECT_TRUE(window1a->GetNativeWindow()->IsVisible());
+  EXPECT_FALSE(window1->GetBaseWindow()->IsActive());
   EXPECT_TRUE(window1a->GetBaseWindow()->IsActive());
+
+  // Ensure the same shelf item and delegate are used for |window1a|.
+  EXPECT_EQ(item1.id, GetLastLauncherItem().id);
+  EXPECT_EQ(item1_delegate, GetShelfItemDelegate(GetLastLauncherItem().id));
+
   // The first click does nothing.
   SelectItem(item1_delegate, ui::ET_MOUSE_PRESSED);
   EXPECT_TRUE(window1->GetNativeWindow()->IsVisible());
   EXPECT_TRUE(window1a->GetNativeWindow()->IsVisible());
-  EXPECT_TRUE(window1->GetBaseWindow()->IsActive());
-  EXPECT_FALSE(window1a->GetBaseWindow()->IsActive());
+  EXPECT_FALSE(window1->GetBaseWindow()->IsActive());
+  EXPECT_TRUE(window1a->GetBaseWindow()->IsActive());
   // The second neither.
   SelectItem(item1_delegate, ui::ET_MOUSE_PRESSED);
   EXPECT_TRUE(window1->GetNativeWindow()->IsVisible());
   EXPECT_TRUE(window1a->GetNativeWindow()->IsVisible());
-  EXPECT_TRUE(window1->GetBaseWindow()->IsActive());
-  EXPECT_FALSE(window1a->GetBaseWindow()->IsActive());
+  EXPECT_FALSE(window1->GetBaseWindow()->IsActive());
+  EXPECT_TRUE(window1a->GetBaseWindow()->IsActive());
 }
 
 // Confirm that ash::ShelfWindowWatcher correctly handles app panels.

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

@@ -78,20 +78,7 @@ void ExtensionAppWindowLauncherController::AdditionalUserAddedToSession(
 
 void ExtensionAppWindowLauncherController::OnAppWindowAdded(
     extensions::AppWindow* app_window) {
-  // TODO(msw): Determine why this only seems to be called in Mash. Setting the
-  // ShelfItemType as early as possible is important on Mash, to prevent Ash's
-  // ShelfWindowWatcher from creating a conflicting ShelfItem. Set the item type
-  // here for the mash config, and later on in RegisterApp for classic ash.
-  if (chromeos::GetAshConfig() != ash::Config::MASH)
-    return;
-
-  const ash::ShelfID shelf_id = GetShelfId(app_window);
-  DCHECK(!shelf_id.IsNull());
-  aura::Window* window = app_window->GetNativeWindow();
-  window->SetProperty<int>(
-      ash::kShelfItemTypeKey,
-      app_window->window_type_is_panel() ? ash::TYPE_APP_PANEL : ash::TYPE_APP);
-  window->SetProperty(ash::kShelfIDKey, new std::string(shelf_id.Serialize()));
+  RegisterApp(app_window);
 }
 
 void ExtensionAppWindowLauncherController::OnAppWindowShown(
@@ -122,15 +109,10 @@ void ExtensionAppWindowLauncherController::RegisterApp(AppWindow* app_window) {
   const ash::ShelfID shelf_id = GetShelfId(app_window);
   DCHECK(!shelf_id.IsNull());
 
-  // TODO(msw): Determine why OnAppWindowAdded only seems to be called in Mash.
-  if (chromeos::GetAshConfig() != ash::Config::MASH) {
-    window->SetProperty(ash::kShelfIDKey,
-                        new std::string(shelf_id.Serialize()));
-    window->SetProperty<int>(ash::kShelfItemTypeKey,
-                             app_window->window_type_is_panel()
-                                 ? ash::TYPE_APP_PANEL
-                                 : ash::TYPE_APP);
-  }
+  window->SetProperty(ash::kShelfIDKey, new std::string(shelf_id.Serialize()));
+  window->SetProperty<int>(
+      ash::kShelfItemTypeKey,
+      app_window->window_type_is_panel() ? ash::TYPE_APP_PANEL : ash::TYPE_APP);
 
   // Windows created by IME extension should be treated the same way as the
   // virtual keyboard window, which does not register itself in launcher.

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

@@ -54,10 +54,10 @@ class ExtensionAppWindowLauncherController
   void OnWindowDestroying(aura::Window* window) override;
 
  protected:
-  // Registers a app window with the shelf and this object.
+  // Registers an app window with the shelf and this object.
   void RegisterApp(extensions::AppWindow* app_window);
 
-  // Unregisters a app window with the shelf and this object.
+  // Unregisters an app window with the shelf and this object.
   void UnregisterApp(aura::Window* window);
 
   // Check if a given window is known to the launcher controller.

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

@@ -30,10 +30,8 @@ MultiProfileAppWindowLauncherController::
 MultiProfileAppWindowLauncherController::
     ~MultiProfileAppWindowLauncherController() {
   // We need to remove all Registry observers for added users.
-  for (AppWindowRegistryList::iterator it = multi_user_registry_.begin();
-       it != multi_user_registry_.end();
-       ++it)
-    (*it)->RemoveObserver(this);
+  for (extensions::AppWindowRegistry* registry : multi_user_registry_)
+    registry->RemoveObserver(this);
 }
 
 void MultiProfileAppWindowLauncherController::ActiveUserChanged(
@@ -42,10 +40,7 @@ void MultiProfileAppWindowLauncherController::ActiveUserChanged(
   // show / hide them one by one. To avoid that a user dependent state
   // "survives" in a launcher item, we first delete all items making sure that
   // nothing remains and then re-create them again.
-  for (AppWindowList::iterator it = app_window_list_.begin();
-       it != app_window_list_.end();
-       ++it) {
-    extensions::AppWindow* app_window = *it;
+  for (extensions::AppWindow* app_window : app_window_list_) {
     Profile* profile =
         Profile::FromBrowserContext(app_window->browser_context());
     if (!multi_user_util::IsProfileFromActiveUser(profile)) {
@@ -58,17 +53,14 @@ void MultiProfileAppWindowLauncherController::ActiveUserChanged(
       }
     }
   }
-  for (AppWindowList::iterator it = app_window_list_.begin();
-       it != app_window_list_.end();
-       ++it) {
-    extensions::AppWindow* app_window = *it;
+  for (extensions::AppWindow* app_window : app_window_list_) {
     Profile* profile =
         Profile::FromBrowserContext(app_window->browser_context());
     if (multi_user_util::IsProfileFromActiveUser(profile) &&
         !IsRegisteredApp(app_window->GetNativeWindow()) &&
         (app_window->GetBaseWindow()->IsMinimized() ||
          app_window->GetNativeWindow()->IsVisible()))
-      RegisterApp(*it);
+      RegisterApp(app_window);
   }
 }
 
@@ -86,13 +78,20 @@ void MultiProfileAppWindowLauncherController::OnAppWindowAdded(
     extensions::AppWindow* app_window) {
   app_window_list_.push_back(app_window);
   Profile* profile = Profile::FromBrowserContext(app_window->browser_context());
-  // If the window got created for a non active user but the user allowed to
-  // teleport to the current user's desktop, we teleport it now.
+  // If the window was created for an inactive user, but the user allowed the
+  // app to teleport to the current user's desktop, teleport this window now.
   if (!multi_user_util::IsProfileFromActiveUser(profile) &&
       UserHasAppOnActiveDesktop(app_window)) {
     chrome::MultiUserWindowManager::GetInstance()->ShowWindowForUser(
         app_window->GetNativeWindow(), multi_user_util::GetCurrentAccountId());
   }
+
+  // If the window was created for the active user or it has been teleported to
+  // the current user's desktop, register it to show an item on the shelf.
+  if (multi_user_util::IsProfileFromActiveUser(profile) ||
+      UserHasAppOnActiveDesktop(app_window)) {
+    RegisterApp(app_window);
+  }
 }
 
 void MultiProfileAppWindowLauncherController::OnAppWindowShown(
@@ -147,10 +146,7 @@ bool MultiProfileAppWindowLauncherController::UserHasAppOnActiveDesktop(
   const AccountId current_account_id = multi_user_util::GetCurrentAccountId();
   chrome::MultiUserWindowManager* manager =
       chrome::MultiUserWindowManager::GetInstance();
-  for (AppWindowList::iterator it = app_window_list_.begin();
-       it != app_window_list_.end();
-       ++it) {
-    extensions::AppWindow* other_window = *it;
+  for (extensions::AppWindow* other_window : app_window_list_) {
     DCHECK(!other_window->browser_context()->IsOffTheRecord());
     if (manager->IsWindowOnDesktopOfUser(other_window->GetNativeWindow(),
                                          current_account_id) &&