cros: Alt+number cycles through existing windows of that app.

It worked OK before, but only if the current active window was also of
the same app. This is for browser and V1 apps.

Test: 
Bug: 1088312
Change-Id: I96cbf4f433b1ed7a73014a5ebd442dbc86093a0e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2240266
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#777251}

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

@@ -10,6 +10,7 @@
 #include <memory>
 #include <utility>
 
+#include "base/callback_forward.h"
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/extensions/launch_util.h"
 #include "chrome/browser/profiles/profile.h"
@@ -52,6 +53,61 @@ bool IsAppBrowser(Browser* browser) {
   return browser->is_type_app() || browser->is_type_app_popup();
 }
 
+// Activate the browser with the given |content| and show the associated tab,
+// or minimize the browser if it is already active. Returns the action
+// performed by activating the content.
+ash::ShelfAction ActivateContentOrMinimize(content::WebContents* content,
+                                           bool allow_minimize) {
+  Browser* browser = chrome::FindBrowserWithWebContents(content);
+  TabStripModel* tab_strip = browser->tab_strip_model();
+  int index = tab_strip->GetIndexOfWebContents(content);
+  DCHECK_NE(TabStripModel::kNoTab, index);
+
+  int old_index = tab_strip->active_index();
+  if (index != old_index)
+    tab_strip->ActivateTabAt(index);
+  return ChromeLauncherController::instance()->ActivateWindowOrMinimizeIfActive(
+      browser->window(), index == old_index && allow_minimize);
+}
+
+// Advance to the next window of an app if possible. |items| is the list of
+// browsers/web contents associated with this app. |active_item_callback|
+// retrieves the window that is currently active, if available.
+// |activate_callback| will activate the next window selected by this function.
+template <class T>
+base::Optional<ash::ShelfAction> AdvanceApp(
+    const std::vector<T*>& items,
+    base::OnceCallback<T*(const std::vector<T*>&, aura::Window**)>
+        active_item_callback,
+    base::OnceCallback<void(T*)> activate_callback) {
+  if (items.empty())
+    return base::nullopt;
+
+  // Get the active item and associated aura::Window if it exists.
+  aura::Window* active_item_window = nullptr;
+  T* active_item =
+      std::move(active_item_callback).Run(items, &active_item_window);
+
+  // If there is only one of the app, and it is the current active item,
+  // bounce the window to signal nothing happened.
+  if (items.size() == 1u && active_item) {
+    DCHECK(active_item_window);
+    ash_util::BounceWindow(active_item_window);
+    return ash::SHELF_ACTION_NONE;
+  }
+
+  // If one of the items is active, active the next one in the list, otherwise
+  // activate the first item in the list.
+  size_t index = 0;
+  if (active_item) {
+    DCHECK(base::Contains(items, active_item));
+    auto it = std::find(items.cbegin(), items.cend(), active_item);
+    index = (it - items.cbegin() + 1) % items.size();
+  }
+  std::move(activate_callback).Run(items[index]);
+  return ash::SHELF_ACTION_WINDOW_ACTIVATED;
+}
+
 // AppMatcher is used to determine if various WebContents instances are
 // associated with a specific app. Clients should call CanMatchWebContents()
 // before iterating through WebContents instances and calling
@@ -439,58 +495,51 @@ std::vector<Browser*> AppShortcutLauncherItemController::GetAppBrowsers() {
   return browsers;
 }
 
-ash::ShelfAction AppShortcutLauncherItemController::ActivateContentOrMinimize(
-    content::WebContents* content,
-    bool allow_minimize) {
-  Browser* browser = chrome::FindBrowserWithWebContents(content);
-  TabStripModel* tab_strip = browser->tab_strip_model();
-  int index = tab_strip->GetIndexOfWebContents(content);
-  DCHECK_NE(TabStripModel::kNoTab, index);
-
-  int old_index = tab_strip->active_index();
-  if (index != old_index)
-    tab_strip->ActivateTabAt(index);
-  return ChromeLauncherController::instance()->ActivateWindowOrMinimizeIfActive(
-      browser->window(), index == old_index && allow_minimize);
-}
-
 base::Optional<ash::ShelfAction>
 AppShortcutLauncherItemController::AdvanceToNextApp() {
-  Browser* browser = chrome::FindLastActive();
-  // The last active browser is not necessarily the active window. The window
-  // could be a v2 app or ARC app.
-  if (browser && browser->window()->IsActive()) {
-    if (IsWindowedWebApp()) {
-      std::vector<Browser*> browsers = GetAppBrowsers();
-      auto it = std::find(browsers.cbegin(), browsers.cend(), browser);
-      if (it == browsers.cend()) {
-        return base::nullopt;
-      }
-      if (browsers.size() == 1) {
-        ash_util::BounceWindow(browser->window()->GetNativeWindow());
-        return ash::SHELF_ACTION_NONE;
-      }
-      size_t index = (it - browsers.cbegin() + 1) % browsers.size();
-      browsers[index]->window()->Show();
-      browsers[index]->window()->Activate();
-      return ash::SHELF_ACTION_WINDOW_ACTIVATED;
-    } else {
-      std::vector<content::WebContents*> items = GetAppWebContents();
-      TabStripModel* tab_strip = browser->tab_strip_model();
-      content::WebContents* active_content = tab_strip->GetActiveWebContents();
-      auto it = std::find(items.cbegin(), items.cend(), active_content);
-      if (it != items.cend()) {
-        if (items.size() == 1) {
-          ash_util::BounceWindow(browser->window()->GetNativeWindow());
-          return ash::SHELF_ACTION_NONE;
-        }
-        size_t index = (it - items.cbegin() + 1) % items.size();
-        ActivateContentOrMinimize(items[index], false);
-        return ash::SHELF_ACTION_WINDOW_ACTIVATED;
-      }
-    }
+  if (!chrome::FindLastActive())
+    return base::nullopt;
+
+  if (IsWindowedWebApp()) {
+    return AdvanceApp(GetAppBrowsers(),
+                      base::BindOnce([](const std::vector<Browser*>& browsers,
+                                        aura::Window** out_window) -> Browser* {
+                        for (auto* browser : browsers) {
+                          if (browser->window()->IsActive()) {
+                            *out_window = browser->window()->GetNativeWindow();
+                            return browser;
+                          }
+                        }
+                        return nullptr;
+                      }),
+                      base::BindOnce([](Browser* browser) -> void {
+                        browser->window()->Show();
+                        browser->window()->Activate();
+                      }));
   }
-  return base::nullopt;
+
+  return AdvanceApp(
+      GetAppWebContents(),
+      base::BindOnce([](const std::vector<content::WebContents*>& web_contents,
+                        aura::Window** out_window) -> content::WebContents* {
+        for (auto* web_content : web_contents) {
+          Browser* browser = chrome::FindBrowserWithWebContents(web_content);
+          // The active web contents is on the active browser, and matches the
+          // index of the current active tab.
+          if (browser->window()->IsActive()) {
+            TabStripModel* tab_strip = browser->tab_strip_model();
+            int index = tab_strip->GetIndexOfWebContents(web_content);
+            if (tab_strip->active_index() == index) {
+              *out_window = browser->window()->GetNativeWindow();
+              return web_content;
+            }
+          }
+        }
+        return nullptr;
+      }),
+      base::BindOnce([](content::WebContents* web_contents) -> void {
+        ActivateContentOrMinimize(web_contents, /*allow_minimize=*/false);
+      }));
 }
 
 bool AppShortcutLauncherItemController::IsV2App() {

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

@@ -72,12 +72,6 @@ class AppShortcutLauncherItemController : public ash::ShelfItemDelegate,
   std::vector<content::WebContents*> GetAppWebContents();
   std::vector<Browser*> GetAppBrowsers();
 
-  // Activate the browser with the given |content| and show the associated tab,
-  // or minimize the browser if it is already active. Returns the action
-  // performed by activating the content.
-  ash::ShelfAction ActivateContentOrMinimize(content::WebContents* content,
-                                             bool allow_minimize);
-
   // If an owned item is already active, this function advances to the next item
   // (or bounce the browser if there is only one item) and returns a shelf
   // action. Otherwise, it returns nullopt.

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

@@ -1648,6 +1648,46 @@ IN_PROC_BROWSER_TEST_F(LauncherPlatformAppBrowserTest, AltNumberAppsTabbing) {
   EXPECT_TRUE(window1a->IsActive());
 }
 
+// Check that the keyboard activation of a launcher item tabs even if the app is
+// not currently activated.
+IN_PROC_BROWSER_TEST_F(LauncherPlatformAppBrowserTest,
+                       AltNumberAppsTabbingFromOtherApp) {
+  // Create one app with two windows.
+  const Extension* app1_extension1 =
+      LoadAndLaunchPlatformApp("launch", "Launched");
+  ui::BaseWindow* app1_window1 =
+      CreateAppWindow(browser()->profile(), app1_extension1)->GetBaseWindow();
+  ui::BaseWindow* app1_window2 =
+      CreateAppWindow(browser()->profile(), app1_extension1)->GetBaseWindow();
+  const ash::ShelfItem item1 = GetLastLauncherItem();
+  EXPECT_EQ(ash::TYPE_APP, item1.type);
+  EXPECT_EQ(ash::STATUS_RUNNING, item1.status);
+
+  // Create another app with two windows.
+  const Extension* app2_extension1 =
+      LoadAndLaunchPlatformApp("launch_2", "Launched");
+  ui::BaseWindow* app2_window1 =
+      CreateAppWindow(browser()->profile(), app2_extension1)->GetBaseWindow();
+  ui::BaseWindow* app2_window2 =
+      CreateAppWindow(browser()->profile(), app2_extension1)->GetBaseWindow();
+  const ash::ShelfItem item2 = GetLastLauncherItem();
+  EXPECT_EQ(ash::TYPE_APP, item2.type);
+  EXPECT_EQ(ash::STATUS_RUNNING, item2.status);
+
+  // Last created window should be active. Hitting the app shortcut should go to
+  // the first window of the app.
+  ASSERT_TRUE(app2_window2->IsActive());
+  SelectItem(item2.id, ui::ET_KEY_RELEASED);
+  EXPECT_TRUE(app2_window1->IsActive());
+
+  // Hitting the other app's shortcut should jump and focus the other app's
+  // windows.
+  SelectItem(item1.id, ui::ET_KEY_RELEASED);
+  EXPECT_TRUE(app1_window2->IsActive());
+  SelectItem(item1.id, ui::ET_KEY_RELEASED);
+  EXPECT_TRUE(app1_window1->IsActive());
+}
+
 // Test that we get correct shelf presence with hidden app windows.
 IN_PROC_BROWSER_TEST_F(LauncherPlatformAppBrowserTest, HiddenAppWindows) {
   int item_count = shelf_model()->item_count();