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

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

Remove LauncherItemController's V1 app shelf item locking.
BrowserStatusMonitor instead reports the app opening and closing.
Add BrowserStatusMonitor::IsV1AppInShelfWithAppId helper.

Replace ChromeLauncherController lock functions with SetV1AppStatus.
Inline CLC::CreateAppShortcutLauncherItemWithType functionality.
Update tests.

BUG=557406
TEST=Automated; no Chrome OS V1 windowed app shelf item regressions.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2790803002
Cr-Commit-Position: refs/heads/master@{#461204}
Committed: https://chromium.googlesource.com/chromium/src/+/18a09913baf68e72752126437c88b87ca34c60b9

[msw, 2017-03-30 22:23:26]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 22:24:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 23:22:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 23:22:55]

Dry run: This issue passed the CQ dry run.

[msw, 2017-03-30 23:32:07]

Description was changed from

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

BUG=557406
TEST=Automated; no Chrome OS V1 windowed app shelf item regressions.
R=jamescook@chromium.org
==========

to

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

Remove LauncherItemController's V1 app shelf item locking.
BrowserStatusMonitor instead reports the app opening and closing.
Add BrowserStatusMonitor::IsV1AppInShelfWithAppId helper.

Replace ChromeLauncherController lock functions with SetV1AppStatus.
Inline CLC::CreateAppShortcutLauncherItemWithType functionality.
Update tests.

BUG=557406
TEST=Automated; no Chrome OS V1 windowed app shelf item regressions.
R=jamescook@chromium.org
==========

[msw, 2017-03-30 23:32:14]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[msw, 2017-03-30 23:32:44]

Hey James, please take a look; thanks!
(this helps move ShelfDelegate code out of CLC)

[commit-bot: I haz the power, 2017-03-30 23:33:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 00:42:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 00:42:54]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-03-31 01:07:42]

I didn't get to the tests yet, a few comments.

https://codereview.chromium.org/2790803002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (left):

https://codereview.chromium.org/2790803002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:263:
launcher_controller_->LockV1AppWithID(app_id);
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.

https://codereview.chromium.org/2790803002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/2790803002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:286: for (const auto&
iter : browser_to_app_id_map_) {
optional: stl_util.h base::ContainsValue()

https://codereview.chromium.org/2790803002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/browser_status_monitor.h (right):

https://codereview.chromium.org/2790803002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_status_monitor.h:103: bool
IsV1AppInShelfWithAppId(std::string app_id);
nit: const &

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

Please take another look; thanks!

https://codereview.chromium.org/2790803002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (left):

https://codereview.chromium.org/2790803002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:263:
launcher_controller_->LockV1AppWithID(app_id);
On 2017/03/31 01:07:41, James Cook wrote:
> 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.

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.

https://codereview.chromium.org/2790803002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/2790803002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:286: for (const auto&
iter : browser_to_app_id_map_) {
On 2017/03/31 01:07:41, James Cook wrote:
> optional: stl_util.h base::ContainsValue()

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

https://codereview.chromium.org/2790803002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/browser_status_monitor.h (right):

https://codereview.chromium.org/2790803002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_status_monitor.h:103: bool
IsV1AppInShelfWithAppId(std::string app_id);
On 2017/03/31 01:07:41, James Cook wrote:
> nit: const &

Done.

[msw, 2017-03-31 03:27:21]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 03:27:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 03:56:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 03:56:33]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-03-31 19:13:22]

LGTM

https://codereview.chromium.org/2790803002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2790803002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1693:
// Reporting that the app is closed agani should have no effect.
nit: again

https://codereview.chromium.org/2790803002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1740:
}
nice tests, documented and easy to read blocks.

[msw, 2017-03-31 19:36:07]

https://codereview.chromium.org/2790803002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2790803002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1693:
// Reporting that the app is closed agani should have no effect.
On 2017/03/31 19:13:22, James Cook wrote:
> nit: again

Done.

https://codereview.chromium.org/2790803002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1740:
}
On 2017/03/31 19:13:22, James Cook wrote:
> nice tests, documented and easy to read blocks.

Acknowledged.

[msw, 2017-03-31 19:36:11]

The CQ bit was checked by msw@chromium.org

[msw, 2017-03-31 19:36:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2790803002/#ps60001
(title: "spelling")

[commit-bot: I haz the power, 2017-03-31 19:37:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 20:17:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490988971510880,
"parent_rev": "95f83c51ade772da4b9d9aa8b1a6c40ad696a897", "commit_rev":
"18a09913baf68e72752126437c88b87ca34c60b9"}

[commit-bot: I haz the power, 2017-03-31 20:18:32]

Description was changed from

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

Remove LauncherItemController's V1 app shelf item locking.
BrowserStatusMonitor instead reports the app opening and closing.
Add BrowserStatusMonitor::IsV1AppInShelfWithAppId helper.

Replace ChromeLauncherController lock functions with SetV1AppStatus.
Inline CLC::CreateAppShortcutLauncherItemWithType functionality.
Update tests.

BUG=557406
TEST=Automated; no Chrome OS V1 windowed app shelf item regressions.
R=jamescook@chromium.org
==========

to

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

Remove LauncherItemController's V1 app shelf item locking.
BrowserStatusMonitor instead reports the app opening and closing.
Add BrowserStatusMonitor::IsV1AppInShelfWithAppId helper.

Replace ChromeLauncherController lock functions with SetV1AppStatus.
Inline CLC::CreateAppShortcutLauncherItemWithType functionality.
Update tests.

BUG=557406
TEST=Automated; no Chrome OS V1 windowed app shelf item regressions.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2790803002
Cr-Commit-Position: refs/heads/master@{#461204}
Committed:
https://chromium.googlesource.com/chromium/src/+/18a09913baf68e72752126437c88...
==========

[commit-bot: I haz the power, 2017-03-31 20:18:33]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/18a09913baf68e72752126437c88...