arc: Show migration success notification on the next sign-in after migration.

arc: Show migration success notification on the next sign-in after migration.

Referring the tri-state preference, shows the notification when needed.
The special case is when the profile was newly created on the device.
In that case, the profile is on the compatible filesystem from the beginning,
we skip showing the migration and mark the pref as if it has been shown.

BUG=711095
TEST=Manually tried new/migrated/skipped-migration/guest/supervised accounts.

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

[kinaba, 2017-04-20 07:03:52]

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

[commit-bot: I haz the power, 2017-04-20 07:04:10]

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

[kinaba, 2017-04-20 07:41:09]

Description was changed from

==========
WIP

BUG=
==========

to

==========
arc: Show migration success notification on the next sign-in after migration.

Referring the tri-state preference, shows the notification when needed.
The special case is when the profile was newly created on the device.
In that case, the profile is on the compatible filesystem from the beginning,
we skip showing the migration and mark the pref as if it has been shown.

BUG=711095
TEST=Manually tried new/migrated/skipped-migration/guest/supervised accounts.
==========

[kinaba, 2017-04-20 07:55:10]

kinaba@chromium.org changed reviewers:
+ hidehiko@chromium.org, xiyuan@chromium.org

[kinaba, 2017-04-20 07:55:10]

PTAL: hidehiko (on arc/), xiyuan (on UserSessionManager)

The icon resource addition is reviewed separately at:
https://codereview.chromium.org/2820433002/
Once the CL landed I'll remove the part from this CL.

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

https://codereview.chromium.org/2828213002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_migration_guide_notification.cc:17: #include
"components/user_manager/known_user.h"
Another implementation option is to factor out the logic for
checking/setting the preference to arc_util, to something like

bool CheckAndUpdateIfMigrationSuccessNotificationNeeded(Profile*);

and turn the method below as
if(CheckAndUpdateIfMigrationSuccessNotificationNeeded()) show();

That's more modular and testable but I'm not so sure if adding
such a method very specific to this feature in arc_util.h is ok.

I'd like to ask the owner's opinion. hidehiko@, WDYT?

[commit-bot: I haz the power, 2017-04-20 08:10:12]

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

[commit-bot: I haz the power, 2017-04-20 08:10:12]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-04-20 15:32:19]

lgtm

[kinaba, 2017-04-21 03:54:19]

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

[commit-bot: I haz the power, 2017-04-21 03:54:49]

Dry run: 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-04-21 04:22:55]

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

[commit-bot: I haz the power, 2017-04-21 04:22:57]

Dry run: This issue passed the CQ dry run.

[kinaba, 2017-04-24 00:49:32]

friendly ping: hidehiko@

[hidehiko, 2017-04-24 05:59:48]

Sorry for delay. LGTM with minor comments.

https://codereview.chromium.org/2828213002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_migration_guide_notification.cc (right):

https://codereview.chromium.org/2828213002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_migration_guide_notification.cc:79: const
AccountId account_id(multi_user_util::GetAccountIdFromProfile(profile));
totally nit/optional: Often, in Chrome, = is used for copy-ctor?

https://codereview.chromium.org/2828213002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2828213002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1834: // Shows and
updates the relevant pref for the one-time notification on the
nit: s/Shows and updates/Show and update/ (please use imperative form, in inline
comments).
Or, maybe "Show the one-time notification and update the relavant pref ..."?

[kinaba, 2017-04-24 06:09:34]

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

[kinaba, 2017-04-24 06:09:37]

thanks!

https://codereview.chromium.org/2828213002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_migration_guide_notification.cc (right):

https://codereview.chromium.org/2828213002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_migration_guide_notification.cc:79: const
AccountId account_id(multi_user_util::GetAccountIdFromProfile(profile));
On 2017/04/24 05:59:48, hidehiko wrote:
> totally nit/optional: Often, in Chrome, = is used for copy-ctor?

Done.

https://codereview.chromium.org/2828213002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2828213002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1834: // Shows and
updates the relevant pref for the one-time notification on the
On 2017/04/24 05:59:48, hidehiko wrote:
> nit: s/Shows and updates/Show and update/ (please use imperative form, in
inline
> comments).
> Or, maybe "Show the one-time notification and update the relavant pref ..."?

Done.

[commit-bot: I haz the power, 2017-04-24 06:09:49]

Dry run: 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-04-24 06:34:14]

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

[commit-bot: I haz the power, 2017-04-24 06:34:16]

Dry run: This issue passed the CQ dry run.

[kinaba, 2017-04-24 06:34:46]

The CQ bit was checked by kinaba@chromium.org

[kinaba, 2017-04-24 06:34:46]

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

[commit-bot: I haz the power, 2017-04-24 06:34:58]

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-04-24 06:37:49]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493015686106840,
"parent_rev": "298f67afd032e7251a2b950715c456769d4977f9", "commit_rev":
"c6d7406702779c75f188e7380dd71f211ed4e765"}

[commit-bot: I haz the power, 2017-04-24 06:37:59]

Description was changed from

==========
arc: Show migration success notification on the next sign-in after migration.

Referring the tri-state preference, shows the notification when needed.
The special case is when the profile was newly created on the device.
In that case, the profile is on the compatible filesystem from the beginning,
we skip showing the migration and mark the pref as if it has been shown.

BUG=711095
TEST=Manually tried new/migrated/skipped-migration/guest/supervised accounts.
==========

to

==========
arc: Show migration success notification on the next sign-in after migration.

Referring the tri-state preference, shows the notification when needed.
The special case is when the profile was newly created on the device.
In that case, the profile is on the compatible filesystem from the beginning,
we skip showing the migration and mark the pref as if it has been shown.

BUG=711095
TEST=Manually tried new/migrated/skipped-migration/guest/supervised accounts.

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

[commit-bot: I haz the power, 2017-04-24 06:38:00]

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