Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808353008:
arc: kArcCompatibleFilesystemChosen pref to local state and integer. (Closed)
|
Created:
3 years, 8 months ago by kinaba Modified:
3 years, 7 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, alemate+watch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, achuith+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org, hidehiko Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: kArcCompatibleFilesystemChosen pref to local state and integer.
For bug 709893,
It needs to be established before profile's KeyedService starts working,
hence is moved to the local state which can be initialized earlier.
For bug 711095,
to implement a one-time UI to show the success of navigation to the user,
this value needs to be tri-state (notyet, done, done-and-notified).
Taking this opportunity to move to local state, this CL changes the type
of the pref from boolean to integer as well.
BUG=709893
BUG=711095
TEST=ChromeArcUtilTest
TEST=Manually checked MediaView in Files app is working on the first run.
Review-Url: https://codereview.chromium.org/2808353008
Cr-Commit-Position: refs/heads/master@{#464699}
Committed: https://chromium.googlesource.com/chromium/src/+/6ea2f510147f3bca05aea7024752e95dcf559912
Patch Set 1 #
Total comments: 22
Patch Set 2 : Update reflecting review comments. #
Total comments: 2
Patch Set 3 : DISALLOW_COPY_AND_ASSIGN #
Total comments: 1
Messages
Total messages: 30 (18 generated)
kinaba@chromium.org changed reviewers: + battre@chromium.org, lhchavez@chromium.org, xiyuan@chromium.org
PTAL. xiyuan@: on the whole strategy around UserSessionManager battre@: browser_prefs.cc lhchavez@: as an arc/ OWNER (For the previous version I asked hidehiko@ but he's ooo today and tomorrow...)
The CQ bit was checked by kinaba@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.
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
FYI. (Defer to Luis for owner review, as, yes, I'm ooo) https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util.cc:128: return false; IIUC, this case is not expected even for testing? Then, could you add NOTREACHED()? If it is expected, then could you add VLOG() with comments? https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util.cc:138: filesystem_compatibility >= kFileSystemCompatible; Could you use "==" instead, just in case that a new value is added? Or, could you comment a caution in the enum definition? https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util.h:30: kFileSystemCompatibleAndNotified = 2, This is not yet used. So how about; - Move this definition to the CL which actually needs to use this, or - add comment (TODO) that this will be used just in case. https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util_unittest.cc:94: : test_local_state_(new TestingPrefServiceSimple()) { nit: How about base::MakeUnique?
https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:186: kFileSystemIncompatible); You don't need to do this for using known_user because it hosts the per-user values under a kKnownUsers key and already registered with local state. [1] [1]: https://cs.chromium.org/chromium/src/components/user_manager/known_user.cc?rc... https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util.cc:128: return false; On 2017/04/13 12:38:27, hidehiko wrote: > IIUC, this case is not expected even for testing? > Then, could you add NOTREACHED()? If it is expected, then could you add VLOG() > with comments? Think this should only happen in tests. GetUserByProfile is based on profile path and should work after a user is logged in and we know the user hash. https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:521: if (arc_filesystem_compatibility == arc::kFileSystemIncompatible) { What about old devices that are never going to be compatible? This would make login slow for them no matter what. I wonder whether it makes sense to check IsArcAvailable (or something similar) to limit fs check to only targeted devices.
https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util.h:24: enum FileSystemCompatibilityState { nit: enum FileSystemCompatibilityState : int32_t https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:516: // If the filesystem compatibiliy with ARC is not established yet, test it. nit: s/compatibiliy/compatibility/ https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:517: int arc_filesystem_compatibility = arc::kFileSystemIncompatible; nit: s/int/arc::FileSystemCompatibilityState/ https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:521: if (arc_filesystem_compatibility == arc::kFileSystemIncompatible) { On 2017/04/13 16:35:59, xiyuan wrote: > What about old devices that are never going to be compatible? This would make > login slow for them no matter what. > > I wonder whether it makes sense to check IsArcAvailable (or something similar) > to limit fs check to only targeted devices. When you move the ownership of the pref to arc_util.cc, add the IsArcAvailable check there. https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:1010: user_manager::known_user::SetIntegerPref( Is it possible to centralize the ownership of pref registering/setting/querying to a single file? arc_util.h seems like a good candidate.
The CQ bit was checked by kinaba@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...
Updated. -battre@ from reviewer, browser_pref change turned out unnecessary. (Thanks for your attention!) https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:186: kFileSystemIncompatible); On 2017/04/13 16:35:59, xiyuan wrote: > You don't need to do this for using known_user because it hosts the per-user > values under a kKnownUsers key and already registered with local state. [1] > > [1]: > https://cs.chromium.org/chromium/src/components/user_manager/known_user.cc?rc... Oh, I didn't know the way it works. Thanks, removed. https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util.cc:128: return false; On 2017/04/13 16:35:59, xiyuan wrote: > On 2017/04/13 12:38:27, hidehiko wrote: > > IIUC, this case is not expected even for testing? > > Then, could you add NOTREACHED()? If it is expected, then could you add VLOG() > > with comments? > > Think this should only happen in tests. GetUserByProfile is based on profile > path and should work after a user is logged in and we know the user hash. For instance, the sign-in profile does not have an user associated. Currently this function is not called for such profiles, but its just because IsArcAllowedForProfile() checks chromeos::ProfileHelper::IsPrimaryProfile earlier than checking with this function--and the order is kind of arbitrary. I'd like to keep the check with a comment added. WDYT? https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util.cc:138: filesystem_compatibility >= kFileSystemCompatible; On 2017/04/13 12:38:27, hidehiko wrote: > Could you use "==" instead, just in case that a new value is added? > Or, could you comment a caution in the enum definition? changed to "!= Incompatible" (to align with the usage below, and added a comment to the enum definition. Only a safe choice here is to use switch-case and rely on the static check on exhaustiveness, but to me it sounds like overkill. https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util.h:24: enum FileSystemCompatibilityState { On 2017/04/13 21:11:47, Luis Héctor Chávez wrote: > nit: enum FileSystemCompatibilityState : int32_t Done. https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util.h:30: kFileSystemCompatibleAndNotified = 2, On 2017/04/13 12:38:27, hidehiko wrote: > This is not yet used. So how about; > - Move this definition to the CL which actually needs to use this, or > - add comment (TODO) that this will be used just in case. Added a TODO comment. https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util_unittest.cc:94: : test_local_state_(new TestingPrefServiceSimple()) { On 2017/04/13 12:38:27, hidehiko wrote: > nit: How about base::MakeUnique? Done. https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:516: // If the filesystem compatibiliy with ARC is not established yet, test it. On 2017/04/13 21:11:47, Luis Héctor Chávez wrote: > nit: s/compatibiliy/compatibility/ Acknowledged. https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:517: int arc_filesystem_compatibility = arc::kFileSystemIncompatible; On 2017/04/13 21:11:47, Luis Héctor Chávez wrote: > nit: s/int/arc::FileSystemCompatibilityState/ GetIntegerPref needs to take int* so it cannot be an enum type unfortunately. I factored out the pref reader code to arc_util.cc::GetFileSystemCompatibilityPref to minimize the use of raw int type. https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:521: if (arc_filesystem_compatibility == arc::kFileSystemIncompatible) { On 2017/04/13 21:11:47, Luis Héctor Chávez wrote: > On 2017/04/13 16:35:59, xiyuan wrote: > > What about old devices that are never going to be compatible? This would make > > login slow for them no matter what. > > > > I wonder whether it makes sense to check IsArcAvailable (or something similar) > > to limit fs check to only targeted devices. > > When you move the ownership of the pref to arc_util.cc, add the IsArcAvailable > check there. Thanks for the suggestions. Both done (IsArcAvailable + move to arc_util) https://codereview.chromium.org/2808353008/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:1010: user_manager::known_user::SetIntegerPref( On 2017/04/13 21:11:47, Luis Héctor Chávez wrote: > Is it possible to centralize the ownership of pref registering/setting/querying > to a single file? arc_util.h seems like a good candidate. Done.
Description was changed from ========== arc: kArcCompatibleFilesystemChosen pref to local state and integer. For bug 709893, It needs to be established before profile's KeyedService starts working, hence is moved to the local state which can be initialized earlier. For bug 711095, to implement a one-time UI to show the success of navigation to the user, this value needs to be tri-state (notyet, done, done-and-notified). Taking this opportunity to move to local state, this CL changes the type of the pref from boolean to integer as well. BUG=709893 BUG=711095 TEST=ChromeArcUtilTest ========== to ========== arc: kArcCompatibleFilesystemChosen pref to local state and integer. For bug 709893, It needs to be established before profile's KeyedService starts working, hence is moved to the local state which can be initialized earlier. For bug 711095, to implement a one-time UI to show the success of navigation to the user, this value needs to be tri-state (notyet, done, done-and-notified). Taking this opportunity to move to local state, this CL changes the type of the pref from boolean to integer as well. BUG=709893 BUG=711095 TEST=ChromeArcUtilTest ==========
kinaba@chromium.org changed reviewers: - battre@chromium.org
Description was changed from ========== arc: kArcCompatibleFilesystemChosen pref to local state and integer. For bug 709893, It needs to be established before profile's KeyedService starts working, hence is moved to the local state which can be initialized earlier. For bug 711095, to implement a one-time UI to show the success of navigation to the user, this value needs to be tri-state (notyet, done, done-and-notified). Taking this opportunity to move to local state, this CL changes the type of the pref from boolean to integer as well. BUG=709893 BUG=711095 TEST=ChromeArcUtilTest ========== to ========== arc: kArcCompatibleFilesystemChosen pref to local state and integer. For bug 709893, It needs to be established before profile's KeyedService starts working, hence is moved to the local state which can be initialized earlier. For bug 711095, to implement a one-time UI to show the success of navigation to the user, this value needs to be tri-state (notyet, done, done-and-notified). Taking this opportunity to move to local state, this CL changes the type of the pref from boolean to integer as well. BUG=709893 BUG=711095 TEST=ChromeArcUtilTest TEST=Manually checked MediaView in Files app is working on the first run. ==========
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2808353008/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2808353008/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util_unittest.cc:104: }; nit: DISALLOW_COPY_AND_ASSIGN(...)
Thanks! https://codereview.chromium.org/2808353008/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2808353008/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util_unittest.cc:104: }; On 2017/04/14 06:51:31, xiyuan wrote: > nit: DISALLOW_COPY_AND_ASSIGN(...) Done.
The CQ bit was checked by kinaba@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2808353008/#ps40001 (title: "DISALLOW_COPY_AND_ASSIGN")
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": 40001, "attempt_start_ts": 1492153039480790,
"parent_rev": "0f65d25a4097959d977dbb1077717323438240a6", "commit_rev":
"6ea2f510147f3bca05aea7024752e95dcf559912"}
Message was sent while issue was closed.
Description was changed from ========== arc: kArcCompatibleFilesystemChosen pref to local state and integer. For bug 709893, It needs to be established before profile's KeyedService starts working, hence is moved to the local state which can be initialized earlier. For bug 711095, to implement a one-time UI to show the success of navigation to the user, this value needs to be tri-state (notyet, done, done-and-notified). Taking this opportunity to move to local state, this CL changes the type of the pref from boolean to integer as well. BUG=709893 BUG=711095 TEST=ChromeArcUtilTest TEST=Manually checked MediaView in Files app is working on the first run. ========== to ========== arc: kArcCompatibleFilesystemChosen pref to local state and integer. For bug 709893, It needs to be established before profile's KeyedService starts working, hence is moved to the local state which can be initialized earlier. For bug 711095, to implement a one-time UI to show the success of navigation to the user, this value needs to be tri-state (notyet, done, done-and-notified). Taking this opportunity to move to local state, this CL changes the type of the pref from boolean to integer as well. BUG=709893 BUG=711095 TEST=ChromeArcUtilTest TEST=Manually checked MediaView in Files app is working on the first run. Review-Url: https://codereview.chromium.org/2808353008 Cr-Commit-Position: refs/heads/master@{#464699} Committed: https://chromium.googlesource.com/chromium/src/+/6ea2f510147f3bca05aea7024752... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6ea2f510147f3bca05aea7024752...
Message was sent while issue was closed.
uekawa@chromium.org changed reviewers: + uekawa@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2808353008/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2808353008/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.h:31: kFileSystemIncompatible = 0, what about the case that the ext4 crypto is already in use when user has created the directory in cryptohome? New users always get ext4 crypto file system on dircrypto capable systems now. They get notifications ?
Message was sent while issue was closed.
On 2017/05/11 22:15:34, Junichi Uekawa wrote: > https://codereview.chromium.org/2808353008/diff/40001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_util.h (right): > > https://codereview.chromium.org/2808353008/diff/40001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_util.h:31: kFileSystemIncompatible = 0, > what about the case that the ext4 crypto is already in use when user has created > the directory in cryptohome? > > New users always get ext4 crypto file system on dircrypto capable systems now. > > They get notifications ? No |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
