Chromium Code Reviews
Help | Chromium Project | Sign in
(4)

Issue 2745533005: Show notification during ARC managed provision

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 3 days ago by emaxx
Modified:
18 hours, 56 minutes 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.

Description

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.

Patch Set 1 #

Patch Set 2 : Add tests, clean up #

Total comments: 16

Patch Set 3 : Renaming, nit fixes #

Total comments: 3

Patch Set 4 : Rebase #

Patch Set 5 : Fix nits, change the notification icon #

Unified diffs Side-by-side diffs Delta from patch set Stats (+543 lines, -18 lines) Patch
M chrome/app/chromeos_strings.grdp View 1 2 3 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/BUILD.gn View 1 2 3 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_service_launcher.cc View 1 2 3 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_session_manager.h View 1 2 3 4 3 chunks +14 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_session_manager.cc View 1 2 3 4 8 chunks +22 lines, -18 lines 0 comments Download
A chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h View 1 2 3 4 1 chunk +59 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/arc/notification/arc_provision_notification_service.cc View 1 2 3 4 1 chunk +137 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/arc/notification/arc_provision_notification_service_unittest.cc View 1 2 3 4 1 chunk +299 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 33 (26 generated)
emaxx
Hidehiko, PTAL.
2 weeks, 2 days ago (2017-03-11 02:37:35 UTC) #14
hidehiko
https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos/arc/arc_session_manager.cc File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos/arc/arc_session_manager.cc#newcode561 chrome/browser/chromeos/arc/arc_session_manager.cc:561: // TODO(hidehiko): DCHECK the previous state. This is called ...
2 weeks ago (2017-03-13 05:10:47 UTC) #17
Luis Héctor Chávez
drive-by https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos/arc/arc_session_manager.h File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos/arc/arc_session_manager.h#newcode99 chrome/browser/chromeos/arc/arc_session_manager.h:99: virtual void OnArcStartOptInAndroidManagementCheck() {} nit: Events should be ...
2 weeks ago (2017-03-13 15:50:41 UTC) #19
emaxx
Updated the CL, thanks for the comments Hidehiko and Luis Héctor. Note that I'll have ...
1 week, 4 days ago (2017-03-16 18:32:12 UTC) #26
Luis Héctor Chávez
https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h File chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h (right): https://codereview.chromium.org/2745533005/diff/10008/chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h#newcode28 chrome/browser/chromeos/arc/notification/arc_provision_notification_service.h:28: }; On 2017/03/16 18:32:12, emaxx wrote: > On 2017/03/13 ...
1 week, 4 days ago (2017-03-16 19:28:14 UTC) #27
emaxx
Hidehiko, Luis Héctor, PTAL at this review again: there have been no objections on this ...
3 days, 10 hours ago (2017-03-24 14:13:15 UTC) #28
Luis Héctor Chávez
3 days, 6 hours ago (2017-03-24 17:39:16 UTC) #29
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46