settings: Make DeviceOAuth2TokenServiceFactory::Get() async

settings: Make DeviceOAuth2TokenServiceFactory::Get() async

This is in preparation for fixing nasty D-Bus method call issues in
DeviceOAuth2TokenService.

In crrev.com/228189, we had to add horrible hacks to workaround an urgent
P1 issue, which needed to be merged to M31 branch immediately.
The root cause of this problem is the fact that DeviceOAuth2TokenService
issues D-Bus method calls, which should be asynchronous, from places
where doing it asynchronously is difficult (in particular, making
GetRefreshToken() asynchronous is very difficult: crbug.com/309952).

A simpler solution for this problem is to get the system salt before
creating DeviceOAuth2TokenService, hence Get() should be asynchronous.

BUG=309959, 306547
TEST=none
R=courage@chromium.org, hashimoto@chromium.org, pastarmovj@chromium.org, pneubeck@chromium.org, xiyuan@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230624

[satorux1, 2013-10-22 07:33:38]

pneubeck: chrome/browser/chromeos/policy
pastarmovj: chrome/browser/chromeos/settings
courage: chrome/browser/extensions/api/identity
vitalybuka: chrome/browser/ui/webui/print_preview/print_preview_handler.cc
xiyuan: chrome/browser/chromeos/login/kiosk_browsertest.cc
hashimoto: in general :)

[hashimoto, 2013-10-22 08:49:51]

lgtm

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/kiosk_browsertest.cc:710:
base::RunLoop().RunUntilIdle();
nit: How about having ASSERT_TRUE(token_service) here?

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:395:
base::RunLoop().RunUntilIdle();
nit: How about having ASSERT_TRUE(token_service) here?

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:176: 
nit: No need to have a blank line here?

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.cc:402:
chromeos::DeviceOAuth2TokenService* service) {
nit: No need to have a NULL check here?

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/ui/webui/p...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/ui/webui/p...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:411:
chromeos::DeviceOAuth2TokenService* token_service) {
nit: No need to have a NULL check here?

[pneubeck (no reviews), 2013-10-22 09:05:03]

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:166:
weak_factory_.GetWeakPtr()));
it needs some reading of the code to see whether it is safe to reuse
weak_factory_ here, because there are InvalidateWeakPtrs() calls.

Alternatively, add another factory and name them according to their usage:
weak_factory_for_lock_device_
weak_factory_for_get_token_service_

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:176: 
nit: remove { } and empty line

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
(right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:44: //  
run callback asynchronously
do you mean "immediately" or "synchronously" instead of "asynchronously" here?

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:52: //
start getting the system salt asynchronously...
Why should we do this lazily triggered by the Get() function?
Couldn't we instead start getting the system salt always and as early as
possible? I.e. either in DeviceOAuth2TokenServiceFactory::Initialize() or in
CryptohomeClient ?

In anyways, putting most of the complexity described here into CryptohomeClient,
would have the advantage that it might simplify the implementation everywhere
where the system salt is required and not only solve the problem
DeviceOAuth2TokenService.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
(right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:24: //
May return null during browser startup and shutdown. Do not hold the
nit: null -> NULL

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:26: //
for null to handle the case where this instance is destroyed during
dito

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.cc:398: this));
should this really rely on the ref-counting and potentially extend the lifetime
of this object?

alternatively use a WeakPtr

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.h:32: #if
defined(OS_CHROMEOS)
nit: if-defined not strictly necessary

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/ui/webui/p...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/ui/webui/p...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:398:
&PrintPreviewHandler::AccessTokenService::DidGetTokenService,
PrintPreviewHandler:: really required? I thought qualification of the class name
is different within the class declaration itself.

[satorux1, 2013-10-22 09:56:43]

hashimoto and pneubeck, thank you for responding very quickly!

I don't have access to code so I'd just reply to your comments for now, and will
fix them tomorrow.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/kiosk_browsertest.cc:710:
base::RunLoop().RunUntilIdle();
On 2013/10/22 08:49:51, hashimoto wrote:
> nit: How about having ASSERT_TRUE(token_service) here?

good idea. will do.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:395:
base::RunLoop().RunUntilIdle();
On 2013/10/22 08:49:51, hashimoto wrote:
> nit: How about having ASSERT_TRUE(token_service) here?

good idea. will do.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:166:
weak_factory_.GetWeakPtr()));
On 2013/10/22 09:05:03, pneubeck wrote:
> it needs some reading of the code to see whether it is safe to reuse
> weak_factory_ here, because there are InvalidateWeakPtrs() calls.
> 
> Alternatively, add another factory and name them according to their usage:
> weak_factory_for_lock_device_
> weak_factory_for_get_token_service_

good point. will take another look tomorrow.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:176: 
On 2013/10/22 09:05:03, pneubeck wrote:
> nit: remove { } and empty line

Will fix.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:176: 
On 2013/10/22 08:49:51, hashimoto wrote:
> nit: No need to have a blank line here?

will fix.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
(right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:44: //  
run callback asynchronously
On 2013/10/22 09:05:03, pneubeck wrote:
> do you mean "immediately" or "synchronously" instead of "asynchronously" here?

I meant asynchronously. we could run callback.Run() here, but running it through
MessageLoop would be a better behavior, as the caller may not expect callback to
be run immediately

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:47: //
if there is no pending callback:
FWIW, I meant "if there is any pending callback"

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:52: //
start getting the system salt asynchronously...
On 2013/10/22 09:05:03, pneubeck wrote:
> Why should we do this lazily triggered by the Get() function?
> Couldn't we instead start getting the system salt always and as early as
> possible? I.e. either in DeviceOAuth2TokenServiceFactory::Initialize() or in
> CryptohomeClient ?

Thanks. Will explore the idea, when I implement the logic outlined here.

> In anyways, putting most of the complexity described here into
CryptohomeClient,
> would have the advantage that it might simplify the implementation everywhere
> where the system salt is required and not only solve the problem
> DeviceOAuth2TokenService.

Yes. hashimoto@ has just added retry logic in SystemSaltGetter, which helps for
everything.

https://codereview.chromium.org/34303002/

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
(right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:24: //
May return null during browser startup and shutdown. Do not hold the
On 2013/10/22 09:05:03, pneubeck wrote:
> nit: null -> NULL

will fix.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:26: //
for null to handle the case where this instance is destroyed during
On 2013/10/22 09:05:03, pneubeck wrote:
> dito

will fix.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.cc:398: this));
On 2013/10/22 09:05:03, pneubeck wrote:
> should this really rely on the ref-counting and potentially extend the
lifetime
> of this object?
> 
> alternatively use a WeakPtr

Inside API functions, relying on ref counting is a common practice, so I'd just
go with this, until this becomes a problem.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.cc:402:
chromeos::DeviceOAuth2TokenService* service) {
On 2013/10/22 08:49:51, hashimoto wrote:
> nit: No need to have a NULL check here?

The original code didn't have one, but sounds like a good idea. will add it.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.h:32: #if
defined(OS_CHROMEOS)
On 2013/10/22 09:05:03, pneubeck wrote:
> nit: if-defined not strictly necessary

Probably it's a good idea to have #ifdef to make it clear that this code is only
for chromeos.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/ui/webui/p...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/ui/webui/p...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:398:
&PrintPreviewHandler::AccessTokenService::DidGetTokenService,
On 2013/10/22 09:05:03, pneubeck wrote:
> PrintPreviewHandler:: really required? I thought qualification of the class
name
> is different within the class declaration itself.

good point. will fix.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/ui/webui/p...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:411:
chromeos::DeviceOAuth2TokenService* token_service) {
On 2013/10/22 08:49:51, hashimoto wrote:
> nit: No need to have a NULL check here?

will add.

[xiyuan, 2013-10-22 20:16:55]

Thank you for following up on this. I would appreciate you add more context for
the "horrible hacks" in CL description. We have to find a quick work around at
the time asap to unblock testers and a TT release. The fix needs to work on both
significantly refactored trunk and M31 branch. It is a terrible hack but it has
reasons. I also filed http://crbug.com/306547 to track the work to remove the
hack.

[satorux1, 2013-10-22 22:57:07]

On 2013/10/22 20:16:55, xiyuan wrote:
> Thank you for following up on this. I would appreciate you add more context
for
> the "horrible hacks" in CL description. We have to find a quick work around at
> the time asap to unblock testers and a TT release. The fix needs to work on
both
> significantly refactored trunk and M31 branch. It is a terrible hack but it
has
> reasons. I also filed http://crbug.com/306547 to track the work to remove the
> hack.

You are absolutely right. Without context, the description could be easily
misunderstood.
I've just updated the description. Hope this is clearer.

[xiyuan, 2013-10-22 23:21:16]

Cool. Thank you for clarifying in CL description.

[Michael Courage, 2013-10-23 00:22:46]

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.cc:402:
chromeos::DeviceOAuth2TokenService* service) {
On 2013/10/22 09:56:43, satorux1 wrote:
> On 2013/10/22 08:49:51, hashimoto wrote:
> > nit: No need to have a NULL check here?
> 
> The original code didn't have one, but sounds like a good idea. will add it.

When would the factory return NULL? Is this going to be a DCHECK or some new
error case developers might have to interpret?

[satorux1, 2013-10-23 04:23:26]

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.cc:402:
chromeos::DeviceOAuth2TokenService* service) {
On 2013/10/23 00:22:47, Michael Courage wrote:
> On 2013/10/22 09:56:43, satorux1 wrote:
> > On 2013/10/22 08:49:51, hashimoto wrote:
> > > nit: No need to have a NULL check here?
> > 
> > The original code didn't have one, but sounds like a good idea. will add it.
> 
> When would the factory return NULL? Is this going to be a DCHECK or some new
> error case developers might have to interpret? 

Per the header file [1], it can return NULL during the browser startup and
shutdown. I think it can also return NULL in unit tests where
DeviceOAuth2TokenServiceFactory is not initialized.  I think it's a good
practice to check NULL.

[1]
https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...

[satorux1, 2013-10-23 06:35:44]

Addressed all comments.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/kiosk_browsertest.cc:710:
base::RunLoop().RunUntilIdle();
On 2013/10/22 09:56:43, satorux1 wrote:
> On 2013/10/22 08:49:51, hashimoto wrote:
> > nit: How about having ASSERT_TRUE(token_service) here?
> 
> good idea. will do.

Done.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:395:
base::RunLoop().RunUntilIdle();
On 2013/10/22 09:56:43, satorux1 wrote:
> On 2013/10/22 08:49:51, hashimoto wrote:
> > nit: How about having ASSERT_TRUE(token_service) here?
> 
> good idea. will do.

Done.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:166:
weak_factory_.GetWeakPtr()));
On 2013/10/22 09:56:43, satorux1 wrote:
> On 2013/10/22 09:05:03, pneubeck wrote:
> > it needs some reading of the code to see whether it is safe to reuse
> > weak_factory_ here, because there are InvalidateWeakPtrs() calls.
> > 
> > Alternatively, add another factory and name them according to their usage:
> > weak_factory_for_lock_device_
> > weak_factory_for_get_token_service_
> 
> good point. will take another look tomorrow.

Took a look. It wasn't clear if it's safe to use the weak ptr factory here. I've
created a separate weak ptr factory for this as you suggested.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:176: 
On 2013/10/22 09:56:43, satorux1 wrote:
> On 2013/10/22 08:49:51, hashimoto wrote:
> > nit: No need to have a blank line here?
> 
> will fix.

Done.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h
(right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:24: //
May return null during browser startup and shutdown. Do not hold the
On 2013/10/22 09:56:43, satorux1 wrote:
> On 2013/10/22 09:05:03, pneubeck wrote:
> > nit: null -> NULL
> 
> will fix.

Done.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h:26: //
for null to handle the case where this instance is destroyed during
On 2013/10/22 09:56:43, satorux1 wrote:
> On 2013/10/22 09:05:03, pneubeck wrote:
> > dito
> 
> will fix.

Done.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.cc:402:
chromeos::DeviceOAuth2TokenService* service) {
On 2013/10/23 04:23:27, satorux1 wrote:
> On 2013/10/23 00:22:47, Michael Courage wrote:
> > On 2013/10/22 09:56:43, satorux1 wrote:
> > > On 2013/10/22 08:49:51, hashimoto wrote:
> > > > nit: No need to have a NULL check here?
> > > 
> > > The original code didn't have one, but sounds like a good idea. will add
it.
> > 
> > When would the factory return NULL? Is this going to be a DCHECK or some new
> > error case developers might have to interpret? 
> 
> Per the header file [1], it can return NULL during the browser startup and
> shutdown. I think it can also return NULL in unit tests where
> DeviceOAuth2TokenServiceFactory is not initialized.  I think it's a good
> practice to check NULL.
> 
> [1]
>
https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...

added:

  if (!service) {
    CompleteFunctionWithError(identity_constants::kAuthFailure);
    return;
  }
 
I thought about doing DCHECK(service) instead as this function won't be called
during browse startup and shutdown in production, but I decided to check NULL,
as this function may be called from tests where DeviceOAuth2TokenServiceFactory
is not initialized

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/ui/webui/p...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/ui/webui/p...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:398:
&PrintPreviewHandler::AccessTokenService::DidGetTokenService,
On 2013/10/22 09:56:43, satorux1 wrote:
> On 2013/10/22 09:05:03, pneubeck wrote:
> > PrintPreviewHandler:: really required? I thought qualification of the class
> name
> > is different within the class declaration itself.
> 
> good point. will fix.

Done.

https://codereview.chromium.org/34523003/diff/40001/chrome/browser/ui/webui/p...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:411:
chromeos::DeviceOAuth2TokenService* token_service) {
On 2013/10/22 09:56:43, satorux1 wrote:
> On 2013/10/22 08:49:51, hashimoto wrote:
> > nit: No need to have a NULL check here?
> 
> will add. 

Done.

[pneubeck (no reviews), 2013-10-23 07:27:38]

lgtm

https://codereview.chromium.org/34523003/diff/250001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/34523003/diff/250001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:183: // Used for
getting the OAuth2 token service. This needs to be separate
nit: empty line before comment.

[pastarmovj, 2013-10-23 08:02:16]

LGTM for settings with one request.

https://codereview.chromium.org/34523003/diff/250001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
(right):

https://codereview.chromium.org/34523003/diff/250001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:40: //
TODO(satorux): Implement async initialization logic for
I think it makes sense to have a bug number referenced here since this seems
like a major chunk that is still missing.

[xiyuan, 2013-10-23 16:16:26]

kiosk_browsertest.cc LGTM

[Michael Courage, 2013-10-23 18:04:10]

lgtm with one nit

https://codereview.chromium.org/34523003/diff/250001/chrome/browser/extension...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/34523003/diff/250001/chrome/browser/extension...
chrome/browser/extensions/api/identity/identity_api.cc:404:
CompleteFunctionWithError(identity_constants::kAuthFailure);
Can you change this to kUserNotSignedIn or a new string? kAuthFailure is used to
pass on errors coming back from gaia.

[satorux1, 2013-10-24 01:49:26]

https://codereview.chromium.org/34523003/diff/250001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/34523003/diff/250001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:183: // Used for
getting the OAuth2 token service. This needs to be separate
On 2013/10/23 07:27:38, pneubeck wrote:
> nit: empty line before comment.

Done.

https://codereview.chromium.org/34523003/diff/250001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
(right):

https://codereview.chromium.org/34523003/diff/250001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:40: //
TODO(satorux): Implement async initialization logic for
On 2013/10/23 08:02:17, pastarmovj wrote:
> I think it makes sense to have a bug number referenced here since this seems
> like a major chunk that is still missing.

good idea. done.

https://codereview.chromium.org/34523003/diff/250001/chrome/browser/extension...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/34523003/diff/250001/chrome/browser/extension...
chrome/browser/extensions/api/identity/identity_api.cc:404:
CompleteFunctionWithError(identity_constants::kAuthFailure);
On 2013/10/23 18:04:11, Michael Courage wrote:
> Can you change this to kUserNotSignedIn or a new string? kAuthFailure is used
to
> pass on errors coming back from gaia.  

Good catch. Changed to kUserNotSignedIn

[satorux1, 2013-10-24 03:54:50]

Committed patchset #5 manually as r230624 (presubmit successful).