arc: Fix regression in handling ARC shelf item.

arc: Fix regression in handling ARC shelf item.

This fix regressions caused by recent refactoring. First regression is
in case multiple shelf items for the same ARC app, clicking on shelf
item always activates last opened window. It is expected that in this
case last active window of that app becomes active. Second one,
selecting which item to activate from menu does not actually activate
selected item

BUG=703895
TEST=Manually, unit_tests extended.

Review-Url: https://codereview.chromium.org/2763893004
Cr-Commit-Position: refs/heads/master@{#458638}
Committed: https://chromium.googlesource.com/chromium/src/+/a19209825989de7d6b620aea95dd0f95b5c3c03f

[khmel, 2017-03-22 00:07:02]

khmel@chromium.org changed reviewers:
+ msw@chromium.org

[khmel, 2017-03-22 00:07:03]

Hi Mike,

PTAL

Thanks!

https://codereview.chromium.org/2763893004/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2763893004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:594:
AppWindowLauncherController::OnWindowActivated(reason, gained_active,
Required to store last active window.

[msw, 2017-03-22 00:30:15]

lgtm, thanks for catching this and adding a test. It'd be great if the
AppWindowLauncherItemController subclasses shared more logic.

https://codereview.chromium.org/2763893004/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2763893004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3789:
// Test that validates correctness of switching multiple shelf item for the same
nit: maybe "Test the application menu of a shelf item with multiple ARC
windows."

https://codereview.chromium.org/2763893004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3791:
TEST_P(ChromeLauncherControllerImplWithArcTest, MultipleItemsInShelf) {
nit: maybe s/MultipleItemsInShelf/ShelfItemWithMultipleWindows/

https://codereview.chromium.org/2763893004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3813:
const ash::ShelfID shelf_app_id =
nit: name this |shelf_id|, it's not an app_id string.

https://codereview.chromium.org/2763893004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3819:
// Selecting the item does not change active window.
nit: maybe "Selecting the item will show its application menu." plus optional "
It does not change the active window." ditto below

https://codereview.chromium.org/2763893004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3824:
// Command ids is just app window indices. Note, apps is registered in
nit: s/is/are/ in both cases

[khmel, 2017-03-22 00:39:58]

Thank you for quick review!

https://codereview.chromium.org/2763893004/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2763893004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3789:
// Test that validates correctness of switching multiple shelf item for the same
On 2017/03/22 00:30:15, msw wrote:
> nit: maybe "Test the application menu of a shelf item with multiple ARC
> windows."

Done.

https://codereview.chromium.org/2763893004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3791:
TEST_P(ChromeLauncherControllerImplWithArcTest, MultipleItemsInShelf) {
On 2017/03/22 00:30:15, msw wrote:
> nit: maybe s/MultipleItemsInShelf/ShelfItemWithMultipleWindows/

Done.

https://codereview.chromium.org/2763893004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3813:
const ash::ShelfID shelf_app_id =
On 2017/03/22 00:30:15, msw wrote:
> nit: name this |shelf_id|, it's not an app_id string.

Done.

https://codereview.chromium.org/2763893004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3819:
// Selecting the item does not change active window.
On 2017/03/22 00:30:15, msw wrote:
> nit: maybe "Selecting the item will show its application menu." plus optional
"
> It does not change the active window." ditto below

Done.

https://codereview.chromium.org/2763893004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3824:
// Command ids is just app window indices. Note, apps is registered in
On 2017/03/22 00:30:15, msw wrote:
> nit: s/is/are/ in both cases

Done.

[khmel, 2017-03-22 00:40:35]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-03-22 00:40:36]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2763893004/#ps20001
(title: "comments updated")

[commit-bot: I haz the power, 2017-03-22 00:41:11]

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-22 02:42:45]

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

[commit-bot: I haz the power, 2017-03-22 02:42:45]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[khmel, 2017-03-22 03:02:33]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2017-03-22 03:03:51]

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-22 05:21:59]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490151753867930,
"parent_rev": "7ac6ca38bc4d106dd0f1580894a4d08beb757149", "commit_rev":
"a19209825989de7d6b620aea95dd0f95b5c3c03f"}

[commit-bot: I haz the power, 2017-03-22 05:23:11]

Description was changed from

==========
arc: Fix regression in handling ARC shelf item.

This fix regressions caused by recent refactoring. First regression is
in case multiple shelf items for the same ARC app, clicking on shelf
item always activates last opened window. It is expected that in this
case last active window of that app becomes active. Second one,
selecting which item to activate from menu does not actually activate
selected item

BUG=703895
TEST=Manually, unit_tests extended.
==========

to

==========
arc: Fix regression in handling ARC shelf item.

This fix regressions caused by recent refactoring. First regression is
in case multiple shelf items for the same ARC app, clicking on shelf
item always activates last opened window. It is expected that in this
case last active window of that app becomes active. Second one,
selecting which item to activate from menu does not actually activate
selected item

BUG=703895
TEST=Manually, unit_tests extended.

Review-Url: https://codereview.chromium.org/2763893004
Cr-Commit-Position: refs/heads/master@{#458638}
Committed:
https://chromium.googlesource.com/chromium/src/+/a19209825989de7d6b620aea95dd...
==========

[commit-bot: I haz the power, 2017-03-22 05:23:12]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a19209825989de7d6b620aea95dd...