Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
789853002:
Add support for child accounts on Android. (Closed)
|
Created:
6 years ago by Bernhard Bauer Modified:
6 years ago CC:
chromium-reviews, rginda+watch_chromium.org, pam+watch_chromium.org, aberent, khannans Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd support for child accounts on Android.
BUG=440405
Committed: https://crrev.com/4b5f497d0aeeb21877400bd46a2e7cfdbc98214d
Cr-Commit-Position: refs/heads/master@{#308117}
Patch Set 1 #Patch Set 2 : sync #
Total comments: 42
Patch Set 3 : review #Patch Set 4 : review #Patch Set 5 : x #Messages
Total messages: 15 (3 generated)
bauerb@chromium.org changed reviewers: + treib@chromium.org
Please review.
https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java (right): https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:48: private static ChildAccountService sChildAccountManager; nit: rename to sChildAccountService https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:52: // "Boolean" rather than "boolean" to force a crash if hasChildAccount is called too soon So essentially a three-way bool? http://thedailywtf.com/articles/What_Is_Truth_0x3f_ Not an actual complaint :D The comment is a bit inaccurate though, as "null" is really just used to mean "don't know yet". https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:65: * Returns the shared ChildAccountManager instance, creating one if necessary. ChildAccountService https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:103: if (googleAccounts.length != 1) { Could this condition potentially change between calls? Seems like that might break things. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:110: if (forceChildAccount(account)) { Should the debug flag work even if there's more than one account on the device? https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:132: boolean hasChildAccount = hasChildAccount(); Should you set mHasChildAccount here? https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:146: private boolean forceChildAccount(Account account) { Rename to shouldForceChildAccount? https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:148: ChromeSwitches.CHILD_ACCOUNT); nit: Indentation? https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:175: mHasChildAccount = getFutureResult(); So this will block on the result, but only if checkHasChildAccount has been called before? Because otherwise mAccountManagerFuture will be null and so getFutureResult will crash. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:184: public void waitUntilFinished() { What does this method do, since it doesn't actually block on the result? It also doesn't seem to be used anywhere? https://codereview.chromium.org/789853002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_service.h (right): https://codereview.chromium.org/789853002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_service.h:38: void SetIsChildAccount(bool is_child_account); Please add a comment explaining why this is public. https://codereview.chromium.org/789853002/diff/20001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/789853002/diff/20001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:681: 'browser/supervised_user/child_accounts/child_account_service_android.cc', Should these go into an "android && enable_supervised_users" section?
https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java (right): https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:48: private static ChildAccountService sChildAccountManager; On 2014/12/10 11:53:27, Marc Treib wrote: > nit: rename to sChildAccountService Done. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:52: // "Boolean" rather than "boolean" to force a crash if hasChildAccount is called too soon On 2014/12/10 11:53:27, Marc Treib wrote: > So essentially a three-way bool? > http://thedailywtf.com/articles/What_Is_Truth_0x3f_ Well, it would be an optional boolean, if Java had a proper type system... ;-) > Not an actual complaint :D > The comment is a bit inaccurate though, as "null" is really just used to mean > "don't know yet". Yes, but in addition, it's not allowed to call e.g. hasChildAccount() if we don't know yet. This field being null will automatically enforce that by crashing in that case :) https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:65: * Returns the shared ChildAccountManager instance, creating one if necessary. On 2014/12/10 11:53:27, Marc Treib wrote: > ChildAccountService Done. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:103: if (googleAccounts.length != 1) { On 2014/12/10 11:53:27, Marc Treib wrote: > Could this condition potentially change between calls? Seems like that might > break things. Once full Unicorn support is in GMSCore etc., GMSCore will enforce the invariant that if there is a Unicorn account in a profile, there is no other Google account in that profile (i.e. you will either have exactly one Unicorn account and nothing else, or any number of non-Unicorn Google accounts, and no accounts can be added or removed. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:110: if (forceChildAccount(account)) { On 2014/12/10 11:53:27, Marc Treib wrote: > Should the debug flag work even if there's more than one account on the device? Like I mentioned above, right now I don't really care about the case where there's more than one account, as that's not supported anyway. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:132: boolean hasChildAccount = hasChildAccount(); On 2014/12/10 11:53:27, Marc Treib wrote: > Should you set mHasChildAccount here? hasChildAccount() will do that. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:146: private boolean forceChildAccount(Account account) { On 2014/12/10 11:53:27, Marc Treib wrote: > Rename to shouldForceChildAccount? Done. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:148: ChromeSwitches.CHILD_ACCOUNT); On 2014/12/10 11:53:27, Marc Treib wrote: > nit: Indentation? 8 space indent for continued lines is actually correct; Android Java code is weird like that. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:175: mHasChildAccount = getFutureResult(); On 2014/12/10 11:53:27, Marc Treib wrote: > So this will block on the result, but only if checkHasChildAccount has been > called before? Because otherwise mAccountManagerFuture will be null and so > getFutureResult will crash. Correct! :) The idea here is that you call checkHasChildAccount() early on, then either wait to get called back, or synchronously wait. It's not the most convenient to use, but it lets us fire the initial check off early during startup, and only wait if necessary. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:184: public void waitUntilFinished() { On 2014/12/10 11:53:27, Marc Treib wrote: > What does this method do, since it doesn't actually block on the result? It also > doesn't seem to be used anywhere? getFutureResult() will block. I updated the comment to explain that. And this is called downstream (like checkHasChildAccount). https://codereview.chromium.org/789853002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_service.h (right): https://codereview.chromium.org/789853002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_service.h:38: void SetIsChildAccount(bool is_child_account); On 2014/12/10 11:53:27, Marc Treib wrote: > Please add a comment explaining why this is public. Done. https://codereview.chromium.org/789853002/diff/20001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/789853002/diff/20001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:681: 'browser/supervised_user/child_accounts/child_account_service_android.cc', On 2014/12/10 11:53:27, Marc Treib wrote: > Should these go into an "android && enable_supervised_users" section? Hm... Now that you mention it and looking at other files in here, it seems that _android.* files are automatically excluded on other platforms, so I can probably just put this into the chrome_browser_supervised_user_sources section.
https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java (right): https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:52: // "Boolean" rather than "boolean" to force a crash if hasChildAccount is called too soon On 2014/12/11 09:07:58, Bernhard Bauer wrote: > On 2014/12/10 11:53:27, Marc Treib wrote: > > So essentially a three-way bool? > > http://thedailywtf.com/articles/What_Is_Truth_0x3f_ > > Well, it would be an optional boolean, if Java had a proper type system... ;-) > > > Not an actual complaint :D > > The comment is a bit inaccurate though, as "null" is really just used to mean > > "don't know yet". > > Yes, but in addition, it's not allowed to call e.g. hasChildAccount() if we > don't know yet. This field being null will automatically enforce that by > crashing in that case :) I'd still argue that the "optional" part is what you actually want, while the crashing is only a (desirable) side-effect, and so the comment should reflect that. I'd put the "crash if not known yet" comment down into hasChildAccount. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:103: if (googleAccounts.length != 1) { On 2014/12/11 09:07:58, Bernhard Bauer wrote: > On 2014/12/10 11:53:27, Marc Treib wrote: > > Could this condition potentially change between calls? Seems like that might > > break things. > > Once full Unicorn support is in GMSCore etc., GMSCore will enforce the invariant > that if there is a Unicorn account in a profile, there is no other Google > account in that profile (i.e. you will either have exactly one Unicorn account > and nothing else, or any number of non-Unicorn Google accounts, and no accounts > can be added or removed. Acknowledged. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:110: if (forceChildAccount(account)) { On 2014/12/11 09:07:58, Bernhard Bauer wrote: > On 2014/12/10 11:53:27, Marc Treib wrote: > > Should the debug flag work even if there's more than one account on the > device? > > Like I mentioned above, right now I don't really care about the case where > there's more than one account, as that's not supported anyway. This is just a debug flag anyway, right? I was just saying that it might be convenient if that worked even if I happen to have more than one account on the device. But it's up to you. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:132: boolean hasChildAccount = hasChildAccount(); On 2014/12/11 09:07:58, Bernhard Bauer wrote: > On 2014/12/10 11:53:27, Marc Treib wrote: > > Should you set mHasChildAccount here? > > hasChildAccount() will do that. Well, if you do it here (and in waitUntilFinished), then hasChildAccount won't have to. Right now, a valid known state can live either in mHasChildAccount or in the future, and it think that's a bit confusing, so my vote is for populating mHasChildAccount as soon as we know the value. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:148: ChromeSwitches.CHILD_ACCOUNT); On 2014/12/11 09:07:58, Bernhard Bauer wrote: > On 2014/12/10 11:53:27, Marc Treib wrote: > > nit: Indentation? > > 8 space indent for continued lines is actually correct; Android Java code is > weird like that. Acknowledged. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:175: mHasChildAccount = getFutureResult(); On 2014/12/11 09:07:58, Bernhard Bauer wrote: > On 2014/12/10 11:53:27, Marc Treib wrote: > > So this will block on the result, but only if checkHasChildAccount has been > > called before? Because otherwise mAccountManagerFuture will be null and so > > getFutureResult will crash. > > Correct! :) The idea here is that you call checkHasChildAccount() early on, then > either wait to get called back, or synchronously wait. It's not the most > convenient to use, but it lets us fire the initial check off early during > startup, and only wait if necessary. Hm. I guess I'd prefer to fire off the check here if it's not running already, but okay. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:184: public void waitUntilFinished() { On 2014/12/11 09:07:58, Bernhard Bauer wrote: > On 2014/12/10 11:53:27, Marc Treib wrote: > > What does this method do, since it doesn't actually block on the result? It > also > > doesn't seem to be used anywhere? > > getFutureResult() will block. I updated the comment to explain that. > > And this is called downstream (like checkHasChildAccount). Ah, I missed the ".get()" at the end. So blocking on the future is only allowed in a background thread, but blocking on a random AsyncTask is fine?! Oh well... https://codereview.chromium.org/789853002/diff/20001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/789853002/diff/20001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:681: 'browser/supervised_user/child_accounts/child_account_service_android.cc', On 2014/12/11 09:07:58, Bernhard Bauer wrote: > On 2014/12/10 11:53:27, Marc Treib wrote: > > Should these go into an "android && enable_supervised_users" section? > > Hm... Now that you mention it and looking at other files in here, it seems that > _android.* files are automatically excluded on other platforms, so I can > probably just put this into the chrome_browser_supervised_user_sources section. Ah.. a bit magic, but convenient.
https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java (right): https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:52: // "Boolean" rather than "boolean" to force a crash if hasChildAccount is called too soon On 2014/12/11 12:21:31, Marc Treib wrote: > On 2014/12/11 09:07:58, Bernhard Bauer wrote: > > On 2014/12/10 11:53:27, Marc Treib wrote: > > > So essentially a three-way bool? > > > http://thedailywtf.com/articles/What_Is_Truth_0x3f_ > > > > Well, it would be an optional boolean, if Java had a proper type system... ;-) > > > > > Not an actual complaint :D > > > The comment is a bit inaccurate though, as "null" is really just used to > mean > > > "don't know yet". > > > > Yes, but in addition, it's not allowed to call e.g. hasChildAccount() if we > > don't know yet. This field being null will automatically enforce that by > > crashing in that case :) > > I'd still argue that the "optional" part is what you actually want, while the > crashing is only a (desirable) side-effect, and so the comment should reflect > that. I'd put the "crash if not known yet" comment down into hasChildAccount. Updated the comment. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:110: if (forceChildAccount(account)) { On 2014/12/11 12:21:31, Marc Treib wrote: > On 2014/12/11 09:07:58, Bernhard Bauer wrote: > > On 2014/12/10 11:53:27, Marc Treib wrote: > > > Should the debug flag work even if there's more than one account on the > > device? > > > > Like I mentioned above, right now I don't really care about the case where > > there's more than one account, as that's not supported anyway. > > This is just a debug flag anyway, right? I was just saying that it might be > convenient if that worked even if I happen to have more than one account on the > device. But it's up to you. Hm. There is some other code in Clank that relies on the fact that we only do automatic signin if there's a single account on the device (even for the non-child account case), and that would break. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:132: boolean hasChildAccount = hasChildAccount(); On 2014/12/11 12:21:31, Marc Treib wrote: > On 2014/12/11 09:07:58, Bernhard Bauer wrote: > > On 2014/12/10 11:53:27, Marc Treib wrote: > > > Should you set mHasChildAccount here? > > > > hasChildAccount() will do that. > > Well, if you do it here (and in waitUntilFinished), then hasChildAccount won't > have to. > Right now, a valid known state can live either in mHasChildAccount or in the > future, and it think that's a bit confusing, so my vote is for populating > mHasChildAccount as soon as we know the value. The problem here is that this callback will be posted to the main run loop, so in the synchronous case it will actually run after waitUntilFinished has been called. If we assign mHasChildAccount here and in waitUntilFinished, we won't actually set it any earlier, we'll just have two places where we set it. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:175: mHasChildAccount = getFutureResult(); On 2014/12/11 12:21:31, Marc Treib wrote: > On 2014/12/11 09:07:58, Bernhard Bauer wrote: > > On 2014/12/10 11:53:27, Marc Treib wrote: > > > So this will block on the result, but only if checkHasChildAccount has been > > > called before? Because otherwise mAccountManagerFuture will be null and so > > > getFutureResult will crash. > > > > Correct! :) The idea here is that you call checkHasChildAccount() early on, > then > > either wait to get called back, or synchronously wait. It's not the most > > convenient to use, but it lets us fire the initial check off early during > > startup, and only wait if necessary. > > Hm. I guess I'd prefer to fire off the check here if it's not running already, > but okay. Yeah, the reason I don't do that is because if someone forgets calling checkHasChildAccount() beforehand, that would increase startup time during synchronous startup, but otherwise succeed, so no one would notice. As it is, forgetting checkHasChildAccount() will actually result in a crash. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:184: public void waitUntilFinished() { On 2014/12/11 12:21:31, Marc Treib wrote: > On 2014/12/11 09:07:58, Bernhard Bauer wrote: > > On 2014/12/10 11:53:27, Marc Treib wrote: > > > What does this method do, since it doesn't actually block on the result? It > > also > > > doesn't seem to be used anywhere? > > > > getFutureResult() will block. I updated the comment to explain that. > > > > And this is called downstream (like checkHasChildAccount). > > Ah, I missed the ".get()" at the end. So blocking on the future is only allowed > in a background thread, but blocking on a random AsyncTask is fine?! Oh well... Welcome to Android :) I do feel somewhat bad for sidestepping the restriction that waiting on an AccountManager future on the main thread is not allowed, but then again when we call this we are already waiting on the main thread for the native library to be loaded.
LGTM with one optional nit below. Thanks for your patience in explaining stuff, and sorry for the slow roundtrip times today, which were caused by me being at interview training. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java (right): https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:110: if (forceChildAccount(account)) { On 2014/12/11 13:38:59, Bernhard Bauer wrote: > On 2014/12/11 12:21:31, Marc Treib wrote: > > On 2014/12/11 09:07:58, Bernhard Bauer wrote: > > > On 2014/12/10 11:53:27, Marc Treib wrote: > > > > Should the debug flag work even if there's more than one account on the > > > device? > > > > > > Like I mentioned above, right now I don't really care about the case where > > > there's more than one account, as that's not supported anyway. > > > > This is just a debug flag anyway, right? I was just saying that it might be > > convenient if that worked even if I happen to have more than one account on > the > > device. But it's up to you. > > Hm. There is some other code in Clank that relies on the fact that we only do > automatic signin if there's a single account on the device (even for the > non-child account case), and that would break. Acknowledged. It's not worth spending extra effort to make it work. Optionally, you could add a log message saying something like "hey, fyi, you specified this debug flag but it won't do anything". https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:132: boolean hasChildAccount = hasChildAccount(); On 2014/12/11 13:38:59, Bernhard Bauer wrote: > On 2014/12/11 12:21:31, Marc Treib wrote: > > On 2014/12/11 09:07:58, Bernhard Bauer wrote: > > > On 2014/12/10 11:53:27, Marc Treib wrote: > > > > Should you set mHasChildAccount here? > > > > > > hasChildAccount() will do that. > > > > Well, if you do it here (and in waitUntilFinished), then hasChildAccount won't > > have to. > > Right now, a valid known state can live either in mHasChildAccount or in the > > future, and it think that's a bit confusing, so my vote is for populating > > mHasChildAccount as soon as we know the value. > > The problem here is that this callback will be posted to the main run loop, so > in the synchronous case it will actually run after waitUntilFinished has been > called. If we assign mHasChildAccount here and in waitUntilFinished, we won't > actually set it any earlier, we'll just have two places where we set it. I'm not worried about setting it at the earliest possible moment, but rather there being just one place we have to check. But it sounds like that won't work anyway :( https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:175: mHasChildAccount = getFutureResult(); On 2014/12/11 13:38:59, Bernhard Bauer wrote: > On 2014/12/11 12:21:31, Marc Treib wrote: > > On 2014/12/11 09:07:58, Bernhard Bauer wrote: > > > On 2014/12/10 11:53:27, Marc Treib wrote: > > > > So this will block on the result, but only if checkHasChildAccount has > been > > > > called before? Because otherwise mAccountManagerFuture will be null and so > > > > getFutureResult will crash. > > > > > > Correct! :) The idea here is that you call checkHasChildAccount() early on, > > then > > > either wait to get called back, or synchronously wait. It's not the most > > > convenient to use, but it lets us fire the initial check off early during > > > startup, and only wait if necessary. > > > > Hm. I guess I'd prefer to fire off the check here if it's not running already, > > but okay. > > Yeah, the reason I don't do that is because if someone forgets calling > checkHasChildAccount() beforehand, that would increase startup time during > synchronous startup, but otherwise succeed, so no one would notice. As it is, > forgetting checkHasChildAccount() will actually result in a crash. Acknowledged. https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:184: public void waitUntilFinished() { On 2014/12/11 13:38:59, Bernhard Bauer wrote: > On 2014/12/11 12:21:31, Marc Treib wrote: > > On 2014/12/11 09:07:58, Bernhard Bauer wrote: > > > On 2014/12/10 11:53:27, Marc Treib wrote: > > > > What does this method do, since it doesn't actually block on the result? > It > > > also > > > > doesn't seem to be used anywhere? > > > > > > getFutureResult() will block. I updated the comment to explain that. > > > > > > And this is called downstream (like checkHasChildAccount). > > > > Ah, I missed the ".get()" at the end. So blocking on the future is only > allowed > > in a background thread, but blocking on a random AsyncTask is fine?! Oh > well... > > Welcome to Android :) > > I do feel somewhat bad for sidestepping the restriction that waiting on an > AccountManager future on the main thread is not allowed, but then again when we > call this we are already waiting on the main thread for the native library to be > loaded. Acknowledged.
https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java (right): https://codereview.chromium.org/789853002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ChildAccountService.java:110: if (forceChildAccount(account)) { On 2014/12/11 15:04:31, Marc Treib wrote: > On 2014/12/11 13:38:59, Bernhard Bauer wrote: > > On 2014/12/11 12:21:31, Marc Treib wrote: > > > On 2014/12/11 09:07:58, Bernhard Bauer wrote: > > > > On 2014/12/10 11:53:27, Marc Treib wrote: > > > > > Should the debug flag work even if there's more than one account on the > > > > device? > > > > > > > > Like I mentioned above, right now I don't really care about the case where > > > > there's more than one account, as that's not supported anyway. > > > > > > This is just a debug flag anyway, right? I was just saying that it might be > > > convenient if that worked even if I happen to have more than one account on > > the > > > device. But it's up to you. > > > > Hm. There is some other code in Clank that relies on the fact that we only do > > automatic signin if there's a single account on the device (even for the > > non-child account case), and that would break. > > Acknowledged. > It's not worth spending extra effort to make it work. Optionally, you could add > a log message saying something like "hey, fyi, you specified this debug flag but > it won't do anything". Good idea! Done.
bauerb@chromium.org changed reviewers: + noms@chromium.org
Monica, can I get an OWNERS review for profiles/?
profiles lgtm.
The CQ bit was checked by bauerb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/789853002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4b5f497d0aeeb21877400bd46a2e7cfdbc98214d Cr-Commit-Position: refs/heads/master@{#308117} |
