Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2745533005:
Show notification during ARC managed provision (Closed)
|
Created:
3 years, 9 months ago by emaxx Modified:
3 years, 8 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org, yusukes+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow notification during ARC managed provision
Display a notification during ARC provisioning when the opt-in flow
happens silently (without showing the Terms of Service screen)
due to enterprise policies.
The notification is removed either when provisioning finishes
successfully, or when it fails and some error is displayed.
BUG=700609
BUG=b/35865652
TEST=New unit tests;
Manual test: set up enterprise policies ArcEnabled,
ArcBackupRestoreEnabled, ArcLocationServiceEnabled,
log into a fresh profile, check that the notification about
ARC being installed is shown, and after 1-2 minutes the
notification goes away and Play Store is available.
Review-Url: https://codereview.chromium.org/2745533005
Cr-Commit-Position: refs/heads/master@{#464707}
Committed: https://chromium.googlesource.com/chromium/src/+/65e89bdb39e1f22f3032cfafdd3f2da1ba64a9b9
Patch Set 1 #Patch Set 2 : Add tests, clean up #
Total comments: 16
Patch Set 3 : Renaming, nit fixes #
Total comments: 6
Patch Set 4 : Rebase #Patch Set 5 : Address feedback. New icon. #
Total comments: 8
Patch Set 6 : Fix nits. Update notification message. #Patch Set 7 : Update message #Messages
Total messages: 63 (49 generated)
The CQ bit was checked by emaxx@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: This issue passed the CQ dry run.
The CQ bit was checked by emaxx@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: This issue passed the CQ dry run.
Description was changed from
==========
WIP: Show notification during ARC managed provision
BUG=
==========
to
==========
WIP: Show notification during ARC managed provision
Display a notification during ARC provisioning when the opt-in flow
happens silently (without showing the Terms of Service screen)
due to enterprise policies.
The notification is removed either when provisioning finishes
successfully, or when it fails and some error is displayed.
BUG=700609
BUG=b/35865652
TEST=New unit tests;
Manual test: set up enterprise policies ArbEnabled,
ArcBackupRestoreEnabled, ArcLocationServiceEnabled,
log into a fresh profile, check that the notification about
ARC being installed is shown, and after 1-2 minutes the
notification goes away and Play Store is available.
==========
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by emaxx@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...
emaxx@chromium.org changed reviewers: + hidehiko@chromium.org
Hidehiko, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:561: // TODO(hidehiko): DCHECK the previous state. This is called for four cases; Good catch :-) https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:960: observer.OnArcSupportHostErrorShown(error); Notifying when |support_host_| is null looks weird? Maybe either; - Notify only |support_host_|->ShowError() is called, or - Change the observer name and its concept to indicate that better. https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:112: // Called to notify that the error is requested to be displayed in the ARC support app can switch to an error page by itself (e.g., failing to fetch the ToS webpage), and this callback is not notified then. As long as this addresses your concern, I'm ok to add this in short term, but could you document it explicitly with TODO? https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... File chrome/browser/chromeos/arc/notification/arc_provision_notification_service_unittest.cc (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/notification/arc_provision_notification_service_unittest.cc:72: arc_service_manager_ = base::MakeUnique<ArcServiceManager>(nullptr); nit: not necessary.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:99: virtual void OnArcStartOptInAndroidManagementCheck() {} nit: Events should be OnArc[Phrase][Verb in past participle](), so this should be called OnArcAndroidOptInManagementCheckStarted() (or OnArcOptInManagementCheckStarted(), not sure how important it is to mention the "Android" part). https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... File chrome/browser/chromeos/arc/notification/arc_provision_notification_service.cc (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.cc:30: const char kManagedProvisionNotificationId[] = "arc_managed_provision"; nit: constexpr https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... File chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h:25: virtual ~Delegate() {} nit: define in the .cc with = default. https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h:28: }; nit: DISALLOW_COPY_AND_ASSIGN(Delegate);
Description was changed from
==========
WIP: Show notification during ARC managed provision
Display a notification during ARC provisioning when the opt-in flow
happens silently (without showing the Terms of Service screen)
due to enterprise policies.
The notification is removed either when provisioning finishes
successfully, or when it fails and some error is displayed.
BUG=700609
BUG=b/35865652
TEST=New unit tests;
Manual test: set up enterprise policies ArbEnabled,
ArcBackupRestoreEnabled, ArcLocationServiceEnabled,
log into a fresh profile, check that the notification about
ARC being installed is shown, and after 1-2 minutes the
notification goes away and Play Store is available.
==========
to
==========
Show notification during ARC managed provision
Display a notification during ARC provisioning when the opt-in flow
happens silently (without showing the Terms of Service screen)
due to enterprise policies.
The notification is removed either when provisioning finishes
successfully, or when it fails and some error is displayed.
BUG=700609
BUG=b/35865652
TEST=New unit tests;
Manual test: set up enterprise policies ArbEnabled,
ArcBackupRestoreEnabled, ArcLocationServiceEnabled,
log into a fresh profile, check that the notification about
ARC being installed is shown, and after 1-2 minutes the
notification goes away and Play Store is available.
==========
Description was changed from
==========
Show notification during ARC managed provision
Display a notification during ARC provisioning when the opt-in flow
happens silently (without showing the Terms of Service screen)
due to enterprise policies.
The notification is removed either when provisioning finishes
successfully, or when it fails and some error is displayed.
BUG=700609
BUG=b/35865652
TEST=New unit tests;
Manual test: set up enterprise policies ArbEnabled,
ArcBackupRestoreEnabled, ArcLocationServiceEnabled,
log into a fresh profile, check that the notification about
ARC being installed is shown, and after 1-2 minutes the
notification goes away and Play Store is available.
==========
to
==========
Show notification during ARC managed provision
Display a notification during ARC provisioning when the opt-in flow
happens silently (without showing the Terms of Service screen)
due to enterprise policies.
The notification is removed either when provisioning finishes
successfully, or when it fails and some error is displayed.
BUG=700609
BUG=b/35865652
TEST=New unit tests;
Manual test: set up enterprise policies ArcEnabled,
ArcBackupRestoreEnabled, ArcLocationServiceEnabled,
log into a fresh profile, check that the notification about
ARC being installed is shown, and after 1-2 minutes the
notification goes away and Play Store is available.
==========
The CQ bit was checked by emaxx@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: This issue passed the CQ dry run.
Updated the CL, thanks for the comments Hidehiko and Luis Héctor. Note that I'll have to hold on landing this until the UI review approval is received (see the b/ bug for the link). https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:960: observer.OnArcSupportHostErrorShown(error); On 2017/03/13 05:10:46, hidehiko wrote: > Notifying when |support_host_| is null looks weird? > Maybe either; > - Notify only |support_host_|->ShowError() is called, or > - Change the observer name and its concept to indicate that better. Done. Renamed to OnArcShowingErrorRequested, do you think this name is clear enough? https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:99: virtual void OnArcStartOptInAndroidManagementCheck() {} On 2017/03/13 15:50:41, Luis Héctor Chávez wrote: > nit: Events should be OnArc[Phrase][Verb in past participle](), so this should > be called OnArcAndroidOptInManagementCheckStarted() (or > OnArcOptInManagementCheckStarted(), not sure how important it is to mention the > "Android" part). Done. https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:112: // Called to notify that the error is requested to be displayed in the On 2017/03/13 05:10:46, hidehiko wrote: > ARC support app can switch to an error page by itself (e.g., failing to fetch > the ToS webpage), and this callback is not notified then. > > As long as this addresses your concern, I'm ok to add this in short term, but > could you document it explicitly with TODO? Done. Tried to address this by the name of the method and a note in the comment. https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... File chrome/browser/chromeos/arc/notification/arc_provision_notification_service.cc (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.cc:30: const char kManagedProvisionNotificationId[] = "arc_managed_provision"; On 2017/03/13 15:50:41, Luis Héctor Chávez wrote: > nit: constexpr Done. https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... File chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h:25: virtual ~Delegate() {} On 2017/03/13 15:50:41, Luis Héctor Chávez wrote: > nit: define in the .cc with = default. Done. https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h:28: }; On 2017/03/13 15:50:41, Luis Héctor Chávez wrote: > nit: DISALLOW_COPY_AND_ASSIGN(Delegate); Done. But could you please explain what's the reason for having it here? This base class is empty and therefore I don't see why copying it could be dangerous. https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... File chrome/browser/chromeos/arc/notification/arc_provision_notification_service_unittest.cc (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/notification/arc_provision_notification_service_unittest.cc:72: arc_service_manager_ = base::MakeUnique<ArcServiceManager>(nullptr); On 2017/03/13 05:10:46, hidehiko wrote: > nit: not necessary. Not sure I got it right, but I think ArcServiceManager is needed in this test. First, because the ArcBridgeService instance is obtained from it. Second, even if we created ArcBridgeService here instead, I think that without ArcServiceManager it crashes when attempting to launch the Play Store app (due to some factory returning nullptr in this case).
https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... File chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h:28: }; On 2017/03/16 18:32:12, emaxx wrote: > On 2017/03/13 15:50:41, Luis Héctor Chávez wrote: > > nit: DISALLOW_COPY_AND_ASSIGN(Delegate); > > Done. > But could you please explain what's the reason for having it here? This base > class is empty and therefore I don't see why copying it could be dangerous. Just for consistency with the rest of the code.
Hidehiko, Luis Héctor, PTAL at this review again: there have been no objections on this change from the UI review side. On 2017/03/16 19:28:14, Luis Héctor Chávez wrote: > On 2017/03/16 18:32:12, emaxx wrote: > > On 2017/03/13 15:50:41, Luis Héctor Chávez wrote: > > > nit: DISALLOW_COPY_AND_ASSIGN(Delegate); > > > > Done. > > But could you please explain what's the reason for having it here? This base > > class is empty and therefore I don't see why copying it could be dangerous. > > Just for consistency with the rest of the code. Thanks for the answer!
https://codereview.chromium.org/2745533005/diff/50001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2745533005/diff/50001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:531: // ArcPlayStoreEnabledPreferenceHandler Thanks for fixing the typo in L529, but can you restore the period at the end of this sentence? That was correct :P https://codereview.chromium.org/2745533005/diff/50001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2745533005/diff/50001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:115: virtual void OnArcShowingErrorRequested(ArcSupportHost::Error error) {} nit: "ARC showing error" sounds kinda weird. How about OnArcErrorShowRequested? https://codereview.chromium.org/2745533005/diff/50001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/notification/arc_provision_notification_service.cc (right): https://codereview.chromium.org/2745533005/diff/50001/chrome/browser/chromeos... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.cc:130: // After the error is displayed, there is no reason to keep showing the The error might not be shown. Do you still want to remove the notification? If so, please update the comment so it reflects reality.
The CQ bit was checked by emaxx@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: This issue passed the CQ dry run.
Patchset #5 (id:90001) has been deleted
The CQ bit was checked by emaxx@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:110001) has been deleted
The CQ bit was checked by emaxx@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: This issue passed the CQ dry run.
Incorporated the latest requirements from the UI team - now the notification uses the special icon. https://codereview.chromium.org/2745533005/diff/50001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2745533005/diff/50001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:531: // ArcPlayStoreEnabledPreferenceHandler On 2017/03/24 17:39:16, Luis Héctor Chávez wrote: > Thanks for fixing the typo in L529, but can you restore the period at the end > of this sentence? That was correct :P Oh, that was unintentional change. Thanks! https://codereview.chromium.org/2745533005/diff/50001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2745533005/diff/50001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:115: virtual void OnArcShowingErrorRequested(ArcSupportHost::Error error) {} On 2017/03/24 17:39:16, Luis Héctor Chávez wrote: > nit: "ARC showing error" sounds kinda weird. How about OnArcErrorShowRequested? Done. https://codereview.chromium.org/2745533005/diff/50001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/notification/arc_provision_notification_service.cc (right): https://codereview.chromium.org/2745533005/diff/50001/chrome/browser/chromeos... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.cc:130: // After the error is displayed, there is no reason to keep showing the On 2017/03/24 17:39:16, Luis Héctor Chávez wrote: > The error might not be shown. Do you still want to remove the notification? If > so, please update the comment so it reflects reality. Done, rephrased the comment so that it doesn't talk about "displaying" anymore.
lgtm with one more nit, but please wait for hidehiko@'s sign-off. https://codereview.chromium.org/2745533005/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h (right): https://codereview.chromium.org/2745533005/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h:38: ArcProvisionNotificationService(ArcBridgeService* bridge_service, nit: Rather than having two constructors, how about having a single constructor and then adding a void SetDelegateForTesting(std::unique_ptr<Delegate> delegate); that way you explicitly signal that it's only supposed to be used in unit tests.
LGTM with minor comments. https://codereview.chromium.org/2745533005/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2745533005/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.h:114: // the support app switches to an error page by itself. nit: could you add comment that "this is called even if Support UI is disabled"? https://codereview.chromium.org/2745533005/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h (right): https://codereview.chromium.org/2745533005/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h:37: // Constructs with the supplied delegate. nit: could you insert an empty line here? https://codereview.chromium.org/2745533005/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h:38: ArcProvisionNotificationService(ArcBridgeService* bridge_service, On 2017/04/11 16:02:53, Luis Héctor Chávez wrote: > nit: Rather than having two constructors, how about having a single constructor > and then adding a > > void SetDelegateForTesting(std::unique_ptr<Delegate> delegate); > > that way you explicitly signal that it's only supposed to be used in unit tests. FYI: if creating default Delegate instance is not a good thing, maybe moving this to private, then introducing static std::unique_ptr<ArcProvisionNotificationService> CreateForTesting(...) Then, non-test code will not be able to call it.
The CQ bit was checked by emaxx@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...
emaxx@chromium.org changed reviewers: + oshima@chromium.org
+oshima@: Please review the changes under chrome/app/. Note that the icon and the notification text come from the UI review (linked at the b/ bug). https://codereview.chromium.org/2745533005/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2745533005/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.h:114: // the support app switches to an error page by itself. On 2017/04/12 04:51:34, hidehiko wrote: > nit: could you add comment that "this is called even if Support UI is disabled"? Done. https://codereview.chromium.org/2745533005/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h (right): https://codereview.chromium.org/2745533005/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h:37: // Constructs with the supplied delegate. On 2017/04/12 04:51:34, hidehiko wrote: > nit: could you insert an empty line here? Acknowledged. https://codereview.chromium.org/2745533005/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h:38: ArcProvisionNotificationService(ArcBridgeService* bridge_service, On 2017/04/11 16:02:53, Luis Héctor Chávez wrote: > nit: Rather than having two constructors, how about having a single constructor > and then adding a > > void SetDelegateForTesting(std::unique_ptr<Delegate> delegate); > > that way you explicitly signal that it's only supposed to be used in unit tests. Acknowledged. https://codereview.chromium.org/2745533005/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h:38: ArcProvisionNotificationService(ArcBridgeService* bridge_service, On 2017/04/12 04:51:34, hidehiko wrote: > On 2017/04/11 16:02:53, Luis Héctor Chávez wrote: > > nit: Rather than having two constructors, how about having a single > constructor > > and then adding a > > > > void SetDelegateForTesting(std::unique_ptr<Delegate> delegate); > > > > that way you explicitly signal that it's only supposed to be used in unit > tests. > > FYI: if creating default Delegate instance is not a good thing, maybe moving > this to private, then introducing > > static std::unique_ptr<ArcProvisionNotificationService> CreateForTesting(...) > > Then, non-test code will not be able to call it. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by emaxx@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: This issue passed the CQ dry run.
c/a/t lgtm
The CQ bit was checked by emaxx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2745533005/#ps170001 (title: "Update message")
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": 170001, "attempt_start_ts": 1492158461248340,
"parent_rev": "ef5c7c7902dc7cabb27c1c0149bf725821ce6dce", "commit_rev":
"65e89bdb39e1f22f3032cfafdd3f2da1ba64a9b9"}
Message was sent while issue was closed.
Description was changed from
==========
Show notification during ARC managed provision
Display a notification during ARC provisioning when the opt-in flow
happens silently (without showing the Terms of Service screen)
due to enterprise policies.
The notification is removed either when provisioning finishes
successfully, or when it fails and some error is displayed.
BUG=700609
BUG=b/35865652
TEST=New unit tests;
Manual test: set up enterprise policies ArcEnabled,
ArcBackupRestoreEnabled, ArcLocationServiceEnabled,
log into a fresh profile, check that the notification about
ARC being installed is shown, and after 1-2 minutes the
notification goes away and Play Store is available.
==========
to
==========
Show notification during ARC managed provision
Display a notification during ARC provisioning when the opt-in flow
happens silently (without showing the Terms of Service screen)
due to enterprise policies.
The notification is removed either when provisioning finishes
successfully, or when it fails and some error is displayed.
BUG=700609
BUG=b/35865652
TEST=New unit tests;
Manual test: set up enterprise policies ArcEnabled,
ArcBackupRestoreEnabled, ArcLocationServiceEnabled,
log into a fresh profile, check that the notification about
ARC being installed is shown, and after 1-2 minutes the
notification goes away and Play Store is available.
Review-Url: https://codereview.chromium.org/2745533005
Cr-Commit-Position: refs/heads/master@{#464707}
Committed:
https://chromium.googlesource.com/chromium/src/+/65e89bdb39e1f22f3032cfafdd3f...
==========
Message was sent while issue was closed.
Committed patchset #7 (id:170001) as https://chromium.googlesource.com/chromium/src/+/65e89bdb39e1f22f3032cfafdd3f... |
