Make the authentication service remember selected usernames.

Make the authentication service remember selected usernames.

R=ppi@chromium.org, tonyg@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/49b1910fc1e14a65e024b986b83e46011bcf185a

[tonyg, 2015-06-03 14:59:27]

Nice. API lgtm. I didn't review the mechanics of the android code super closely
-- assuming ppi@ has that.

https://codereview.chromium.org/1163483004/diff/1/mojo/services/authenticatio...
File mojo/services/authentication/public/interfaces/authentication.mojom
(right):

https://codereview.chromium.org/1163483004/diff/1/mojo/services/authenticatio...
mojo/services/authentication/public/interfaces/authentication.mojom:15:
SelectAccount(bool returned_last_selected) =>
s/returned/use/ or at least s/returned/return/?

Also, recommend updating comment.

[qsr, 2015-06-03 15:12:42]

https://codereview.chromium.org/1163483004/diff/1/mojo/services/authenticatio...
File mojo/services/authentication/public/interfaces/authentication.mojom
(right):

https://codereview.chromium.org/1163483004/diff/1/mojo/services/authenticatio...
mojo/services/authentication/public/interfaces/authentication.mojom:15:
SelectAccount(bool returned_last_selected) =>
On 2015/06/03 14:59:27, tonyg wrote:
> s/returned/use/ or at least s/returned/return/?
> 
> Also, recommend updating comment.

Done.

[ppi, 2015-06-03 17:11:17]

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
File services/authentication/authentication_impl_db.mojom (right):

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
services/authentication/authentication_impl_db.mojom:9: struct Db {
Why are we defining this in a .mojom and not directly in
AuthenticationServiceImpl.java?

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
services/authentication/authentication_impl_db.mojom:9: struct Db {
Oh, from the code it seems that we define that in mojom so that we have
serialization. Nifty:).

If that is the case, could you please state in the comment that this struct is
not actually passed between services?

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
File
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java
(right):

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:44:
// The current version of the database.
When this should be updated / incremented? Could you state that in the comment?

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:48:
private static final String GOOGLE_ACCOUNT_TYPE = "com.google";
Should we put "private static final String[] sAccountTypes = new String[]
{GOOGLE_ACCOUNT_TYPE};" here too?

A comment could explain that these are the accounts that this service allows the
user to use / pick.

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:172:
}
nit: maybe add a blank line afterwards?

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:279:
Message m = db.serialize(null);
At the first sight, it is puzzling to see Message being created here. Could you
add a comment that we're using mojo serialization to persist the db?

[qsr, 2015-06-04 09:16:15]

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
File services/authentication/authentication_impl_db.mojom (right):

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
services/authentication/authentication_impl_db.mojom:9: struct Db {
On 2015/06/03 17:11:17, ppi wrote:
> Why are we defining this in a .mojom and not directly in
> AuthenticationServiceImpl.java?

Acknowledged.

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
services/authentication/authentication_impl_db.mojom:9: struct Db {
On 2015/06/03 17:11:17, ppi wrote:
> Oh, from the code it seems that we define that in mojom so that we have
> serialization. Nifty:).
> 
> If that is the case, could you please state in the comment that this struct is
> not actually passed between services?

Done.

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
File
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java
(right):

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:44:
// The current version of the database.
On 2015/06/03 17:11:17, ppi wrote:
> When this should be updated / incremented? Could you state that in the
comment?

Done.

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:48:
private static final String GOOGLE_ACCOUNT_TYPE = "com.google";
On 2015/06/03 17:11:17, ppi wrote:
> Should we put "private static final String[] sAccountTypes = new String[]
> {GOOGLE_ACCOUNT_TYPE};" here too?
> 
> A comment could explain that these are the accounts that this service allows
the
> user to use / pick.

Done.

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:172:
}
On 2015/06/03 17:11:17, ppi wrote:
> nit: maybe add a blank line afterwards?

Done.

https://codereview.chromium.org/1163483004/diff/40001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:279:
Message m = db.serialize(null);
On 2015/06/03 17:11:17, ppi wrote:
> At the first sight, it is puzzling to see Message being created here. Could
you
> add a comment that we're using mojo serialization to persist the db?

Done.

[ppi, 2015-06-04 09:19:13]

lgtm % nit

https://codereview.chromium.org/1163483004/diff/60001/services/authentication...
File
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java
(right):

https://codereview.chromium.org/1163483004/diff/60001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:44:
// The current version of the database. This must be incremented each time Db is
changed in a
nit: maybe s/Db/authentication_impl_db.mojom/ or maybe: "Db definition in
authentication_impl_db.mojom", to make it clear that you mean the type?

[qsr, 2015-06-04 09:21:36]

https://codereview.chromium.org/1163483004/diff/60001/services/authentication...
File
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java
(right):

https://codereview.chromium.org/1163483004/diff/60001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:44:
// The current version of the database. This must be incremented each time Db is
changed in a
On 2015/06/04 09:19:12, ppi wrote:
> nit: maybe s/Db/authentication_impl_db.mojom/ or maybe: "Db definition in
> authentication_impl_db.mojom", to make it clear that you mean the type?

Done.

[qsr, 2015-06-04 09:46:35]

Committed patchset #5 (id:80001) manually as
49b1910fc1e14a65e024b986b83e46011bcf185a (presubmit successful).