Remove LauncherItemController::IsOpen, check ShelfItem::status.

Remove LauncherItemController::IsOpen, check ShelfItem::status.

Helps toward landing https://codereview.chromium.org/2462753002

BUG=557406
TEST=Automated; no shelf item context menu changes (to 'Close').
R=jamescook@chromium.org

Committed: https://crrev.com/39139eb6552be981119ef7508b4101f095228a40
Cr-Commit-Position: refs/heads/master@{#431167}

[msw, 2016-11-09 20:42:02]

Description was changed from

==========
Remove ShelfItemDelegate::IsOpen, check ShelfItem::status.

BUG=557406
TEST=Automated; no shelf item context menu changes (to 'Close').
R=jamescook@chromium.org
==========

to

==========
Remove ShelfItemDelegate::IsOpen, check ShelfItem::status.

Helps toward landing https://codereview.chromium.org/2462753002

BUG=557406
TEST=Automated; no shelf item context menu changes (to 'Close').
R=jamescook@chromium.org
==========

[msw, 2016-11-09 20:42:27]

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

[msw, 2016-11-09 20:42:38]

Hey James, please take a look; thanks!

[commit-bot: I haz the power, 2016-11-09 20:42:51]

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

[James Cook, 2016-11-09 21:04:34]

LGTM assuming you can verify that the ARC++ case works as expected.

[commit-bot: I haz the power, 2016-11-09 21:19:20]

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

[commit-bot: I haz the power, 2016-11-09 21:19:20]

Dry run: This issue passed the CQ dry run.

[msw, 2016-11-10 01:04:50]

Description was changed from

==========
Remove ShelfItemDelegate::IsOpen, check ShelfItem::status.

Helps toward landing https://codereview.chromium.org/2462753002

BUG=557406
TEST=Automated; no shelf item context menu changes (to 'Close').
R=jamescook@chromium.org
==========

to

==========
Remove LauncherItemController::IsOpen, check ShelfItem::status.

Helps toward landing https://codereview.chromium.org/2462753002

BUG=557406
TEST=Automated; no shelf item context menu changes (to 'Close').
R=jamescook@chromium.org
==========

[msw, 2016-11-10 01:27:17]

I verified that this works manually, and added browser tests; landing now, but
lmk if you have any comments.

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

https://codereview.chromium.org/2487953003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:462: bool
ChromeLauncherControllerImpl::IsOpen(ash::ShelfID id) {
FYI, this is only used by ArcAppContextMenu::BuildMenu.
khmel@ and I think this could be replaced in a followup.

[msw, 2016-11-10 01:27:24]

The CQ bit was checked by msw@chromium.org

[msw, 2016-11-10 01:27:24]

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/2487953003/#ps40001
(title: "Cleanup.")

[commit-bot: I haz the power, 2016-11-10 01:27:45]

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

[khmel, 2016-11-10 01:31:26]

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

[khmel, 2016-11-10 01:31:26]

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

https://codereview.chromium.org/2487953003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc:376:
EXPECT_FALSE(delegate->IsAppOpen(app_id));
Should not it be EXPECT_TRUE()?

[msw, 2016-11-10 01:34:37]

The CQ bit was unchecked by msw@chromium.org

[James Cook, 2016-11-10 01:39:53]

still lgtm modulo khmel's comment

https://codereview.chromium.org/2487953003/diff/40001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_controller_ash_browsertest.cc
(right):

https://codereview.chromium.org/2487953003/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_controller_ash_browsertest.cc:19:
AppListControllerDelegate* delegate = &delegate_ash;
optional nit: This is a little odd. I see you are calling private methods in
AppListControllerDelegateAsh via their public versions in
AppListControllerDelegate. That's OK, but I think just making the derived
interface public would be a little less odd.

If you prefer this way a comment would be nice.

[msw, 2016-11-10 02:31:58]

Patchset #4 (id:60001) has been deleted

[msw, 2016-11-10 02:39:10]

Comments addressed; thanks!

https://codereview.chromium.org/2487953003/diff/40001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_controller_ash_browsertest.cc
(right):

https://codereview.chromium.org/2487953003/diff/40001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_controller_ash_browsertest.cc:19:
AppListControllerDelegate* delegate = &delegate_ash;
On 2016/11/10 01:39:53, James Cook wrote:
> optional nit: This is a little odd. I see you are calling private methods in
> AppListControllerDelegateAsh via their public versions in
> AppListControllerDelegate. That's OK, but I think just making the derived
> interface public would be a little less odd.
> 
> If you prefer this way a comment would be nice.

Done (made functions public).

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

https://codereview.chromium.org/2487953003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc:376:
EXPECT_FALSE(delegate->IsAppOpen(app_id));
On 2016/11/10 01:31:26, khmel wrote:
> Should not it be EXPECT_TRUE()?

You're right; good catch! Fixed with a fake task creation.

[msw, 2016-11-10 02:39:18]

The CQ bit was checked by msw@chromium.org

[msw, 2016-11-10 02:39:18]

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/2487953003/#ps80001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-11-10 02:39:41]

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, 2016-11-10 03:34:24]

Description was changed from

==========
Remove LauncherItemController::IsOpen, check ShelfItem::status.

Helps toward landing https://codereview.chromium.org/2462753002

BUG=557406
TEST=Automated; no shelf item context menu changes (to 'Close').
R=jamescook@chromium.org
==========

to

==========
Remove LauncherItemController::IsOpen, check ShelfItem::status.

Helps toward landing https://codereview.chromium.org/2462753002

BUG=557406
TEST=Automated; no shelf item context menu changes (to 'Close').
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2016-11-10 03:34:25]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-11-10 03:36:01]

Description was changed from

==========
Remove LauncherItemController::IsOpen, check ShelfItem::status.

Helps toward landing https://codereview.chromium.org/2462753002

BUG=557406
TEST=Automated; no shelf item context menu changes (to 'Close').
R=jamescook@chromium.org
==========

to

==========
Remove LauncherItemController::IsOpen, check ShelfItem::status.

Helps toward landing https://codereview.chromium.org/2462753002

BUG=557406
TEST=Automated; no shelf item context menu changes (to 'Close').
R=jamescook@chromium.org

Committed: https://crrev.com/39139eb6552be981119ef7508b4101f095228a40
Cr-Commit-Position: refs/heads/master@{#431167}
==========

[commit-bot: I haz the power, 2016-11-10 03:36:02]

Patchset 4 (id:??) landed as
https://crrev.com/39139eb6552be981119ef7508b4101f095228a40
Cr-Commit-Position: refs/heads/master@{#431167}