Mask the AccountManager{Future,Callback} with a simple Callback.

Mask the AccountManager{Future,Callback} with a simple Callback.

This abstracts the implementation of SystemAccountManagerDelegate so
that it is easier to replace it with other implementations.

This will facilitate child account detection in Android M.

This cl also deprecates the existing synchronous getAccounts method
and adds an asynchronous method instead though this is presently
only used in ChildAccountService.

Depends on:
http://crrev.com/1368663002
http://crrev.com/1369503002

BUG=534377, 517697
Committed: https://crrev.com/bac7a75018ab99a2ab1883bcc53efe87861ccc6a
Cr-Commit-Position: refs/heads/master@{#352016}

[knn, 2015-09-21 14:13:53]

knn@chromium.org changed reviewers:
+ bauerb@chromium.org

[knn, 2015-09-21 14:13:54]

PTAL. Thanks!

[Bernhard Bauer, 2015-09-21 14:32:54]

https://codereview.chromium.org/1353393002/diff/1/sync/android/java/src/org/c...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java
(right):

https://codereview.chromium.org/1353393002/diff/1/sync/android/java/src/org/c...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:19: *
Generic callback class to be used to make this interface asynchronous.
Which interface? Maybe rather phrase it as "to allow asynchronous methods".

[knn, 2015-09-21 14:51:10]

PTAL.

https://codereview.chromium.org/1353393002/diff/1/sync/android/java/src/org/c...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java
(right):

https://codereview.chromium.org/1353393002/diff/1/sync/android/java/src/org/c...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:19: *
Generic callback class to be used to make this interface asynchronous.
On 2015/09/21 14:32:54, Bernhard Bauer wrote:
> Which interface? Maybe rather phrase it as "to allow asynchronous methods".

Done.

[Bernhard Bauer, 2015-09-21 15:23:35]

lgtm

https://codereview.chromium.org/1353393002/diff/20001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java
(right):

https://codereview.chromium.org/1353393002/diff/20001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:19: *
A callback class for use to allow asynchronous methods.
Grammar nit: "for use" takes a preposition ("for use in" or "for use with"), but
the "to" here is an infinitival particle
(https://en.wikipedia.org/wiki/Grammatical_particle#Infinitival_and_adverbial_...),
which makes this sentence read weirdly. Maybe just write "[...] that can be used
to allow [...]"?

[knn, 2015-09-22 10:21:37]

knn@chromium.org changed reviewers:
+ nyquist@chromium.org, rogerta@chromium.org

[knn, 2015-09-22 10:21:38]

Thanks!

Adding more owners:
rogerta@chromium.org: Please review changes in 
ChildAccountInfoFetcher.java

nyquist@chromium.org: Please review changes in 
sync/* (which is pretty much the entire cl)

https://codereview.chromium.org/1353393002/diff/20001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java
(right):

https://codereview.chromium.org/1353393002/diff/20001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:19: *
A callback class for use to allow asynchronous methods.
On 2015/09/21 15:23:35, Bernhard Bauer wrote:
> Grammar nit: "for use" takes a preposition ("for use in" or "for use with"),
but
> the "to" here is an infinitival particle
>
(https://en.wikipedia.org/wiki/Grammatical_particle#Infinitival_and_adverbial_...),
> which makes this sentence read weirdly. Maybe just write "[...] that can be
used
> to allow [...]"?

Done.

[nyquist, 2015-09-22 20:39:33]

https://codereview.chromium.org/1353393002/diff/40001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java
(right):

https://codereview.chromium.org/1353393002/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:23:
public interface BooleanCallback {
This is implicitly public, no? Same with the method.

https://codereview.chromium.org/1353393002/diff/40001/sync/android/java/src/o...
File
sync/android/java/src/org/chromium/sync/signin/SystemAccountManagerDelegate.java
(right):

https://codereview.chromium.org/1353393002/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/SystemAccountManagerDelegate.java:35:
private static final String TAG = "Auth";
cr_Auth, no?

https://codereview.chromium.org/1353393002/diff/40001/sync/test/android/javat...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1353393002/diff/40001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:73:
private static final String TAG = "MockAccountManager";
Why was this removed? Shouldn't this be cr_... something?

https://codereview.chromium.org/1353393002/diff/40001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:279:
hasAllFeatures = false;
Could you break out of the loop here?

Or even better, remove hasAllFeatures variable and just:
callback.gotResult(false);
return;
here, and callback.gotResult(true) after the loop?.

[knn, 2015-09-22 22:25:10]

https://codereview.chromium.org/1353393002/diff/40001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java
(right):

https://codereview.chromium.org/1353393002/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:23:
public interface BooleanCallback {
On 2015/09/22 20:39:33, nyquist wrote:
> This is implicitly public, no? Same with the method.

Acknowledged.

https://codereview.chromium.org/1353393002/diff/40001/sync/android/java/src/o...
File
sync/android/java/src/org/chromium/sync/signin/SystemAccountManagerDelegate.java
(right):

https://codereview.chromium.org/1353393002/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/SystemAccountManagerDelegate.java:35:
private static final String TAG = "Auth";
On 2015/09/22 20:39:33, nyquist wrote:
> cr_Auth, no?

no, This is now done in the normalizeTag function.
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...
Also android_logging.md suggests no prefix is preferred.

https://codereview.chromium.org/1353393002/diff/40001/sync/test/android/javat...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1353393002/diff/40001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:73:
private static final String TAG = "MockAccountManager";
On 2015/09/22 20:39:33, nyquist wrote:
> Why was this removed? Shouldn't this be cr_... something?

Same as above.

https://codereview.chromium.org/1353393002/diff/40001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:279:
hasAllFeatures = false;
On 2015/09/22 20:39:33, nyquist wrote:
> Could you break out of the loop here?
> 
> Or even better, remove hasAllFeatures variable and just:
> callback.gotResult(false);
> return;
> here, and callback.gotResult(true) after the loop?.

This was consciously done as this is a test code and it makes sense to see all
the errors at once? WDYT?

[nyquist, 2015-09-23 00:58:22]

lgtm

[knn, 2015-09-24 15:27:36]

Patchset #4 (id:60001) has been deleted

[knn, 2015-09-24 15:31:41]

Tommy, PTAL,
I added an async getAccounts method.
I tried moving the usages of the synchronous methods but I'll have to devote
some more time to understand it better.

I'd like to submit the rest of the changes before that.

[nyquist, 2015-09-24 18:13:02]

Could you add 517697 to the BUG= line?

https://codereview.chromium.org/1353393002/diff/80001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java
(right):

https://codereview.chromium.org/1353393002/diff/80001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:31:
void getAccountsByType(String type, Callback<Account[]> callback);
Can we remove "type" here? I think we only want this for the "com.google" type,
so it's unnecessary for everyone to pass that in in the new API. Just add a
comment to this method to clarify that.

https://codereview.chromium.org/1353393002/diff/80001/sync/android/java/src/o...
File
sync/android/java/src/org/chromium/sync/signin/SystemAccountManagerDelegate.java
(right):

https://codereview.chromium.org/1353393002/diff/80001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/SystemAccountManagerDelegate.java:60:
return getAccountsByType(type);
I think you can force "com.google" here.

[knn, 2015-09-24 19:25:55]

On 2015/09/24 18:13:02, nyquist wrote:
> Could you add 517697 to the BUG= line?
> 
>
https://codereview.chromium.org/1353393002/diff/80001/sync/android/java/src/o...
> File
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java
> (right):
> 
>
https://codereview.chromium.org/1353393002/diff/80001/sync/android/java/src/o...
> sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:31:
> void getAccountsByType(String type, Callback<Account[]> callback);
> Can we remove "type" here? I think we only want this for the "com.google"
type,
> so it's unnecessary for everyone to pass that in in the new API. Just add a
> comment to this method to clarify that.
> 
>
https://codereview.chromium.org/1353393002/diff/80001/sync/android/java/src/o...
> File
>
sync/android/java/src/org/chromium/sync/signin/SystemAccountManagerDelegate.java
> (right):
> 
>
https://codereview.chromium.org/1353393002/diff/80001/sync/android/java/src/o...
>
sync/android/java/src/org/chromium/sync/signin/SystemAccountManagerDelegate.java:60:
> return getAccountsByType(type);
> I think you can force "com.google" here.

Hi Tommy,

It seems like Kerberos support may use the type string.
Presently it uses getAuthTokenByFeatures directly but will soon split the call
into individual components.
(i.e. getAccounts, getFeatures, getToken ... )
https://code.google.com/p/chromium/codesearch#chromium/src/net/android/java/s...

I have added the BUG number, PTAL.

[nyquist, 2015-09-24 21:36:47]

lgtm

[knn, 2015-09-24 21:48:28]

On 2015/09/24 21:36:47, nyquist wrote:
> lgtm

Thanks Tommy! Updated the cl description.

Roger, please take a look at the very simple change in
ChildAccountInfoFetcher.java

[Roger Tawa OOO till Jul 10th, 2015-09-28 19:31:08]

lgtm ChildAccountInfoFetcher.java

[knn, 2015-09-29 09:12:24]

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

[commit-bot: I haz the power, 2015-09-29 09:12:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1353393002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1353393002/80001

[commit-bot: I haz the power, 2015-09-29 09:33:27]

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

[commit-bot: I haz the power, 2015-09-29 09:33:28]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[knn, 2015-10-02 12:01:43]

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

[commit-bot: I haz the power, 2015-10-02 12:03:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1353393002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1353393002/100001

[commit-bot: I haz the power, 2015-10-02 12:51:22]

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

[commit-bot: I haz the power, 2015-10-02 12:51:23]

Dry run: This issue passed the CQ dry run.

[knn, 2015-10-02 13:13:40]

The CQ bit was checked by knn@chromium.org

[knn, 2015-10-02 13:13:40]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
nyquist@chromium.org, rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/1353393002/#ps100001
(title: "fix test. move background task to where it is required.")

[commit-bot: I haz the power, 2015-10-02 13:14:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1353393002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1353393002/100001

[commit-bot: I haz the power, 2015-10-02 13:20:06]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-10-02 13:21:08]

Patchset 5 (id:??) landed as
https://crrev.com/bac7a75018ab99a2ab1883bcc53efe87861ccc6a
Cr-Commit-Position: refs/heads/master@{#352016}