arc: Make shortcuts runnable in deferred mode.

arc: Make shortcuts runnable in deferred mode.

BUG=708792
BUG=b/36990317
TEST=Manually, unit_tests extended

Review-Url: https://codereview.chromium.org/2799813003
Cr-Commit-Position: refs/heads/master@{#462324}
Committed: https://chromium.googlesource.com/chromium/src/+/58df084dd54feed0de177c707cb26b004c05cf68

[khmel, 2017-04-05 22:18:01]

Description was changed from

==========
arc: Make shorcuts runnablee in deferred mode.

BUG=708792
BUG=b/36990317
TEST=Manually, unit_tests extended
==========

to

==========
arc: Make shortcuts runnable in deferred mode.

BUG=708792
BUG=b/36990317
TEST=Manually, unit_tests extended
==========

[khmel, 2017-04-05 22:19:51]

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

[khmel, 2017-04-05 22:19:51]

Hi Xiyuan and Mike

PTAL

Thank you!

https://codereview.chromium.org/2799813003/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2799813003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:984:
NotifyAppReadyChanged(app_id, true);
This is the actual reason of this bug. Ready state change was not dispatched for
shortcuts.

[xiyuan, 2017-04-05 22:37:19]

lgtm

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

https://codereview.chromium.org/2799813003/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1923:
// Not deferred contollers should go away together with shelf items and ARC
nit: Not -> Note ?

[khmel, 2017-04-05 22:40:54]

Thank you Xiyuan for review!

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

https://codereview.chromium.org/2799813003/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1923:
// Not deferred contollers should go away together with shelf items and ARC
On 2017/04/05 22:37:18, xiyuan wrote:
> nit: Not -> Note ?

Now :)

[msw, 2017-04-06 00:23:54]

lgtm with minor qs/comments.

https://codereview.chromium.org/2799813003/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2799813003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:645:
NotifyAppReadyChanged(app_id, false);
Should this be run regardless of default_apps_.HasApp?

https://codereview.chromium.org/2799813003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:982: if
(!ready_apps_.count(app_id)) {
The top of the block skips cases where |ready_apps_.count(app_id)| is non-zero,
so this check should always be true; DCHECK instead?

[msw, 2017-04-06 00:46:27]

Also: I can't access the buganizer issue; the crbug issue should have the
details.

[khmel, 2017-04-06 01:31:43]

Thanks for review Mike!

https://codereview.chromium.org/2799813003/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2799813003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:645:
NotifyAppReadyChanged(app_id, false);
On 2017/04/06 00:23:53, msw wrote:
> Should this be run regardless of default_apps_.HasApp?

For default apps this is actually switching to disabled states. For all others
apps this is removal operation and it is called at L641

In conclusion:
For default apps: OnAppReadyChanged() notification
For other apps: OnAppRemoved() notification.

https://codereview.chromium.org/2799813003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:982: if
(!ready_apps_.count(app_id)) {
On 2017/04/06 00:23:53, msw wrote:
> The top of the block skips cases where |ready_apps_.count(app_id)| is
non-zero,
> so this check should always be true; DCHECK instead?

Nice catch, I reverted to my first impl :) when no check was used.

[khmel, 2017-04-06 01:31:48]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-04-06 01:31:49]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2799813003/#ps40001
(title: "remove not-needed check")

[commit-bot: I haz the power, 2017-04-06 01:32:50]

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-04-06 02:05:09]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491442308976470,
"parent_rev": "b65623a9c0565962586271410beb98e53c2beaee", "commit_rev":
"58df084dd54feed0de177c707cb26b004c05cf68"}

[commit-bot: I haz the power, 2017-04-06 02:05:58]

Description was changed from

==========
arc: Make shortcuts runnable in deferred mode.

BUG=708792
BUG=b/36990317
TEST=Manually, unit_tests extended
==========

to

==========
arc: Make shortcuts runnable in deferred mode.

BUG=708792
BUG=b/36990317
TEST=Manually, unit_tests extended

Review-Url: https://codereview.chromium.org/2799813003
Cr-Commit-Position: refs/heads/master@{#462324}
Committed:
https://chromium.googlesource.com/chromium/src/+/58df084dd54feed0de177c707cb2...
==========

[commit-bot: I haz the power, 2017-04-06 02:05:59]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/58df084dd54feed0de177c707cb2...