Better handle existing browsers in BrowserStatusMonitor initialization

During initialization, BrowserStatusMonitor initializes its
browser_tab_strip_tracker_, which creates insertion events for all
existing web contents - on these events, BrowserStatusMonitor was
updating associated app statuses, and started observing the web contents
for navigation, but was not updating the browser windows kShelfIDKey
window property (which should match the active web content's app
ID). This property would get updated on the next navigation, but if
the web contents navigation had already finished by the time
BrowserStatusMonitor was created, the browser window kShelfIDKey would
remain unset. For example, during user first run flow, Discovery app
window gets launched before the user session changes to active state,
and thus before the BrowserStatusMonitor creation, causing the linked
bug.

This updates BrowserStatusMonitor::OnTabInserted to update the browser
window kShelfIDKey immediately, instead of waiting for navigation.

This required BrowserStatusMonitor::Initialize() call to be removed from
ChromeLauncherController's ctor to ChromeLauncherController::Init, after
CreateBrowserShortcutLauncherItem(). Attempts to update the shelf ID
window property before calling CreateBrowserShortcutLauncherItem()
are hitting DCHECK in
ChromeLauncherController::GetBrowserShortcutLauncherItemController().

BUG=1075940

Change-Id: I9fb8e404349c01d1998fb01b5d8133ff0afc105f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2223976Reviewed-by: Nancy Wang <nancylingwang@chromium.org>
Reviewed-by: Timothy Loh <timloh@chromium.org>
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775308}

chrome/browser/chromeos/plugin_vm/plugin_vm_files_unittest.cc

@@ -106,6 +106,7 @@ TEST_F(PluginVmFilesTest, LaunchPluginVmApp) {
               })));
   ash::ShelfModel shelf_model;
   ChromeLauncherController chrome_launcher_controller(&profile_, &shelf_model);
+  chrome_launcher_controller.Init();
 
   // Ensure that Plugin VM is allowed.
   test_helper.AllowPluginVm();

chrome/browser/chromeos/plugin_vm/plugin_vm_manager_impl_unittest.cc

@@ -54,6 +54,7 @@ class PluginVmManagerImplTest : public testing::Test {
     shelf_model_ = std::make_unique<ash::ShelfModel>();
     chrome_launcher_controller_ = std::make_unique<ChromeLauncherController>(
         testing_profile_.get(), shelf_model_.get());
+    chrome_launcher_controller_->Init();
     histogram_tester_ = std::make_unique<base::HistogramTester>();
     chromeos::DlcserviceClient::InitializeFake();
   }

chrome/browser/ui/app_list/arc/arc_app_unittest.cc

@@ -315,6 +315,7 @@ class ArcAppModelBuilderTest
   ChromeLauncherController* CreateLauncherController() {
     launcher_controller_ = std::make_unique<ChromeLauncherController>(
         profile_.get(), model_.get());
+    launcher_controller_->Init();
     return launcher_controller_.get();
   }
 

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

@@ -21,6 +21,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/common/chrome_features.h"
+#include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -195,7 +196,8 @@ void BrowserStatusMonitor::OnTabStripModelChanged(
 
   if (change.type() == TabStripModelChange::kInserted) {
     for (const auto& contents : change.GetInsert()->contents)
-      OnTabInserted(contents.contents);
+      OnTabInserted(tab_strip_model, contents.contents);
+    UpdateBrowserItemState();
   } else if (change.type() == TabStripModelChange::kRemoved) {
     auto* remove = change.GetRemove();
     if (remove->will_be_deleted) {
@@ -313,8 +315,18 @@ void BrowserStatusMonitor::OnTabReplaced(TabStripModel* tab_strip_model,
     app_service_instance_helper_->OnTabReplaced(old_contents, new_contents);
 }
 
-void BrowserStatusMonitor::OnTabInserted(content::WebContents* contents) {
+void BrowserStatusMonitor::OnTabInserted(TabStripModel* tab_strip_model,
+                                         content::WebContents* contents) {
   UpdateAppItemState(contents, false /*remove*/);
+  // If the contents does not have a visible navigation entry, wait until a
+  // navigation status changes before setting the browser window Shelf ID
+  // (done by the web contents observer added by AddWebContentsObserver()).
+  if (tab_strip_model->GetActiveWebContents() == contents &&
+      contents->GetController().GetVisibleEntry()) {
+    Browser* browser = chrome::FindBrowserWithWebContents(contents);
+    SetShelfIDForBrowserWindowContents(browser, contents);
+  }
+
   AddWebContentsObserver(contents);
   if (app_service_instance_helper_)
     app_service_instance_helper_->OnTabInserted(contents);

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

@@ -86,7 +86,8 @@ class BrowserStatusMonitor : public BrowserListObserver,
   void OnTabReplaced(TabStripModel* tab_strip_model,
                      content::WebContents* old_contents,
                      content::WebContents* new_contents);
-  void OnTabInserted(content::WebContents* contents);
+  void OnTabInserted(TabStripModel* tab_strip_model,
+                     content::WebContents* contents);
   void OnTabClosing(content::WebContents* contents);
 
   // Create LocalWebContentsObserver for |contents|.

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

@@ -260,12 +260,10 @@ ChromeLauncherController::ChromeLauncherController(Profile* profile,
       // version of status monitor.
       browser_status_monitor_ =
           std::make_unique<MultiProfileBrowserStatusMonitor>(this);
-      browser_status_monitor_->Initialize();
     } else {
       // Create our v1/v2 application / browser monitors which will inform the
       // launcher of status changes.
       browser_status_monitor_ = std::make_unique<BrowserStatusMonitor>(this);
-      browser_status_monitor_->Initialize();
     }
     return;
   }
@@ -278,14 +276,12 @@ ChromeLauncherController::ChromeLauncherController(Profile* profile,
     // of status monitor.
     browser_status_monitor_ =
         std::make_unique<MultiProfileBrowserStatusMonitor>(this);
-    browser_status_monitor_->Initialize();
     extension_app_window_controller.reset(
         new MultiProfileAppWindowLauncherController(this));
   } else {
     // Create our v1/v2 application / browser monitors which will inform the
     // launcher of status changes.
     browser_status_monitor_ = std::make_unique<BrowserStatusMonitor>(this);
-    browser_status_monitor_->Initialize();
     extension_app_window_controller.reset(
         new ExtensionAppWindowLauncherController(this));
   }
@@ -335,6 +331,7 @@ ChromeLauncherController::~ChromeLauncherController() {
 void ChromeLauncherController::Init() {
   CreateBrowserShortcutLauncherItem();
   UpdateAppLaunchersFromSync();
+  browser_status_monitor_->Initialize();
 }
 
 ash::ShelfID ChromeLauncherController::CreateAppLauncherItem(

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

@@ -4013,7 +4013,36 @@ TEST_P(ChromeLauncherControllerTest, PersistPinned) {
   EXPECT_EQ(ash::TYPE_PINNED_APP, model_->items()[app_index].type);
 
   launcher_controller_->UnpinAppWithID("1");
-  ASSERT_EQ(initial_size, model_->items().size());
+  EXPECT_EQ(initial_size + 1, model_->items().size());
+
+  tab_strip_model->CloseWebContentsAt(0, 0);
+  EXPECT_EQ(initial_size, model_->items().size());
+}
+
+// Verifies that ShelfID property is updated for browsers that are present when
+// ChromeLauncherController is created.
+TEST_P(ChromeLauncherControllerTest, ExistingBrowserWindowShelfIDSet) {
+  InitLauncherControllerWithBrowser();
+  launcher_controller_->PinAppWithID("1");
+
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+  ASSERT_EQ(1, tab_strip_model->count());
+
+  TestLauncherControllerHelper* helper = new TestLauncherControllerHelper;
+  helper->SetAppID(tab_strip_model->GetWebContentsAt(0), "0");
+  SetLauncherControllerHelper(helper);
+
+  RecreateLauncherController();
+  helper = new TestLauncherControllerHelper(profile());
+  helper->SetAppID(tab_strip_model->GetWebContentsAt(0), "1");
+  SetLauncherControllerHelper(helper);
+  launcher_controller_->Init();
+
+  EXPECT_TRUE(launcher_controller_->GetItem(ash::ShelfID("1")));
+  EXPECT_EQ(ash::ShelfID("1"),
+            ash::ShelfID::Deserialize(
+                browser()->window()->GetNativeWindow()->GetProperty(
+                    ash::kShelfIDKey)));
 }
 
 TEST_P(ChromeLauncherControllerTest, MultipleAppIconLoaders) {

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

@@ -101,6 +101,7 @@ class ShelfContextMenuTest
     model_ = std::make_unique<ash::ShelfModel>();
     launcher_controller_ =
         std::make_unique<ChromeLauncherController>(&profile_, model_.get());
+    launcher_controller_->Init();
 
     // Disable safe icon decoding to ensure ArcAppShortcutRequests returns in
     // the test environment.