Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(9)

Issue 2751913004: arc: Fix race conditon when Play Store is started too early. (Closed)

Created:
3 years, 9 months ago by khmel
Modified:
3 years, 9 months ago
Reviewers:
hidehiko, xiyuan, oshima
CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, tfarina, hidehiko+watch_chromium.org, sadrul, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, kalyank, davemoore+watch_chromium.org, Matt Giuca
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 8

Patch Set 2 : adressed Hidehiko's comments #

Total comments: 8

Patch Set 3 : nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+100 lines, -27 lines) Patch
M chrome/browser/chromeos/arc/arc_session_manager.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/app_list/arc/arc_app_launcher.h View 1 2 chunks +7 lines, -2 lines 0 comments Download
M chrome/browser/ui/app_list/arc/arc_app_launcher.cc View 1 3 chunks +11 lines, -6 lines 0 comments Download
M chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/app_list/arc/arc_app_test.h View 3 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/ui/app_list/arc/arc_app_test.cc View 1 2 2 chunks +9 lines, -3 lines 0 comments Download
M chrome/browser/ui/app_list/arc/arc_app_unittest.cc View 1 2 5 chunks +54 lines, -3 lines 0 comments Download
M chrome/browser/ui/app_list/arc/arc_app_utils.h View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h View 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc View 1 2 chunks +4 lines, -11 lines 0 comments Download

Messages

Total messages: 18 (9 generated)
khmel
Hi, PTAL Thanks! https://codereview.chromium.org/2751913004/diff/1/chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc 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/launcher/arc_playstore_shortcut_launcher_item_controller.cc#newcode34 chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:34: I discarded similar code in one ...
3 years, 9 months ago (2017-03-15 22:51:48 UTC) #4
xiyuan
lgtm https://codereview.chromium.org/2751913004/diff/1/chrome/browser/chromeos/arc/arc_session_manager.cc File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2751913004/diff/1/chrome/browser/chromeos/arc/arc_session_manager.cc#newcode261 chrome/browser/chromeos/arc/arc_session_manager.cc:261: new ArcAppLauncher(profile_, kPlayStoreAppId, true, false)); nit: Annotate the ...
3 years, 9 months ago (2017-03-15 23:01:47 UTC) #5
oshima
lgtm
3 years, 9 months ago (2017-03-15 23:27:47 UTC) #8
hidehiko
https://codereview.chromium.org/2751913004/diff/1/chrome/browser/ui/app_list/arc/arc_app_launcher.cc File chrome/browser/ui/app_list/arc/arc_app_launcher.cc (right): https://codereview.chromium.org/2751913004/diff/1/chrome/browser/ui/app_list/arc/arc_app_launcher.cc#newcode57 chrome/browser/ui/app_list/arc/arc_app_launcher.cc:57: if (!arc::LaunchApp(context_, app_id_, landscape_layout_)) (I know this is not ...
3 years, 9 months ago (2017-03-16 01:05:39 UTC) #9
khmel
Thanks for your comments PTAL https://codereview.chromium.org/2751913004/diff/1/chrome/browser/ui/app_list/arc/arc_app_launcher.cc File chrome/browser/ui/app_list/arc/arc_app_launcher.cc (right): https://codereview.chromium.org/2751913004/diff/1/chrome/browser/ui/app_list/arc/arc_app_launcher.cc#newcode57 chrome/browser/ui/app_list/arc/arc_app_launcher.cc:57: if (!arc::LaunchApp(context_, app_id_, landscape_layout_)) ...
3 years, 9 months ago (2017-03-16 16:12:14 UTC) #10
hidehiko
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_list/arc/arc_app_test.cc ...
3 years, 9 months ago (2017-03-16 16:23:19 UTC) #11
khmel
https://codereview.chromium.org/2751913004/diff/20001/chrome/browser/ui/app_list/arc/arc_app_test.cc File chrome/browser/ui/app_list/arc/arc_app_test.cc (right): https://codereview.chromium.org/2751913004/diff/20001/chrome/browser/ui/app_list/arc/arc_app_test.cc#newcode110 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: > ...
3 years, 9 months ago (2017-03-16 16:38:48 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2751913004/40001
3 years, 9 months ago (2017-03-16 16:39:27 UTC) #15
commit-bot: I haz the power
3 years, 9 months ago (2017-03-16 17:17:32 UTC) #18
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c281fac8706e3002c31696a1e734...

Powered by Google App Engine
This is Rietveld 408576698