arc: Fix race conditon when Play Store is started too early.

arc: Fix race conditon when Play Store is started too early.

This fixes race condition when Play Store OptIn is started from
the shelf, in case default apps list has not been read yet.

BUG=b/36279017
TEST=Unit tests extended and manually on device simulating race
     conditions.

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

[khmel, 2017-03-15 22:46:25]

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

[commit-bot: I haz the power, 2017-03-15 22:46:45]

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

[khmel, 2017-03-15 22:51:48]

khmel@chromium.org changed reviewers:
+ hidehiko@chromium.org, oshima@chromium.org, xiyuan@chromium.org

[khmel, 2017-03-15 22:51:48]

Hi,

PTAL

Thanks!

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

https://codereview.chromium.org/2751913004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:34:

I discarded similar code in one of my previous CL:
https://codereview.chromium.org/2739323004/diff/80001/chrome/browser/ui/ash/l...

however that code was outdated and did not work as expected anyway.

[xiyuan, 2017-03-15 23:01:47]

lgtm

https://codereview.chromium.org/2751913004/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2751913004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:261: new
ArcAppLauncher(profile_, kPlayStoreAppId, true, false));
nit: Annotate the bool args since there are multiple ones now.

[commit-bot: I haz the power, 2017-03-15 23:17:50]

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

[commit-bot: I haz the power, 2017-03-15 23:17:51]

Dry run: This issue passed the CQ dry run.

[oshima, 2017-03-15 23:27:47]

lgtm

[hidehiko, 2017-03-16 01:05:39]

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

https://codereview.chromium.org/2751913004/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_launcher.cc:57: if
(!arc::LaunchApp(context_, app_id_, landscape_layout_))
(I know this is not a main focus of this CL, but...)

This looks a bug.
LaunchApp() has two signature:
LaunchApp(BrowserContext*, const std::string&, int event_flags);
LaunchApp(BrowserContext*, const std::string&, bool landscape_layout, int
event_flags);

We should call latter, so here should be LaunchApp(context_, app_id_,
landscape_layout_, ui::EF_NONE);

https://codereview.chromium.org/2751913004/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_launcher.h (right):

https://codereview.chromium.org/2751913004/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_launcher.h:21:
ArcAppLauncher(content::BrowserContext* context,
Please document what "deferred_launch_allowed" is.

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

https://codereview.chromium.org/2751913004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:35:
if (arc_app_prefs->IsRegistered(arc::kPlayStoreAppId)) {
L35-L37 is just a dup logic with ArcAppLauncher's ctor. So, simply

playstore_launcher_ = base::MakeUnique<ArcAppLauncher>(
    profile, arc::kPlayStoreAppId, true, true);

should work.

[khmel, 2017-03-16 16:12:14]

Thanks for your comments

PTAL

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

https://codereview.chromium.org/2751913004/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_launcher.cc:57: if
(!arc::LaunchApp(context_, app_id_, landscape_layout_))
On 2017/03/16 01:05:39, hidehiko wrote:
> (I know this is not a main focus of this CL, but...)
> 
> This looks a bug.
> LaunchApp() has two signature:
> LaunchApp(BrowserContext*, const std::string&, int event_flags);
> LaunchApp(BrowserContext*, const std::string&, bool landscape_layout, int
> event_flags);
> 
> We should call latter, so here should be LaunchApp(context_, app_id_,
> landscape_layout_, ui::EF_NONE);

Nice catch! thanks

https://codereview.chromium.org/2751913004/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_launcher.h (right):

https://codereview.chromium.org/2751913004/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_launcher.h:21:
ArcAppLauncher(content::BrowserContext* context,
On 2017/03/16 01:05:39, hidehiko wrote:
> Please document what "deferred_launch_allowed" is.

Done.

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

https://codereview.chromium.org/2751913004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:35:
if (arc_app_prefs->IsRegistered(arc::kPlayStoreAppId)) {
On 2017/03/16 01:05:39, hidehiko wrote:
> L35-L37 is just a dup logic with ArcAppLauncher's ctor. So, simply
> 
> playstore_launcher_ = base::MakeUnique<ArcAppLauncher>(
>     profile, arc::kPlayStoreAppId, true, true);
> 
> should work.
> 

Nice, Done.

[hidehiko, 2017-03-16 16:23:19]

LGTM with a few minor comments.
Please take a look at them before landing.

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

https://codereview.chromium.org/2751913004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_test.cc:110: arc_app_list_pref_ =
ArcAppListPrefs::Get(profile_);
Initializing twice looks not straightforward.

Could you:

move L110-L111 to L97 (just before "if (wait_default_apps_)" block),
and remove L118?
Note: Keeping DCHECK at L118 should make sense, meaning arc_app_list_pref_ needs
to be set beforehand, IMO.

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

https://codereview.chromium.org/2751913004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:434: virtual bool
IsNeedWaitDefaultApps() const { return true; }
nit: IsWaitDefaultAppsNeeded() looks more common pattern in Chrome?

https://codereview.chromium.org/2751913004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:1386: printf("#### %s %d
%d\n", app_id.c_str(), oem_states[app_id],
Looks debugging purpose? Could you remove?

https://codereview.chromium.org/2751913004/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_utils.h (right):

https://codereview.chromium.org/2751913004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.h:46: // TODO (khmel) Rename this
method to reduce the chance of using another
Nice comment!

Or, maybe replacing "bool" for layout with enum class, so that implicit
conversion won't be triggered.

enum class Layout {
  PORTRAIT,
  LANDSCAPE,
};

bool LaunchApp(content::BrowserContext* context,
               const std::string& app_id,
               Layout layout,
               int event_flags);

Note: not necessary to address it in this CL, I think.

[khmel, 2017-03-16 16:38:48]

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

https://codereview.chromium.org/2751913004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_test.cc:110: arc_app_list_pref_ =
ArcAppListPrefs::Get(profile_);
On 2017/03/16 16:23:19, hidehiko wrote:
> Initializing twice looks not straightforward.
> 
> Could you:
> 
> move L110-L111 to L97 (just before "if (wait_default_apps_)" block),
> and remove L118?
> Note: Keeping DCHECK at L118 should make sense, meaning arc_app_list_pref_
needs
> to be set beforehand, IMO.

Done.

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

https://codereview.chromium.org/2751913004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:434: virtual bool
IsNeedWaitDefaultApps() const { return true; }
On 2017/03/16 16:23:19, hidehiko wrote:
> nit: IsWaitDefaultAppsNeeded() looks more common pattern in Chrome?

Done.

https://codereview.chromium.org/2751913004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:1386: printf("#### %s %d
%d\n", app_id.c_str(), oem_states[app_id],
On 2017/03/16 16:23:19, hidehiko wrote:
> Looks debugging purpose? Could you remove?

Hmm, why git cl upload did not catch this? Done

https://codereview.chromium.org/2751913004/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_utils.h (right):

https://codereview.chromium.org/2751913004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.h:46: // TODO (khmel) Rename this
method to reduce the chance of using another
On 2017/03/16 16:23:19, hidehiko wrote:
> Nice comment!
> 
> Or, maybe replacing "bool" for layout with enum class, so that implicit
> conversion won't be triggered.
> 
> enum class Layout {
>   PORTRAIT,
>   LANDSCAPE,
> };
> 
> bool LaunchApp(content::BrowserContext* context,
>                const std::string& app_id,
>                Layout layout,
>                int event_flags);
> 
> Note: not necessary to address it in this CL, I think.

Nice alternative

[khmel, 2017-03-16 16:38:54]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-03-16 16:38:54]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
oshima@chromium.org, hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/2751913004/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2017-03-16 16:39:27]

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-16 17:16:49]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489682334337460,
"parent_rev": "d7c3d43fe9c81a9c2856e6c45abfc0724ae20e0d", "commit_rev":
"c281fac8706e3002c31696a1e7343762f0eb038f"}

[commit-bot: I haz the power, 2017-03-16 17:17:30]

Description was changed from

==========
arc: Fix race conditon when Play Store is started too early.

This fixes race condition when Play Store OptIn is started from
the shelf, in case default apps list has not been read yet.

BUG=b/36279017
TEST=Unit tests extended and manually on device simulating race
     conditions.
==========

to

==========
arc: Fix race conditon when Play Store is started too early.

This fixes race condition when Play Store OptIn is started from
the shelf, in case default apps list has not been read yet.

BUG=b/36279017
TEST=Unit tests extended and manually on device simulating race
     conditions.

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

[commit-bot: I haz the power, 2017-03-16 17:17:32]

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