Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2739323004:
arc: Reactivate OptIn flow on clicking Play Store. (Closed)
|
Created:
3 years, 9 months ago by khmel Modified:
3 years, 9 months ago 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. |
Descriptionarc: Reactivate OptIn flow on clicking Play Store.
This fixes broken flow when ARC OptIn can be re-activated for managed
user in case user clicks Play Store icon in App Launcher or on shelf.
TEST=Manually + unit_test added
BUG=700601
Review-Url: https://codereview.chromium.org/2739323004
Cr-Commit-Position: refs/heads/master@{#457107}
Committed: https://chromium.googlesource.com/chromium/src/+/919bb1a623fd23ecbcd3664ff45d63cec36da977
Patch Set 1 #
Total comments: 8
Patch Set 2 : rebased + nits #Patch Set 3 : update unit tests #Patch Set 4 : comment extended, fixed forgotten unit_test #
Total comments: 4
Patch Set 5 : TODO added/rebased #
Messages
Total messages: 36 (19 generated)
khmel@chromium.org changed reviewers: + yusukes@chromium.org
Hi Yusuke, PTAL Thanks!
Sorry for the delay. Can you run git cl try? https://codereview.chromium.org/2739323004/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2739323004/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_utils.cc:299: if (app_id == kPlayStoreAppId) { qq: When this can be false? Is it okay to execute L298 even when this evaluates to false? https://codereview.chromium.org/2739323004/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/2739323004/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:34: // Play Store should always be registered and arc::LaunchApp handles can nit: s/handles// ? https://codereview.chromium.org/2739323004/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h (right): https://codereview.chromium.org/2739323004/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h:18: ArcPlaystoreShortcutLauncherItemController( (while you're at it) please add explicit.
The CQ bit was checked by khmel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by khmel@chromium.org to run a CQ dry run
Thank you Yusuke for comments. PTAL updated version. https://codereview.chromium.org/2739323004/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2739323004/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_utils.cc:299: if (app_id == kPlayStoreAppId) { On 2017/03/13 18:05:56, Yusuke Sato wrote: > qq: When this can be false? Is it okay to execute L298 even when this evaluates > to false? Another default app may activate this flow, for example ArtCanvas. In this case we have spinning animation for ArtCanvas. Play Store is also implemented as default app. We have OptIn UI for this flow and don't show animation in shelf. Return here is just exit earlier and don't register deferred launcher which is responsible for spinning animation. Similar to L285 https://codereview.chromium.org/2739323004/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/2739323004/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:34: // Play Store should always be registered and arc::LaunchApp handles can On 2017/03/13 18:05:56, Yusuke Sato wrote: > nit: s/handles// ? Done. https://codereview.chromium.org/2739323004/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h (right): https://codereview.chromium.org/2739323004/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h:18: ArcPlaystoreShortcutLauncherItemController( On 2017/03/13 18:05:56, Yusuke Sato wrote: > (while you're at it) please add explicit. Good catch, don't know how gl cl upload passed this before
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
arc/ lgtm with a comment: https://codereview.chromium.org/2739323004/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2739323004/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_utils.cc:299: if (app_id == kPlayStoreAppId) { On 2017/03/13 19:44:19, khmel wrote: > On 2017/03/13 18:05:56, Yusuke Sato wrote: > > qq: When this can be false? Is it okay to execute L298 even when this > evaluates > > to false? > > Another default app may activate this flow, for example ArtCanvas. In this case > we have spinning animation for ArtCanvas. Play Store is also implemented as > default app. We have OptIn UI for this flow and don't show animation in shelf. > Return here is just exit earlier and don't register deferred launcher which is > responsible for spinning animation. Similar to L285 Can you copy the summary of your comment to the code as a code comment? This was not immediately obvious to me.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
khmel@chromium.org changed reviewers: + oshima@chromium.org
Thank you Yusuke for review. adding oshima@ for controller review. PTAL, Thanks. https://codereview.chromium.org/2739323004/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2739323004/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_utils.cc:299: if (app_id == kPlayStoreAppId) { On 2017/03/13 20:08:18, Yusuke Sato wrote: > On 2017/03/13 19:44:19, khmel wrote: > > On 2017/03/13 18:05:56, Yusuke Sato wrote: > > > qq: When this can be false? Is it okay to execute L298 even when this > > evaluates > > > to false? > > > > Another default app may activate this flow, for example ArtCanvas. In this > case > > we have spinning animation for ArtCanvas. Play Store is also implemented as > > default app. We have OptIn UI for this flow and don't show animation in shelf. > > Return here is just exit earlier and don't register deferred launcher which is > > responsible for spinning animation. Similar to L285 > > Can you copy the summary of your comment to the code as a code comment? This was > not immediately obvious to me. Done.
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
drive-by https://codereview.chromium.org/2739323004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2739323004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:123: ArcSessionManager* arc_session_manager = ArcSessionManager::Get(); QQ: If this is only for launcher icon click case, can this move to there instead of here? This function is used in various places, I'm slightly worried about its side-effect. Also, with this change, we cannot rely on the assumption IsArcPlayStoreEnabled() == ARC is running, any more. So, e.g., settings page etc. may not work? Is this safely doable?
https://codereview.chromium.org/2739323004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2739323004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:123: ArcSessionManager* arc_session_manager = ArcSessionManager::Get(); On 2017/03/13 21:46:47, hidehiko wrote: > QQ: If this is only for launcher icon click case, can this move to there instead > of here? > This function is used in various places, I'm slightly worried about its > side-effect. > Also, with this change, we cannot rely on the assumption IsArcPlayStoreEnabled() > == ARC is running, any more. > So, e.g., settings page etc. may not work? Is this safely doable? This is not only for launcher item case. My first attempt was to add RequestEnable into arc_app_utils. However this was not enough. One problem is when user closes OptIn UI. In this case this function is called from host handler IIRC. We have to reset enable_request in session manager in order to get a chance to reactivate negotiator. We should not worry about settings case because user cannot change arc.enabled state (which settings represents actually)
c/b/ui/ash lgtm
Thank you for review!
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2739323004/#ps60001 (title: "comment extended, fixed forgotten unit_test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2739323004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2739323004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:123: ArcSessionManager* arc_session_manager = ArcSessionManager::Get(); On 2017/03/13 21:58:49, khmel wrote: > On 2017/03/13 21:46:47, hidehiko wrote: > > QQ: If this is only for launcher icon click case, can this move to there > instead > > of here? > > This function is used in various places, I'm slightly worried about its > > side-effect. > > Also, with this change, we cannot rely on the assumption > IsArcPlayStoreEnabled() > > == ARC is running, any more. > > So, e.g., settings page etc. may not work? Is this safely doable? > > This is not only for launcher item case. My first attempt was to add > RequestEnable into arc_app_utils. However this was not enough. One problem is > when user closes OptIn UI. In this case this function is called from host > handler IIRC. We have to reset enable_request in session manager in order to get > a chance to reactivate negotiator. We should not worry about settings case > because user cannot change arc.enabled state (which settings represents > actually) If this is short term fix that's ok, but please add TODO to remove this. Actually this is not SetArcPlayStoreEnabledForProfile conceptually. Also please make sure components IsArcPlayStoreEnabledForProfile() does not break by this change.
On 2017/03/15 00:45:00, hidehiko wrote: > https://codereview.chromium.org/2739323004/diff/60001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_util.cc (right): > > https://codereview.chromium.org/2739323004/diff/60001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_util.cc:123: ArcSessionManager* > arc_session_manager = ArcSessionManager::Get(); > On 2017/03/13 21:58:49, khmel wrote: > > On 2017/03/13 21:46:47, hidehiko wrote: > > > QQ: If this is only for launcher icon click case, can this move to there > > instead > > > of here? > > > This function is used in various places, I'm slightly worried about its > > > side-effect. > > > Also, with this change, we cannot rely on the assumption > > IsArcPlayStoreEnabled() > > > == ARC is running, any more. > > > So, e.g., settings page etc. may not work? Is this safely doable? > > > > This is not only for launcher item case. My first attempt was to add > > RequestEnable into arc_app_utils. However this was not enough. One problem is > > when user closes OptIn UI. In this case this function is called from host > > handler IIRC. We have to reset enable_request in session manager in order to > get > > a chance to reactivate negotiator. We should not worry about settings case > > because user cannot change arc.enabled state (which settings represents > > actually) > > If this is short term fix that's ok, but please add TODO to remove this. > Actually this is not SetArcPlayStoreEnabledForProfile conceptually. > Also please make sure components IsArcPlayStoreEnabledForProfile() does not > break by this change. Sorry, typo... "components using IsArcPlayStoreEnabledForProfile()" I meant.
khmel@chromium.org changed reviewers: + xiyuan@chromium.org
Hi Xiyuan, PTAL Thanks! https://codereview.chromium.org/2739323004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2739323004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:123: ArcSessionManager* arc_session_manager = ArcSessionManager::Get(); On 2017/03/15 00:45:00, hidehiko wrote: > On 2017/03/13 21:58:49, khmel wrote: > > On 2017/03/13 21:46:47, hidehiko wrote: > > > QQ: If this is only for launcher icon click case, can this move to there > > instead > > > of here? > > > This function is used in various places, I'm slightly worried about its > > > side-effect. > > > Also, with this change, we cannot rely on the assumption > > IsArcPlayStoreEnabled() > > > == ARC is running, any more. > > > So, e.g., settings page etc. may not work? Is this safely doable? > > > > This is not only for launcher item case. My first attempt was to add > > RequestEnable into arc_app_utils. However this was not enough. One problem is > > when user closes OptIn UI. In this case this function is called from host > > handler IIRC. We have to reset enable_request in session manager in order to > get > > a chance to reactivate negotiator. We should not worry about settings case > > because user cannot change arc.enabled state (which settings represents > > actually) > > If this is short term fix that's ok, but please add TODO to remove this. > Actually this is not SetArcPlayStoreEnabledForProfile conceptually. > Also please make sure components IsArcPlayStoreEnabledForProfile() does not > break by this change. TODO added, IsArcPlayStoreEnabledForProfile is always true for this case.
lgtm
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2739323004/#ps80001 (title: "TODO added/rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489593942331410,
"parent_rev": "2bfe02ea896c0fe595f3eb3ab6af4fc977554ac3", "commit_rev":
"919bb1a623fd23ecbcd3664ff45d63cec36da977"}
Message was sent while issue was closed.
Description was changed from ========== arc: Reactivate OptIn flow on clicking Play Store. This fixes broken flow when ARC OptIn can be re-activated for managed user in case user clicks Play Store icon in App Launcher or on shelf. TEST=Manually + unit_test added BUG=700601 ========== to ========== arc: Reactivate OptIn flow on clicking Play Store. This fixes broken flow when ARC OptIn can be re-activated for managed user in case user clicks Play Store icon in App Launcher or on shelf. TEST=Manually + unit_test added BUG=700601 Review-Url: https://codereview.chromium.org/2739323004 Cr-Commit-Position: refs/heads/master@{#457107} Committed: https://chromium.googlesource.com/chromium/src/+/919bb1a623fd23ecbcd3664ff45d... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/919bb1a623fd23ecbcd3664ff45d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
