mash: Remove V1 app shelf item locking; monitor running status instead.

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

Index: chrome/browser/ui/ash/launcher/browser_status_monitor.cc
diff --git a/chrome/browser/ui/ash/launcher/browser_status_monitor.cc b/chrome/browser/ui/ash/launcher/browser_status_monitor.cc
index 4b7d06bf0940463cdeacea7fdc76d440398c972e..02f11e316b800b5dbbddd3659a6882e96d532137 100644
--- a/chrome/browser/ui/ash/launcher/browser_status_monitor.cc
+++ b/chrome/browser/ui/ash/launcher/browser_status_monitor.cc
@@ -259,8 +259,9 @@ void BrowserStatusMonitor::AddV1AppToShelf(Browser* browser) {
   std::string app_id =
       web_app::GetExtensionIdFromApplicationName(browser->app_name());
   if (!app_id.empty()) {
+    if (!IsV1AppInShelfWithAppId(app_id))
+      launcher_controller_->SetV1AppStatus(app_id, ash::STATUS_RUNNING);
     browser_to_app_id_map_[browser] = app_id;
-    launcher_controller_->LockV1AppWithID(app_id);

[James Cook, 2017/03/31 01:07:41]

Like I mentioned on IM, I'm a little concerned in theory that the lock count
could have been incremented above 1 here. That said, I don't really know how
this works (for v1 apps that are in multiple tabs, or v1 apps in multiple
windows). Based on the code in RemoveV1AppFromShelf() it looks like things would
go wrong if the count was > 1, since the unlock call would be followed by
removing it from the map.

Anyway, I think it would be good to manually exercise the code a bit and see if
it works the same. I think there are Googlers who use Secure Shell in multiple
tabs and in multiple windows.

[msw, 2017/03/31 03:27:14]

This matches the existing behavior. Any popup browser windows running the V1 app
are tracked in browser_to_app_id_map_, and I'm only adding or removing a shelf
item when the number of map entries with the given |app_id| value switches
0<->1. Since this is the only non-test V1 app locking user, there's no need for
a separate lock count. That said, this behavior doesn't count tabs, given the
existing crbug.com/707110. I can follow up with that separately, if you think
it's worthwhile.

   }
 }
 
@@ -268,9 +269,12 @@ void BrowserStatusMonitor::RemoveV1AppFromShelf(Browser* browser) {
   DCHECK(browser->is_type_popup() && browser->is_app());
   DCHECK(initialized_);
 
-  if (browser_to_app_id_map_.find(browser) != browser_to_app_id_map_.end()) {
-    launcher_controller_->UnlockV1AppWithID(browser_to_app_id_map_[browser]);
-    browser_to_app_id_map_.erase(browser);
+  auto iter = browser_to_app_id_map_.find(browser);
+  if (iter != browser_to_app_id_map_.end()) {
+    std::string app_id = iter->second;
+    browser_to_app_id_map_.erase(iter);
+    if (!IsV1AppInShelfWithAppId(app_id))
+      launcher_controller_->SetV1AppStatus(app_id, ash::STATUS_CLOSED);
   }
 }
 
@@ -278,6 +282,14 @@ bool BrowserStatusMonitor::IsV1AppInShelf(Browser* browser) {
   return browser_to_app_id_map_.find(browser) != browser_to_app_id_map_.end();
 }
 
+bool BrowserStatusMonitor::IsV1AppInShelfWithAppId(std::string app_id) {
+  for (const auto& iter : browser_to_app_id_map_) {

[James Cook, 2017/03/31 01:07:41]

optional: stl_util.h base::ContainsValue()

[msw, 2017/03/31 03:27:14]

Unfortunately I'm looking for a map *value* matching |app_id|, not a key. 

+    if (iter.second == app_id)
+      return true;
+  }
+  return false;
+}
+
 void BrowserStatusMonitor::AddWebContentsObserver(
     content::WebContents* contents) {
   if (webcontents_to_observer_map_.find(contents) ==