Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2849333002:
cros: Skip need-dircrypto-migration check if not needed (Closed)
Descriptioncros: Skip need-dircrypto-migration check if not needed
Skip need-dircrypto-migration check if ARC is not available
or migration UI is disabled.
BUG=712580
Review-Url: https://codereview.chromium.org/2849333002
Cr-Commit-Position: refs/heads/master@{#468619}
Committed: https://chromium.googlesource.com/chromium/src/+/494548aec6a17848303f5502e2eddcc7f0f0b10b
Patch Set 1 #
Total comments: 4
Patch Set 2 : update test #
Messages
Total messages: 22 (12 generated)
xiyuan@chromium.org changed reviewers: + fukino@chromium.org
Could help to review this? Thanks.
The CQ bit was checked by xiyuan@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thank you! This change lgtm, but it seems UserSelectionScreenTest.ShowDircryptoMigrationBanner needs to be updated to show the banner?
uekawa@chromium.org changed reviewers: + hidehiko@chromium.org, uekawa@chromium.org
+hidehiko FYI I guess we missed this case where device is capable of direncryption but we do not enable it yet. https://codereview.chromium.org/2849333002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right): https://codereview.chromium.org/2849333002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/screens/user_selection_screen.cc:556: dircrypto_migration_checker_->Check(account_id); From what I can see from src/chrome/browser/chromeos/login/screens/user_selection_screen.cc DircryptoMigrationChecker::Check sends a request to cryptohome, where cryptohome already should do a check .... but from what I read in https://chromium-review.googlesource.com/#/c/479475/ I guess it doesn't have a concept of experiement or Arc enablement check.
Thanks fukino@. Will fix the test in the next CL. https://codereview.chromium.org/2849333002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right): https://codereview.chromium.org/2849333002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/screens/user_selection_screen.cc:556: dircrypto_migration_checker_->Check(account_id); On 2017/05/02 05:02:24, Junichi Uekawa wrote: > > From what I can see from > src/chrome/browser/chromeos/login/screens/user_selection_screen.cc > DircryptoMigrationChecker::Check > sends a request to cryptohome, where cryptohome already should do a check .... > > but from what I read in > https://chromium-review.googlesource.com/#/c/479475/ > I guess it doesn't have a concept of experiement or Arc enablement check. Correct. The CL is needed because cryptohomed does not have knowledge of ARC. It only knows whether the user is on eCryptFS and whether dircrypto is supported. I missed this because I assumed all dircrypto enabled device would have ARC.
The CQ bit was checked by xiyuan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org Link to the patchset: https://codereview.chromium.org/2849333002/#ps20001 (title: "update test")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
FYI https://codereview.chromium.org/2849333002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right): https://codereview.chromium.org/2849333002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/screens/user_selection_screen.cc:149: arc::IsArcAvailable(); Just clarification: Is it ok to pass this flow in the situation where; - ARC++ is not officially supported, but - ARC++ KIOSK is supported. This is just a notification, IIUC, so maybe ok to keep as is, but just in case.
The CQ bit was unchecked by xiyuan@chromium.org
https://codereview.chromium.org/2849333002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right): https://codereview.chromium.org/2849333002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/screens/user_selection_screen.cc:149: arc::IsArcAvailable(); On 2017/05/02 05:58:57, hidehiko wrote: > Just clarification: > Is it ok to pass this flow in the situation where; > - ARC++ is not officially supported, but > - ARC++ KIOSK is supported. > > This is just a notification, IIUC, so maybe ok to keep as is, but just in case. I skipped the ARC++ KIOSK test because the banner is meant for a real user. So for such devices, a user would not get ARC so the banner should not show up either. Please confirm whether this is the desired behavior. I will hold off committing for now. Thanks.
On 2017/05/02 06:06:05, xiyuan wrote: > https://codereview.chromium.org/2849333002/diff/1/chrome/browser/chromeos/log... > File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right): > > https://codereview.chromium.org/2849333002/diff/1/chrome/browser/chromeos/log... > chrome/browser/chromeos/login/screens/user_selection_screen.cc:149: > arc::IsArcAvailable(); > On 2017/05/02 05:58:57, hidehiko wrote: > > Just clarification: > > Is it ok to pass this flow in the situation where; > > - ARC++ is not officially supported, but > > - ARC++ KIOSK is supported. > > > > This is just a notification, IIUC, so maybe ok to keep as is, but just in > case. > > I skipped the ARC++ KIOSK test because the banner is meant for a real user. So > for such devices, a user would not get ARC so the banner should not show up > either. > > Please confirm whether this is the desired behavior. I will hold off committing > for now. > Thanks. lgtm
The CQ bit was checked by xiyuan@chromium.org
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": 20001, "attempt_start_ts": 1493732165493320,
"parent_rev": "e27f9842f0a01769ba463a45174390bc70807139", "commit_rev":
"494548aec6a17848303f5502e2eddcc7f0f0b10b"}
Message was sent while issue was closed.
Description was changed from ========== cros: Skip need-dircrypto-migration check if not needed Skip need-dircrypto-migration check if ARC is not available or migration UI is disabled. BUG=712580 ========== to ========== cros: Skip need-dircrypto-migration check if not needed Skip need-dircrypto-migration check if ARC is not available or migration UI is disabled. BUG=712580 Review-Url: https://codereview.chromium.org/2849333002 Cr-Commit-Position: refs/heads/master@{#468619} Committed: https://chromium.googlesource.com/chromium/src/+/494548aec6a17848303f5502e2ed... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/494548aec6a17848303f5502e2ed... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
