Refactor AO2TS to make it easier to componentize.

Refactor AO2TS to make it easier to componentize

This cl is dedicated to refactor token service to make it easier to componentize. It introduces a new class OAuth2TokenServiceDelegate for each platform to implement it. OAuth2TokenService and its subclasses will call into platform specific delegate and the user can get that delegate through public interface GetDelegate() of OAuth2TokenService.

BUG=490882
Committed: https://crrev.com/63376b55f05201ea493fe6c26d3b9a8f26bc2f16
Cr-Commit-Position: refs/heads/master@{#337248}

[gogerald1, 2015-05-22 19:29:22]

gogerald@chromium.org changed reviewers:
+ rogerta@chromium.org

[gogerald1, 2015-05-22 19:41:42]

Patchset #1 (id:1) has been deleted

[gogerald1, 2015-05-22 20:01:07]

Hi Roger, This is the draft version of the work, I will keep working on it.

Below files are temporarily changed to compile unit_tests.
fake_profile_oauth2_token_service.cc
fake_profile_oauth2_token_service_builder.cc
fake_oauth2_token_service.cc

And there are three functions in oauth2_token_service.h were changed to virtual
temporarily compared to original patch for android ("comments: // ganggui
temp").

[Roger Tawa OOO till Jul 10th, 2015-05-24 21:13:19]

Looking good Ganggui!  Here are some initial comments, I can continue looking on
Tuesday.

https://codereview.chromium.org/1143323005/diff/20001/chrome/browser/signin/f...
File chrome/browser/signin/fake_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/20001/chrome/browser/signin/f...
chrome/browser/signin/fake_profile_oauth2_token_service.cc:71: //
FireRefreshTokensLoaded();
Probably don't need this class anymore, once we have a fake O2TS delegate.

https://codereview.chromium.org/1143323005/diff/20001/chrome/browser/signin/o...
File chrome/browser/signin/oauth2_token_service_delegate_android.h (right):

https://codereview.chromium.org/1143323005/diff/20001/chrome/browser/signin/o...
chrome/browser/signin/oauth2_token_service_delegate_android.h:105: const
std::string& access_token) override;
Nit: don't use virtual and override.  Use only the latter.

https://codereview.chromium.org/1143323005/diff/20001/components/signin/core/...
File components/signin/core/browser/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/20001/components/signin/core/...
components/signin/core/browser/profile_oauth2_token_service.cc:41: }
Should always cancel requests and clear the cache for the given account, no need
to check refresh token first.

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/fake_o...
File google_apis/gaia/fake_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/fake_o...
google_apis/gaia/fake_oauth2_token_service.cc:63: //
FireRefreshTokenRevoked(account_id);
We need a FakeOAuth2TokenServiceDelegate.  I think once this is done it will be
easier to implement these functions.

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service.h:181: const std::string& account_id)
const;  // ganggui_temp
The above two methods should not be virtual.  Is this what you mean by temp?

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service.h:219: const std::string& access_token);
I think this should be made private, otherwise there is no difference between
this and line 196 above.

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service.h:283: const ScopeSet& scopes);
This should not be virtual.

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service.h:293: OAuth2AccessTokenConsumer*
consumer);  // ganggui_temp
This should not be virtual.

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
File google_apis/gaia/oauth2_token_service_delegate.h (right):

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_delegate.h:32: virtual std::string
GetRefreshToken(const std::string& account_id) const = 0;
GetRefreshToken() should not be exposed here.  On some platforms, like mobile,
you can't implement this method.  Why do you need this method?

[gogerald1, 2015-05-25 21:06:45]

Patchset #2 (id:40001) has been deleted

[gogerald1, 2015-05-25 21:10:58]

Please ignore all modifications in the files with file "fake_*". Most of my
modifications are focused on files for Linux/Windows.

https://codereview.chromium.org/1143323005/diff/20001/chrome/browser/signin/f...
File chrome/browser/signin/fake_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/20001/chrome/browser/signin/f...
chrome/browser/signin/fake_profile_oauth2_token_service.cc:71: //
FireRefreshTokensLoaded();
On 2015/05/24 21:13:18, Roger Tawa wrote:
> Probably don't need this class anymore, once we have a fake O2TS delegate.

Probably, need more investigation after we lock down the interface and
implementation of the OAuth2TokenService (like FetchOAuth2Token).

https://codereview.chromium.org/1143323005/diff/20001/chrome/browser/signin/o...
File chrome/browser/signin/oauth2_token_service_delegate_android.h (right):

https://codereview.chromium.org/1143323005/diff/20001/chrome/browser/signin/o...
chrome/browser/signin/oauth2_token_service_delegate_android.h:105: const
std::string& access_token) override;
On 2015/05/24 21:13:18, Roger Tawa wrote:
> Nit: don't use virtual and override.  Use only the latter.

Done.

https://codereview.chromium.org/1143323005/diff/20001/chrome/browser/signin/o...
chrome/browser/signin/oauth2_token_service_delegate_android.h:105: const
std::string& access_token) override;
On 2015/05/24 21:13:18, Roger Tawa wrote:
> Nit: don't use virtual and override.  Use only the latter.

Something wrong here. It overrides the interface declared in
OAuth2TokenServiceDelegate, but it is not called by any interface in
OAuth2TokenService.

https://codereview.chromium.org/1143323005/diff/20001/components/signin/core/...
File components/signin/core/browser/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/20001/components/signin/core/...
components/signin/core/browser/profile_oauth2_token_service.cc:41: }
On 2015/05/24 21:13:18, Roger Tawa wrote:
> Should always cancel requests and clear the cache for the given account, no
need
> to check refresh token first.

what if the update credential is the current credential associated with the
account? In previous implementation it will do nothing.

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/fake_o...
File google_apis/gaia/fake_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/fake_o...
google_apis/gaia/fake_oauth2_token_service.cc:63: //
FireRefreshTokenRevoked(account_id);
On 2015/05/24 21:13:18, Roger Tawa wrote:
> We need a FakeOAuth2TokenServiceDelegate.  I think once this is done it will
be
> easier to implement these functions.

Acknowledged. Definitely

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service.h:181: const std::string& account_id)
const;  // ganggui_temp
On 2015/05/24 21:13:18, Roger Tawa wrote:
> The above two methods should not be virtual.  Is this what you mean by temp?

Done.

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service.h:181: const std::string& account_id)
const;  // ganggui_temp
On 2015/05/24 21:13:18, Roger Tawa wrote:
> The above two methods should not be virtual.  Is this what you mean by temp?

Yes, just make it pass the unit_tests compilation, since many other fake token
services override them.

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service.h:219: const std::string& access_token);
On 2015/05/24 21:13:18, Roger Tawa wrote:
> I think this should be made private, otherwise there is no difference between
> this and line 196 above.

Yes, I will move it into protect group, it was moved from protect group to
public in the modifications for android.

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service.h:283: const ScopeSet& scopes);
On 2015/05/24 21:13:18, Roger Tawa wrote:
> This should not be virtual.

I just find the fake token services override this interface, and this interface
has concrete implementation only in OAuth2TokenService, so if this interface is
non-virtual, it is difficult to change its behavior for tests. We may need
FakeOAuth2TokenService and FakeOAuth2TokenServiceDelegate for tests.

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service.h:293: OAuth2AccessTokenConsumer*
consumer);  // ganggui_temp
On 2015/05/24 21:13:18, Roger Tawa wrote:
> This should not be virtual.

Done.

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
File google_apis/gaia/oauth2_token_service_delegate.h (right):

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_delegate.h:32: virtual std::string
GetRefreshToken(const std::string& account_id) const = 0;
On 2015/05/24 21:13:19, Roger Tawa wrote:
> GetRefreshToken() should not be exposed here.  On some platforms, like mobile,
> you can't implement this method.  Why do you need this method?

That's because we need to know whether the updated refresh token is the same as
the already exist one to decide whether we should do anything. If this interface
is available we can use it instead of "RefreshTokenIsAvailable". Otherwise, how
about introduce another interface like "RefreshTokenIsPresent(const std::string&
account_id, const std::string& refresh_token)"?

[gogerald1, 2015-05-26 19:07:49]

Hi Roger, this update includes modifications for unit_tests, it passed all
unit_tests and normal use of chrome signin. Please review them.

[gogerald1, 2015-05-26 22:58:25]

update for google_apis_unittests,

[Roger Tawa OOO till Jul 10th, 2015-05-28 14:54:44]

Thanks Ganggui, again looks great.  Some more comments below to simply the O2TS
classes, since much of the works is moving to the delegate.

https://codereview.chromium.org/1143323005/diff/20001/components/signin/core/...
File components/signin/core/browser/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/20001/components/signin/core/...
components/signin/core/browser/profile_oauth2_token_service.cc:41: }
On 2015/05/25 21:10:57, gogerald1 wrote:
> On 2015/05/24 21:13:18, Roger Tawa wrote:
> > Should always cancel requests and clear the cache for the given account, no
> need
> > to check refresh token first.
> 
> what if the update credential is the current credential associated with the
> account? In previous implementation it will do nothing.

Acknowledged.

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
File google_apis/gaia/oauth2_token_service_delegate.h (right):

https://codereview.chromium.org/1143323005/diff/20001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_delegate.h:32: virtual std::string
GetRefreshToken(const std::string& account_id) const = 0;
On 2015/05/25 21:10:58, gogerald1 wrote:
> On 2015/05/24 21:13:19, Roger Tawa wrote:
> > GetRefreshToken() should not be exposed here.  On some platforms, like
mobile,
> > you can't implement this method.  Why do you need this method?
> 
> That's because we need to know whether the updated refresh token is the same
as
> the already exist one to decide whether we should do anything. If this
interface
> is available we can use it instead of "RefreshTokenIsAvailable". Otherwise,
how
> about introduce another interface like "RefreshTokenIsPresent(const
std::string&
> account_id, const std::string& refresh_token)"?

See other comments, but by having the token service add itself as an observer to
the delegate, I do not believe this method is necessary.

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/fake_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
chrome/browser/signin/fake_profile_oauth2_token_service.h:72: void
IssueAllRefreshTokensLoaded();
We don't need the three IssueXXX methods above, since they simply delegate.

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/fake_profile_oauth2_token_service_delegate.h (right):

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
chrome/browser/signin/fake_profile_oauth2_token_service_delegate.h:11: : public
OAuth2TokenServiceDelegate {
Why doesn't this derive from FakeOAuth2TokenServiceDelegate?  Could we combine
all the functionality from this class into FakeOAuth2TokenServiceDelegate so
that we don't need two fake delegate implementations?

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc
(right):

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc:133:
signin_error_controller_ = signin_error_controller;
Move lines 132 and 133 to the initializer list at line 128 above.

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc:156: if
(refresh_tokens_[account_id]->GetAuthStatus().IsPersistentError()) {
Better to leave the call to HasPersistentError().

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/profile_oauth2_token_service_factory.h (right):

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
chrome/browser/signin/profile_oauth2_token_service_factory.h:18: #endif
We don't need these two forward declarations anymore.

https://codereview.chromium.org/1143323005/diff/100001/components/signin/core...
File components/signin/core/browser/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/100001/components/signin/core...
components/signin/core/browser/profile_oauth2_token_service.cc:25:
GetDelegate()->LoadCredentials(primary_account_id);
Do we really need to call CancelAllRequests() on load?  The request list should
be empty as this point.

https://codereview.chromium.org/1143323005/diff/100001/components/signin/core...
components/signin/core/browser/profile_oauth2_token_service.cc:44: }
I don't think we need these three methods in this class.  If the PO2TS added
itself as an observer of the delegate, then it would receive notifications when
refresh tokens become available.  It could then cancel requests as needed.

By doing this, you don't need the function GetRefreshToken().  The delegate
won't fire a notification unless the token actually changes in
UpdateCredentials().

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/fake_...
File google_apis/gaia/fake_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/fake_...
google_apis/gaia/fake_oauth2_token_service.cc:51: ->RemoveAccount(account_id);
Should not need casts here and line 45.

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/fake_...
File google_apis/gaia/fake_oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/fake_...
google_apis/gaia/fake_oauth2_token_service.h:21: void RemoveAccount(const
std::string& account_id);
I think it would be cleaner to remove these functions and have callers do:

GetDelegate()->AddAccount()
GetDelegate()->RemoveAccount()

But would that be a lot of changes?

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/fake_...
google_apis/gaia/fake_oauth2_token_service.h:32: void
set_request_context(net::URLRequestContextGetter* request_context);
Don't need this method here.  Put this method directly on the fake delegate.

Also, add a new method:

   FakeOAuth2TokenServiceDelegate* GetDelegate()

so that calling code does not need to cast when access the fake delegate.

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/oauth...
File google_apis/gaia/oauth2_token_service_delegate.h (right):

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service_delegate.h:42: const std::string&
access_token){};
"OAuth2Token" is not descriptive enough.  Let's cleanup the method name and call
it "InvalidateAccessToken()".

Also, need an extra space before {} and no trailing ;

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service_delegate.h:60: }
Should put the implementation of this function in cc file.

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/oauth...
File google_apis/gaia/oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service_unittest.cc:102: };
Why do we need this class and not use FakeOAuth2TokenServiceDelegate?  If there
is something missing from the latter, it should be extended so that the fake can
be used in both places.

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service_unittest.cc:110:
->set_request_context(getter);
Don't cast and call set_request_context() here.  Make the caller set it before
calling the ctor.

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service_unittest.cc:132:
message_loop_.message_loop_proxy())));
Should pass the request context getter directly to the delegate.

[gogerald1, 2015-06-02 22:03:53]

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

[gogerald1, 2015-06-02 22:06:00]

The CQ bit was unchecked by gogerald@chromium.org

[gogerald1, 2015-06-02 22:06:16]

The CQ bit was checked by gogerald@chromium.org

[gogerald1, 2015-06-02 22:06:16]

The CQ bit was unchecked by gogerald@chromium.org

[gogerald1, 2015-06-02 22:06:43]

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

[gogerald1, 2015-06-02 22:06:47]

The CQ bit was unchecked by gogerald@chromium.org

[commit-bot: I haz the power, 2015-06-02 22:07:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1143323005/120001

[commit-bot: I haz the power, 2015-06-02 22:07:32]

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

[commit-bot: I haz the power, 2015-06-02 22:07:34]

Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[gogerald1, 2015-06-03 16:44:26]

Patchset #5 (id:120001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-06-03 17:58:02]

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

[commit-bot: I haz the power, 2015-06-03 17:58:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1143323005/140001

[commit-bot: I haz the power, 2015-06-03 18:06:47]

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

[commit-bot: I haz the power, 2015-06-03 18:06:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gogerald1, 2015-06-03 18:12:59]

Address previous comments, please help review them, thanks.

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/fake_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
chrome/browser/signin/fake_profile_oauth2_token_service.h:72: void
IssueAllRefreshTokensLoaded();
On 2015/05/28 14:54:43, Roger Tawa wrote:
> We don't need the three IssueXXX methods above, since they simply delegate.

Done.

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/fake_profile_oauth2_token_service_delegate.h (right):

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
chrome/browser/signin/fake_profile_oauth2_token_service_delegate.h:11: : public
OAuth2TokenServiceDelegate {
On 2015/05/28 14:54:43, Roger Tawa wrote:
> Why doesn't this derive from FakeOAuth2TokenServiceDelegate?  Could we combine
> all the functionality from this class into FakeOAuth2TokenServiceDelegate so
> that we don't need two fake delegate implementations?

Done.

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc
(right):

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc:133:
signin_error_controller_ = signin_error_controller;
On 2015/05/28 14:54:43, Roger Tawa wrote:
> Move lines 132 and 133 to the initializer list at line 128 above.

Done.

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc:156: if
(refresh_tokens_[account_id]->GetAuthStatus().IsPersistentError()) {
On 2015/05/28 14:54:43, Roger Tawa wrote:
> Better to leave the call to HasPersistentError()

The reason I am thinking to keep this function is to make it readable, can we
use a simple comment to achieve that to save a function call, in addition,
nobody else use it.

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/profile_oauth2_token_service_factory.h (right):

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
chrome/browser/signin/profile_oauth2_token_service_factory.h:18: #endif
On 2015/05/28 14:54:43, Roger Tawa wrote:
> We don't need these two forward declarations anymore.

Done.

https://codereview.chromium.org/1143323005/diff/100001/components/signin/core...
File components/signin/core/browser/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/100001/components/signin/core...
components/signin/core/browser/profile_oauth2_token_service.cc:25:
GetDelegate()->LoadCredentials(primary_account_id);
On 2015/05/28 14:54:43, Roger Tawa wrote:
> Do we really need to call CancelAllRequests() on load?  The request list
should
> be empty as this point.

Done. Confirmed with chromeos guy

https://codereview.chromium.org/1143323005/diff/100001/components/signin/core...
components/signin/core/browser/profile_oauth2_token_service.cc:44: }
On 2015/05/28 14:54:43, Roger Tawa wrote:
> I don't think we need these three methods in this class.  If the PO2TS added
> itself as an observer of the delegate, then it would receive notifications
when
> refresh tokens become available.  It could then cancel requests as needed.
> 
> By doing this, you don't need the function GetRefreshToken().  The delegate
> won't fire a notification unless the token actually changes in
> UpdateCredentials().

Done.

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/fake_...
File google_apis/gaia/fake_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/fake_...
google_apis/gaia/fake_oauth2_token_service.cc:51: ->RemoveAccount(account_id);
On 2015/05/28 14:54:44, Roger Tawa wrote:
> Should not need casts here and line 45.

Done.

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/fake_...
File google_apis/gaia/fake_oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/fake_...
google_apis/gaia/fake_oauth2_token_service.h:21: void RemoveAccount(const
std::string& account_id);
On 2015/05/28 14:54:44, Roger Tawa wrote:
> I think it would be cleaner to remove these functions and have callers do:
> 
> GetDelegate()->AddAccount()
> GetDelegate()->RemoveAccount()
> 
> But would that be a lot of changes?

Done.

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/fake_...
google_apis/gaia/fake_oauth2_token_service.h:32: void
set_request_context(net::URLRequestContextGetter* request_context);
On 2015/05/28 14:54:44, Roger Tawa wrote:
> Don't need this method here.  Put this method directly on the fake delegate.
> 
> Also, add a new method:
> 
>    FakeOAuth2TokenServiceDelegate* GetDelegate()
> 
> so that calling code does not need to cast when access the fake delegate.

Done.

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/oauth...
File google_apis/gaia/oauth2_token_service_delegate.h (right):

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service_delegate.h:42: const std::string&
access_token){};
On 2015/05/28 14:54:44, Roger Tawa wrote:
> "OAuth2Token" is not descriptive enough.  Let's cleanup the method name and
call
> it "InvalidateAccessToken()".
> 
> Also, need an extra space before {} and no trailing ;

Done.

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service_delegate.h:60: }
On 2015/05/28 14:54:44, Roger Tawa wrote:
> Should put the implementation of this function in cc file.

Done.

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/oauth...
File google_apis/gaia/oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service_unittest.cc:102: };
On 2015/05/28 14:54:44, Roger Tawa wrote:
> Why do we need this class and not use FakeOAuth2TokenServiceDelegate?  If
there
> is something missing from the latter, it should be extended so that the fake
can
> be used in both places.

Done.

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service_unittest.cc:110:
->set_request_context(getter);
On 2015/05/28 14:54:44, Roger Tawa wrote:
> Don't cast and call set_request_context() here.  Make the caller set it before
> calling the ctor.

Done.

https://codereview.chromium.org/1143323005/diff/100001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service_unittest.cc:132:
message_loop_.message_loop_proxy())));
On 2015/05/28 14:54:44, Roger Tawa wrote:
> Should pass the request context getter directly to the delegate.

Done.

[Roger Tawa OOO till Jul 10th, 2015-06-04 18:19:56]

Looks great Ganagui, just a few more questions.  IF you haven't already, sync up
and merge to run the tests on the bots.  Thanks.

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc
(right):

https://codereview.chromium.org/1143323005/diff/100001/chrome/browser/signin/...
chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc:156: if
(refresh_tokens_[account_id]->GetAuthStatus().IsPersistentError()) {
On 2015/06/03 18:12:58, gogerald1 wrote:
> On 2015/05/28 14:54:43, Roger Tawa wrote:
> > Better to leave the call to HasPersistentError()
> 
> The reason I am thinking to keep this function is to make it readable, can we
> use a simple comment to achieve that to save a function call, in addition,
> nobody else use it.

Acknowledged.

https://codereview.chromium.org/1143323005/diff/100001/components/signin/core...
File components/signin/core/browser/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/100001/components/signin/core...
components/signin/core/browser/profile_oauth2_token_service.cc:44: }
On 2015/06/03 18:12:58, gogerald1 wrote:
> On 2015/05/28 14:54:43, Roger Tawa wrote:
> > I don't think we need these three methods in this class.  If the PO2TS added
> > itself as an observer of the delegate, then it would receive notifications
> when
> > refresh tokens become available.  It could then cancel requests as needed.
> > 
> > By doing this, you don't need the function GetRefreshToken().  The delegate
> > won't fire a notification unless the token actually changes in
> > UpdateCredentials().
> 
> Done.

You say done, but the three methods are still here.  Is there a reason for that?
 Note that for RevokeCredentials(), you could listen for On
RefreshTokenRevoked() to call CancelRequestsForAccount() and
ClearCacheForAccount().

https://codereview.chromium.org/1143323005/diff/140001/chrome/browser/signin/...
File chrome/browser/signin/fake_profile_oauth2_token_service_builder.cc (right):

https://codereview.chromium.org/1143323005/diff/140001/chrome/browser/signin/...
chrome/browser/signin/fake_profile_oauth2_token_service_builder.cc:9: #include
"chrome/browser/signin/signin_error_controller_factory.h"
Remove headers that are no longer needed.

https://codereview.chromium.org/1143323005/diff/140001/chrome/browser/signin/...
File chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.h
(right):

https://codereview.chromium.org/1143323005/diff/140001/chrome/browser/signin/...
chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.h:48: void
Shutdown() override;
Add comment:  // Overridden from OAuth2TokenServiceDelegate.

https://codereview.chromium.org/1143323005/diff/140001/chrome/browser/signin/...
File
chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc
(left):

https://codereview.chromium.org/1143323005/diff/140001/chrome/browser/signin/...
chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc:387:
}
Why are the two tests above removed?

https://codereview.chromium.org/1143323005/diff/140001/extensions/shell/brows...
File extensions/shell/browser/shell_oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/140001/extensions/shell/brows...
extensions/shell/browser/shell_oauth2_token_service.h:36: std::string
account_id() const;
Since this is no longer a simple accessor method, chrome style guide says this
function should be called AccountId().

https://codereview.chromium.org/1143323005/diff/140001/extensions/shell/brows...
File extensions/shell/browser/shell_oauth2_token_service_delegate.cc (right):

https://codereview.chromium.org/1143323005/diff/140001/extensions/shell/brows...
extensions/shell/browser/shell_oauth2_token_service_delegate.cc:36:
DCHECK(!refresh_token_.empty());
Should this DCHECK_EQ(account_id, account_id_); ?

https://codereview.chromium.org/1143323005/diff/140001/google_apis/gaia/oauth...
File google_apis/gaia/oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/140001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service.cc:399: }
The above two functions just call through to the delegate. Do we need them here?

https://codereview.chromium.org/1143323005/diff/140001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service.cc:441: }
The above function just calls through to the delegate. Do we need here?

https://codereview.chromium.org/1143323005/diff/140001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service.cc:564: }
The above function just calls through to the delegate. Do we need here?

https://codereview.chromium.org/1143323005/diff/140001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service.cc:728: }
The above function just calls through to the delegate. Do we need here?

[gogerald1, 2015-06-08 15:11:35]

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

[commit-bot: I haz the power, 2015-06-08 15:12:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1143323005/160001

[commit-bot: I haz the power, 2015-06-08 15:12:29]

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

[commit-bot: I haz the power, 2015-06-08 15:12:30]

Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[gogerald1, 2015-06-09 15:37:29]

Patchset #6 (id:160001) has been deleted

[gogerald1, 2015-06-09 21:36:32]

Patchset #6 (id:180001) has been deleted

[gogerald1, 2015-06-10 14:17:24]

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

[commit-bot: I haz the power, 2015-06-10 14:17:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1143323005/200001

[commit-bot: I haz the power, 2015-06-10 14:17:41]

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

[commit-bot: I haz the power, 2015-06-10 14:17:42]

Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[gogerald1, 2015-06-15 17:23:45]

Patchset #9 (id:260001) has been deleted

[gogerald1, 2015-06-15 17:41:19]

Patchset #8 (id:240001) has been deleted

[gogerald1, 2015-06-15 17:41:35]

Patchset #8 (id:280001) has been deleted

[gogerald1, 2015-06-15 17:41:53]

Patchset #8 (id:300001) has been deleted

[gogerald1, 2015-06-15 22:58:34]

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

[commit-bot: I haz the power, 2015-06-15 22:59:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1143323005/430001

[commit-bot: I haz the power, 2015-06-15 23:00:02]

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

[commit-bot: I haz the power, 2015-06-15 23:00:03]

Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[gogerald1, 2015-06-16 14:46:10]

Patchset #8 (id:320001) has been deleted

[gogerald1, 2015-06-16 14:46:27]

Patchset #8 (id:340001) has been deleted

[gogerald1, 2015-06-16 14:46:43]

Patchset #8 (id:360001) has been deleted

[gogerald1, 2015-06-16 14:47:00]

Patchset #8 (id:380001) has been deleted

[gogerald1, 2015-06-16 14:47:14]

Patchset #8 (id:400001) has been deleted

[gogerald1, 2015-06-16 14:47:32]

Patchset #8 (id:420001) has been deleted

[gogerald1, 2015-06-16 18:00:29]

Patchset #12 (id:500001) has been deleted

[gogerald1, 2015-06-16 18:00:55]

Patchset #10 (id:470001) has been deleted

[gogerald1, 2015-06-16 18:01:13]

Patchset #9 (id:450001) has been deleted

[gogerald1, 2015-06-18 18:07:47]

Patchset #11 (id:540001) has been deleted

[gogerald1, 2015-06-18 18:08:48]

Patchset #11 (id:560001) has been deleted

[gogerald1, 2015-06-18 18:09:43]

Patchset #11 (id:580001) has been deleted

[gogerald1, 2015-06-18 18:10:28]

Patchset #25 (id:860001) has been deleted

[gogerald1, 2015-06-18 18:10:51]

Patchset #11 (id:600001) has been deleted

[gogerald1, 2015-06-18 18:11:16]

Patchset #11 (id:620001) has been deleted

[gogerald1, 2015-06-18 18:11:49]

Patchset #11 (id:470003) has been deleted

[gogerald1, 2015-06-18 18:12:31]

Patchset #11 (id:650001) has been deleted

[gogerald1, 2015-06-18 18:13:00]

Patchset #30 (id:1020001) has been deleted

[gogerald1, 2015-06-18 18:13:31]

Patchset #11 (id:560002) has been deleted

[gogerald1, 2015-06-18 18:13:54]

Patchset #11 (id:680001) has been deleted

[gogerald1, 2015-06-18 18:14:20]

Patchset #11 (id:700001) has been deleted

[gogerald1, 2015-06-18 18:14:40]

Patchset #11 (id:720001) has been deleted

[gogerald1, 2015-06-18 18:15:02]

Patchset #11 (id:740001) has been deleted

[gogerald1, 2015-06-18 18:15:22]

Patchset #11 (id:760001) has been deleted

[gogerald1, 2015-06-18 18:23:18]

Patchset #11 (id:780001) has been deleted

[gogerald1, 2015-06-18 18:28:27]

Patchset #11 (id:800001) has been deleted

[gogerald1, 2015-06-18 18:32:03]

Patchset #11 (id:820001) has been deleted

[gogerald1, 2015-06-18 18:32:26]

Patchset #11 (id:840001) has been deleted

[gogerald1, 2015-06-18 18:32:50]

Patchset #12 (id:890001) has been deleted

[gogerald1, 2015-06-18 18:33:44]

Patchset #12 (id:910001) has been deleted

[gogerald1, 2015-06-18 18:39:54]

Patchset #12 (id:930001) has been deleted

[gogerald1, 2015-06-18 18:40:55]

Patchset #12 (id:340002) has been deleted

[gogerald1, 2015-06-18 18:41:14]

Patchset #12 (id:940001) has been deleted

[gogerald1, 2015-06-18 18:41:31]

Patchset #12 (id:960001) has been deleted

[gogerald1, 2015-06-18 18:42:16]

Patchset #12 (id:980001) has been deleted

[gogerald1, 2015-06-18 18:42:34]

Patchset #12 (id:1000001) has been deleted

[gogerald1, 2015-06-18 18:42:54]

Patchset #12 (id:1040001) has been deleted

[gogerald1, 2015-06-22 22:45:42]

Patchset #14 (id:1100001) has been deleted

[gogerald1, 2015-06-22 22:45:59]

Patchset #14 (id:1120001) has been deleted

[gogerald1, 2015-06-22 22:46:17]

Patchset #14 (id:1140001) has been deleted

[gogerald1, 2015-06-23 18:42:03]

Patchset #14 (id:1160001) has been deleted

[gogerald1, 2015-06-23 18:42:23]

Patchset #14 (id:1180001) has been deleted

[gogerald1, 2015-06-23 18:42:42]

Patchset #15 (id:1220001) has been deleted

[gogerald1, 2015-06-23 18:43:00]

Patchset #15 (id:1240001) has been deleted

[gogerald1, 2015-06-23 18:43:15]

Patchset #15 (id:1260001) has been deleted

[gogerald1, 2015-06-23 18:43:31]

Patchset #14 (id:1200001) has been deleted

[gogerald1, 2015-06-23 18:43:46]

Patchset #14 (id:1280001) has been deleted

[gogerald1, 2015-06-25 13:58:42]

Patchset #17 (id:1360001) has been deleted

[gogerald1, 2015-06-25 13:59:00]

Patchset #14 (id:1300001) has been deleted

[gogerald1, 2015-06-25 13:59:21]

Patchset #14 (id:1320001) has been deleted

[gogerald1, 2015-06-25 13:59:45]

Patchset #14 (id:1340001) has been deleted

[gogerald1, 2015-06-25 14:06:19]

Hi, this review contains modifications for all platforms, please review it.

https://codereview.chromium.org/1143323005/diff/140001/chrome/browser/signin/...
File chrome/browser/signin/fake_profile_oauth2_token_service_builder.cc (right):

https://codereview.chromium.org/1143323005/diff/140001/chrome/browser/signin/...
chrome/browser/signin/fake_profile_oauth2_token_service_builder.cc:9: #include
"chrome/browser/signin/signin_error_controller_factory.h"
On 2015/06/04 18:19:55, Roger Tawa wrote:
> Remove headers that are no longer needed.

Done.

https://codereview.chromium.org/1143323005/diff/140001/chrome/browser/signin/...
File chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.h
(right):

https://codereview.chromium.org/1143323005/diff/140001/chrome/browser/signin/...
chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.h:48: void
Shutdown() override;
On 2015/06/04 18:19:55, Roger Tawa wrote:
> Add comment:  // Overridden from OAuth2TokenServiceDelegate.

Done.

https://codereview.chromium.org/1143323005/diff/140001/chrome/browser/signin/...
File
chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc
(left):

https://codereview.chromium.org/1143323005/diff/140001/chrome/browser/signin/...
chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc:387:
}
On 2015/06/04 18:19:55, Roger Tawa wrote:
> Why are the two tests above removed?

The test "TokenServiceUpdateClearsCache" test the cache function of access
tokens in OAuth2_token_service.cc, so move it to
Oauth2_token_service_unittest.cc (Didn't implement it before since it is a
simply function mostly covered by other test cases). The test
"FetchTransientError" test whether the function "UpdateAuthError" record
Transient Error, which has been covered by below test cases.

https://codereview.chromium.org/1143323005/diff/140001/components/signin/core...
File components/signin/core/browser/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/140001/components/signin/core...
components/signin/core/browser/profile_oauth2_token_service.cc:42:
ClearCacheForAccount(account_id);
try to save one function lookup and call, but since we already register this
class as an observer, so it will not going to save anything, move them to
observer callback function.

https://codereview.chromium.org/1143323005/diff/140001/extensions/shell/brows...
File extensions/shell/browser/shell_oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/140001/extensions/shell/brows...
extensions/shell/browser/shell_oauth2_token_service.h:36: std::string
account_id() const;
On 2015/06/04 18:19:55, Roger Tawa wrote:
> Since this is no longer a simple accessor method, chrome style guide says this
> function should be called AccountId().

Done.

https://codereview.chromium.org/1143323005/diff/140001/extensions/shell/brows...
File extensions/shell/browser/shell_oauth2_token_service_delegate.cc (right):

https://codereview.chromium.org/1143323005/diff/140001/extensions/shell/brows...
extensions/shell/browser/shell_oauth2_token_service_delegate.cc:36:
DCHECK(!refresh_token_.empty());
On 2015/06/04 18:19:55, Roger Tawa wrote:
> Should this DCHECK_EQ(account_id, account_id_); ?

Acknowledged. We may need to do both.

https://codereview.chromium.org/1143323005/diff/140001/google_apis/gaia/oauth...
File google_apis/gaia/oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/140001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service.cc:399: }
On 2015/06/04 18:19:56, Roger Tawa wrote:
> The above two functions just call through to the delegate. Do we need them
here?

Acknowledged.

https://codereview.chromium.org/1143323005/diff/140001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service.cc:441: }
On 2015/06/04 18:19:56, Roger Tawa wrote:
> The above function just calls through to the delegate. Do we need here?

Acknowledged.

https://codereview.chromium.org/1143323005/diff/140001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service.cc:564: }
On 2015/06/04 18:19:56, Roger Tawa wrote:
> The above function just calls through to the delegate. Do we need here?

Acknowledged.

https://codereview.chromium.org/1143323005/diff/140001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service.cc:728: }
On 2015/06/04 18:19:55, Roger Tawa wrote:
> The above function just calls through to the delegate. Do we need here?

Acknowledged.

[Roger Tawa OOO till Jul 10th, 2015-06-26 14:58:51]

lgtm

Thanks Ganggui for this CL!

https://codereview.chromium.org/1143323005/diff/140001/extensions/shell/brows...
File extensions/shell/browser/shell_oauth2_token_service_delegate.cc (right):

https://codereview.chromium.org/1143323005/diff/140001/extensions/shell/brows...
extensions/shell/browser/shell_oauth2_token_service_delegate.cc:36:
DCHECK(!refresh_token_.empty());
On 2015/06/25 14:06:19, gogerald1 wrote:
> On 2015/06/04 18:19:55, Roger Tawa wrote:
> > Should this DCHECK_EQ(account_id, account_id_); ?
> 
> Acknowledged. We may need to do both.

did you forget to add new DCHECK?

[gogerald1, 2015-06-26 17:14:38]

gogerald@chromium.org changed reviewers:
+ caitkp@chromium.org, droger@chromium.org, jcivelli@chromium.org,
mek@chromium.org, miguelg@chromium.org, mnissler@chromium.org, msw@chromium.org,
pavely@chromium.org, treib@chromium.org, vitalybuka@chromium.org,
xiyuan@chromium.org, zea@chromium.org

[gogerald1, 2015-06-26 17:14:40]

jcivelli@chromium.org: Please review changes in 
chrome/test/base/testing_profile.cc

vitalybuka@chromium.org: Please review changes in 
chrome/browser/local_discovery/gcd_api_flow_unittest.cc

pavely@chromium.org: Please review changes in 
chrome/browser/invalidation/gcm_invalidation_bridge_unittest.cc

msw@chromium.org: Please review changes in 
chrome/browser/ui/app_list/speech_auth_helper_unittest.cc
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm
chrome/browser/ui/views/profiles/profile_chooser_view.cc

xiyuan@chromium.org: Please review changes in 
chrome/browser/chromeos/policy/consumer_enrollment_handler_unittest.cc
chrome/browser/chromeos/policy/upload_job_impl.cc
chrome/browser/chromeos/policy/upload_job_unittest.cc
chrome/browser/chromeos/settings/device_oauth2_token_service.h
chrome/browser/chromeos/settings/device_oauth2_token_service.cc

caitkp@chromium.org: Please review changes in 
components/autofill/content/browser/wallet/real_pan_wallet_client_unittest.cc
components/autofill/core/browser/wallet/real_pan_wallet_client.cc
components/history/core/browser/web_history_service.cc
components/invalidation/gcm_invalidation_bridge.cc
components/invalidation/ticl_invalidation_service.cc
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher_impl_unittest.cc

droger@chromium.org: Please review changes in 
ios/chrome/browser/enhanced_bookmarks/bookmark_server_cluster_service_factory.cc
ios/public/provider/chrome/browser/keyed_service_provider.h
ios/public/test/test_keyed_service_provider.h
ios/public/test/test_keyed_service_provider.cc

treib@chromium.org: Please review changes in 
chrome/browser/supervised_user/child_accounts/family_info_fetcher.cc
chrome/browser/supervised_user/child_accounts/family_info_fetcher_unittest.cc
chrome/browser/supervised_user/child_accounts/permission_request_creator_apiary.cc
chrome/browser/supervised_user/child_accounts/permission_request_creator_apiary_unittest.cc
chrome/browser/supervised_user/legacy/supervised_user_refresh_token_fetcher.cc
chrome/browser/supervised_user/legacy/supervised_user_refresh_token_fetcher_unittest.cc

miguelg@chromium.org: Please review changes in 
chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java
chrome/browser/android/chrome_jni_registrar.cc
chrome/browser/android/signin/signin_manager_android.cc

mnissler@chromium.org: Please review changes in 
chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc

zea@chromium.org: Please review changes in 
chrome/browser/services/gcm/gcm_account_tracker_unittest.cc
chrome/browser/sync/profile_sync_auth_provider.cc
chrome/browser/sync/profile_sync_auth_provider_unittest.cc
chrome/browser/sync/profile_sync_service.cc
chrome/browser/sync/startup_controller_unittest.cc
sync/internal_api/attachments/attachment_downloader_impl_unittest.cc
sync/internal_api/attachments/attachment_uploader_impl_unittest.cc

mek@chromium.org: Please review changes in 
chrome/browser/extensions/api/identity/identity_apitest.cc
chrome/browser/extensions/updater/extension_updater_unittest.cc
extensions/browser/updater/extension_downloader.cc
extensions/shell/app_shell.gypi
extensions/shell/browser/api/identity/identity_api.cc
extensions/shell/browser/shell_oauth2_token_service.h
extensions/shell/browser/shell_oauth2_token_service.cc
extensions/shell/browser/shell_oauth2_token_service_delegate.h
extensions/shell/browser/shell_oauth2_token_service_delegate.cc
extensions/shell/browser/shell_oauth2_token_service_unittest.cc

[droger, 2015-06-26 17:19:37]

msarda : can you take a look at iOS changes?

[Nicolas Zea, 2015-06-26 18:09:36]

gcm and sync LGTM

[xiyuan, 2015-06-26 18:15:53]

+mnissler fyi

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/chrome...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:118:
(DeviceOAuth2TokenServiceDelegate*)GetDelegate();
nit: static_cast<DeviceOAuth2TokenServiceDelegate*>(GetDelegate());

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/chrome...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:50: class
DeviceOAuth2TokenServiceDelegate : public OAuth2TokenServiceDelegate {
nit: We can probably move DeviceOAuth2TokenServiceDelegate declartion into cc
file. See https://www.chromium.org/developers/coding-style/cpp-dos-and-donts,
look for "Move inner classes into the implementation."

Also, does this class needs to be public?

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:52: friend class
DeviceOAuth2TokenService;
Do we need this? Think inner class is friend of outer class by default.

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:57:
OAuth2AccessTokenFetcher* CreateAccessTokenFetcher(
nit: // OAuth2TokenServiceDelegate

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:71:
DeviceOAuth2TokenService* device_token_service_;
nit: DISALLOW_COPY_AND_ASSIGN(...);

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
google_apis/gaia/oauth2_token_service.h:133:
OAuth2TokenService(OAuth2TokenServiceDelegate* delegate);
nit: explicit and use scoped_ptr to make ownership transfer more clear,

i.e.
  explicit OAuth2TokenService(scoped_ptr<OAuth2TokenServiceDelegate> delegate);

[pavely, 2015-06-26 18:30:38]

chrome/browser/invalidation lgtm

[Vitaly Buka (NO REVIEWS), 2015-06-26 18:37:55]

chrome/browser/local_discovery lgtm

[Cait (Slow), 2015-06-26 19:12:32]

components/autofill/, components/history/, components/invalidation,
components/proximity_auth/ LGTM

[Marc Treib, 2015-06-29 07:56:11]

c/b/supervised_user/ LGTM

[Mattias Nissler (ping if slow), 2015-06-29 08:20:07]

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/chrome...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:46:
DeviceOAuth2TokenService::DeviceOAuth2TokenServiceDelegate::
It's pretty odd that the CL introduces a delegate interface that the code here
then implements trivially to delegate back to DeviceOAuth2TokenService. So in a
sense, DeviceOAuth2TokenService is now its own delegate, which seems a bit
silly. Is there a longer-term plan for this that I'm missing?

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:78: const
GoogleServiceAuthError& error) {
Should have a comment explaining why it's OK that this is empty.

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:91:
RevokeAllCredentials() {
Again, please explain why it's OK that this is empty.

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:118:
(DeviceOAuth2TokenServiceDelegate*)GetDelegate();
On 2015/06/26 18:15:52, xiyuan wrote:
> nit: static_cast<DeviceOAuth2TokenServiceDelegate*>(GetDelegate());

Actually, it looks like none of the uses of
|device_oauth2_token_service_delegate_| requires the specific subtype, i.e.
DeviceOAuth2TokenServiceDelegate. If so, does it make sense at all to keep a
pointer of type DeviceOAuth2TokenServiceDelegate?

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
google_apis/gaia/oauth2_token_service.h:182: void RevokeAllCredentials();
Would be good to put a comment stating what this does. Drop all access tokens?
Drop even refresh tokens?

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
google_apis/gaia/oauth2_token_service.h:207: OAuth2TokenServiceDelegate*
GetDelegate();
From an architectural perspective, it seems wrong to have this function in the
public interface. The CL description suggests that the delegate makes it easier
to implement the platform-specific logic for token service implementations
separate from OAuth2TokenService. I thus expected that the delegate is merely an
implementation detail of O2TS (and subclasses) that consumers wouldn't need to
know about. However, this function means that all the complexity of having to
deal with a delegate is exposed, and in fact some code does make calls directly
into the delegate.

Roger, what are your thoughts regarding this?

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
File google_apis/gaia/oauth2_token_service_delegate.h (right):

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
google_apis/gaia/oauth2_token_service_delegate.h:19: //
ProfileOAuth2TokenService. It allows token to be fetched from other external
nit: Why is this talking about *Profile*OAuth2TokenService(Delegate) and why is
the delegate an "alaternative" to the token service? IIUC, you still need both
for the system to be functional.

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
google_apis/gaia/oauth2_token_service_delegate.h:82: int batch_change_depth_;
nit: DISALLOW_COPY_AND_ASSIGN?

[msarda, 2015-06-29 08:49:43]

msarda@chromium.org changed reviewers:
+ msarda@chromium.org

[msarda, 2015-06-29 08:49:45]

LGTM for the iOS code.

This needs some casts on the iOS downstream code as the iOS specific functions
are now part of the delegate (not of the PO2TS), so please let me know when you
plan to land this so I can cherry-pick it.

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
google_apis/gaia/oauth2_token_service.h:207: OAuth2TokenServiceDelegate*
GetDelegate();
On 2015/06/29 08:20:06, Mattias Nissler wrote:
> From an architectural perspective, it seems wrong to have this function in the
> public interface. The CL description suggests that the delegate makes it
easier
> to implement the platform-specific logic for token service implementations
> separate from OAuth2TokenService. I thus expected that the delegate is merely
an
> implementation detail of O2TS (and subclasses) that consumers wouldn't need to
> know about. However, this function means that all the complexity of having to
> deal with a delegate is exposed, and in fact some code does make calls
directly
> into the delegate.
> 
> Roger, what are your thoughts regarding this?

I agree that from architectural perspective having the delegate public may look
strange. However, on iOS we have specific logic in PO2TS_IOS to reload
credentials. That logic is now being moved to the delegate. So if we do not want
any of that logic in the token service and we do not want to subclass the PO2TS
any more, then we need this delegate to be public.

[Mattias Nissler (ping if slow), 2015-06-29 09:01:29]

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
google_apis/gaia/oauth2_token_service.h:207: OAuth2TokenServiceDelegate*
GetDelegate();
On 2015/06/29 08:49:44, msarda wrote:
> On 2015/06/29 08:20:06, Mattias Nissler wrote:
> > From an architectural perspective, it seems wrong to have this function in
the
> > public interface. The CL description suggests that the delegate makes it
> easier
> > to implement the platform-specific logic for token service implementations
> > separate from OAuth2TokenService. I thus expected that the delegate is
merely
> an
> > implementation detail of O2TS (and subclasses) that consumers wouldn't need
to
> > know about. However, this function means that all the complexity of having
to
> > deal with a delegate is exposed, and in fact some code does make calls
> directly
> > into the delegate.
> > 
> > Roger, what are your thoughts regarding this?
> 
> I agree that from architectural perspective having the delegate public may
look
> strange. However, on iOS we have specific logic in PO2TS_IOS to reload
> credentials. That logic is now being moved to the delegate. So if we do not
want
> any of that logic in the token service and we do not want to subclass the
PO2TS
> any more, then we need this delegate to be public.

Who calls that logic to reload credentials? If that's an action which is
relevant for consumers in general, it may make sense to lift the API into PO2TS.
If the call comes from IOS-specific code, it may make sense for that code to
hold on to the delegate pointer that it passed to PO2TS at construction time so
it can make the call without relying on the getter here. I don't know your use
case though, so these suggestions are merely a stab in the dark.

[droger, 2015-06-29 11:55:54]

ios/chrome and ios/public LGTM with nit:

https://codereview.chromium.org/1143323005/diff/1380001/ios/public/provider/c...
File ios/public/provider/chrome/browser/keyed_service_provider.h (right):

https://codereview.chromium.org/1143323005/diff/1380001/ios/public/provider/c...
ios/public/provider/chrome/browser/keyed_service_provider.h:63: virtual
KeyedServiceBaseFactory* GetProfileOAuth2TokenServiceIOSFactory() = 0;
Can we also rename the functions here (remove IOS):
GetProfileOAuth2TokenServiceFactory()
GetProfileOAuth2TokenServiceForBrowserState()

[Marijn Kruisselbrink, 2015-06-29 17:21:39]

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/extens...
File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/extens...
chrome/browser/extensions/updater/extension_updater_unittest.cc:390:
fake_token_service_->GetDelegate()->UpdateCredentials(kFakeAccountId,
Similar to some of the other comments it seems a bit weird to have to call a
method on GetDelegate() here. But in this case it seems especially weird since
ProfileOAuth2TokenService already adds an UpdateCredentials method that just
calls the same method on the delegate; why not move those pass-through methods
to OAuth2TokenService and get rid of the GetDelegate() here?

[msw, 2015-06-29 19:42:39]

c/b/ui lgtm with an optional nit.

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/ui/ash...
File chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc (right):

https://codereview.chromium.org/1143323005/diff/1380001/chrome/browser/ui/ash...
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc:68:
oauth2_token_service->RevokeCredentials(account_id);
optional nit: inline the GetForProfile call, like cocoa.

[gogerald1, 2015-06-29 22:55:45]

Patchset #15 (id:1400001) has been deleted

[gogerald1, 2015-06-29 23:39:21]

Patchset #15 (id:1420001) has been deleted

[gogerald1, 2015-06-30 00:36:27]

Patchset #15 (id:1440001) has been deleted

[gogerald1, 2015-06-30 01:12:47]

Patchset #15 (id:1460001) has been deleted

[gogerald1, 2015-06-30 13:32:29]

Patchset #15 (id:1480001) has been deleted

[gogerald1, 2015-06-30 13:33:11]

Patchset #15 (id:1500001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-06-30 19:55:31]

Hi Mattias,

Please see below.  Thanks.

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
google_apis/gaia/oauth2_token_service.h:207: OAuth2TokenServiceDelegate*
GetDelegate();
On 2015/06/29 09:01:29, Mattias Nissler wrote:
> On 2015/06/29 08:49:44, msarda wrote:
> > On 2015/06/29 08:20:06, Mattias Nissler wrote:
> > > From an architectural perspective, it seems wrong to have this function in
> the
> > > public interface. The CL description suggests that the delegate makes it
> > easier
> > > to implement the platform-specific logic for token service implementations
> > > separate from OAuth2TokenService. I thus expected that the delegate is
> merely
> > an
> > > implementation detail of O2TS (and subclasses) that consumers wouldn't
need
> to
> > > know about. However, this function means that all the complexity of having
> to
> > > deal with a delegate is exposed, and in fact some code does make calls
> > directly
> > > into the delegate.
> > > 
> > > Roger, what are your thoughts regarding this?
> > 
> > I agree that from architectural perspective having the delegate public may
> look
> > strange. However, on iOS we have specific logic in PO2TS_IOS to reload
> > credentials. That logic is now being moved to the delegate. So if we do not
> want
> > any of that logic in the token service and we do not want to subclass the
> PO2TS
> > any more, then we need this delegate to be public.
> 
> Who calls that logic to reload credentials? If that's an action which is
> relevant for consumers in general, it may make sense to lift the API into
PO2TS.
> If the call comes from IOS-specific code, it may make sense for that code to
> hold on to the delegate pointer that it passed to PO2TS at construction time
so
> it can make the call without relying on the getter here. I don't know your use
> case though, so these suggestions are merely a stab in the dark.

There are a few types of token services in chrome, depending on platform.  On
desktop, the token service implementation does it all.  It mints and caches
access tokens, manages refresh tokens/accounts, and updates the access tokens
from refresh tokens.

On mobile, the token service implementation is a thin wrapper around non-chrome
services.  In these cases, the token service mints and caches access tokens but
has no way to manipulate refresh tokens (i.e., add and/or remove accounts from
the device).  The delegate updates access tokens when needed, as that is
platform specific.  Furthermore, there are other mobile specific things that
need to be done, and that code now belongs to the delegate as well.

Cros is slightly different.  The token service always has at least one refresh
token matching the signed in user.  This token cannot be removed.  But when
account consistency is enabled, cros can manager secondary accounts.

This CL keeps the interface for requesting access tokens and the caching of
these tokens in the OAuth2TokenService class.  The delegate handles updating the
access tokens from the refresh tokens.  The management of refresh tokens is also
done by the delegate if needed.  There is no longer a need to have
OAuth2TokenService derivative classes per platform.

An alternative could be to save the delegate pointer somewhere and then get it
directly, as you suggest.  Maybe the delegate should even be its own PKS.  But I
don't think it's a problem for the token service to have a public GetDelegate()
method.  The token service is always backed by some delegate.

If you feel strongly about this, I could open a bug to track how to better
manage the token service delegates as a follow up CL.  This CL is already large
enough.  Is that OK?

[Mattias Nissler (ping if slow), 2015-06-30 20:42:52]

Here's a reply to the discussion regarding GetDelegate() being public. I will
take a closer look at the DeviceOAuth2TokenService changes tomorrow (it's 11pm
here and my brain isn't up to that task right now).

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
google_apis/gaia/oauth2_token_service.h:207: OAuth2TokenServiceDelegate*
GetDelegate();
On 2015/06/30 19:55:30, Roger Tawa wrote:
> On 2015/06/29 09:01:29, Mattias Nissler wrote:
> > On 2015/06/29 08:49:44, msarda wrote:
> > > On 2015/06/29 08:20:06, Mattias Nissler wrote:
> > > > From an architectural perspective, it seems wrong to have this function
in
> > the
> > > > public interface. The CL description suggests that the delegate makes it
> > > easier
> > > > to implement the platform-specific logic for token service
implementations
> > > > separate from OAuth2TokenService. I thus expected that the delegate is
> > merely
> > > an
> > > > implementation detail of O2TS (and subclasses) that consumers wouldn't
> need
> > to
> > > > know about. However, this function means that all the complexity of
having
> > to
> > > > deal with a delegate is exposed, and in fact some code does make calls
> > > directly
> > > > into the delegate.
> > > > 
> > > > Roger, what are your thoughts regarding this?
> > > 
> > > I agree that from architectural perspective having the delegate public may
> > look
> > > strange. However, on iOS we have specific logic in PO2TS_IOS to reload
> > > credentials. That logic is now being moved to the delegate. So if we do
not
> > want
> > > any of that logic in the token service and we do not want to subclass the
> > PO2TS
> > > any more, then we need this delegate to be public.
> > 
> > Who calls that logic to reload credentials? If that's an action which is
> > relevant for consumers in general, it may make sense to lift the API into
> PO2TS.
> > If the call comes from IOS-specific code, it may make sense for that code to
> > hold on to the delegate pointer that it passed to PO2TS at construction time
> so
> > it can make the call without relying on the getter here. I don't know your
use
> > case though, so these suggestions are merely a stab in the dark.
> 
> There are a few types of token services in chrome, depending on platform.  On
> desktop, the token service implementation does it all.  It mints and caches
> access tokens, manages refresh tokens/accounts, and updates the access tokens
> from refresh tokens.
> 
> On mobile, the token service implementation is a thin wrapper around
non-chrome
> services.  In these cases, the token service mints and caches access tokens
but
> has no way to manipulate refresh tokens (i.e., add and/or remove accounts from
> the device).  The delegate updates access tokens when needed, as that is
> platform specific.  Furthermore, there are other mobile specific things that
> need to be done, and that code now belongs to the delegate as well.
> 
> Cros is slightly different.  The token service always has at least one refresh
> token matching the signed in user.  This token cannot be removed.  But when
> account consistency is enabled, cros can manager secondary accounts.
> 
> This CL keeps the interface for requesting access tokens and the caching of
> these tokens in the OAuth2TokenService class.  The delegate handles updating
the
> access tokens from the refresh tokens.  The management of refresh tokens is
also
> done by the delegate if needed.  There is no longer a need to have
> OAuth2TokenService derivative classes per platform.

That's awesome, and I'm very happy to hear this. Does that mean the plan is to
remove all OAuth2TokenService derivatives in follow-up CLs?

> 
> An alternative could be to save the delegate pointer somewhere and then get it
> directly, as you suggest.  Maybe the delegate should even be its own PKS.  But
I
> don't think it's a problem for the token service to have a public
GetDelegate()
> method.  The token service is always backed by some delegate.

As long as regular consumers (i.e. services that just request access tokens)
don't have to be aware of the fact that there's a delegate, I'm fine with
keeping the public GetDelegate() function. My main concern was to avoid making
the OAuth2TokenService API more complex to use for consumers.

In that spirit, I suggest to simplify FakeOAuth2TokenService to provide
functions that allow consumer to set up fake tokens? That way,
FakeOAuth2TokenService consumers don't need to be aware of the presence of the
delegate, and in fact we may change the interface between OAuth2TokenService and
OAuth2TokenServiceDelegate with less friction (i.e. tests to update) in the
future.

> 
> If you feel strongly about this, I could open a bug to track how to better
> manage the token service delegates as a follow up CL.  This CL is already
large
> enough.  Is that OK?

I trust you to make a good decision on this architectural question as you know
this code much better than I do. I was just trying to point out what looked like
an architectural flaw from a perspective of OAuth2TokenService consumers. Please
don't consider this comment thread informative, i.e. I'm not meaning to hold the
CL over it.

[Mattias Nissler (ping if slow), 2015-07-01 12:34:14]

DeviceOAuth2TokenService looks much nicer now, thanks for cleaning that up! I
think the structure is good, merely requesting a bunch of renames that clarify
intended semantics.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:64: // Implement
DeviceOAuth2TokenServiceDelegate::ServiceErrorStatusObserver
nit: Line 52 uses "Implementation of XYZ." for this type of comment. The style
you're using here is rather rare AFAICT and missing a terminating period.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
File chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc
(right):

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:69: if
(service_error_status_observer_)
nit: add curly braces for multi-line conditional statement.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:134: if
(service_error_status_observer_)
nit: braces

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:145: if
(service_error_status_observer_)
nit: braces

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:250: if
(service_error_status_observer_)
nit: braces

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:259: if
(service_error_status_observer_)
nit: braces

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:270: if
(service_error_status_observer_)
nit: braces

Given the frequency of this code block, it might be a good idea to introduce a
helper:

void DeviceOAuthTokenServiceDelegate::ReportServiceError(GoogleAuthServiceError
error) {
 if (service_error_status_observer_)
   service_error_status_observer_->OnServiceError(error);
}

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:324:
void DeviceOAuth2TokenServiceDelegate::HasPendingRequests(bool has) {
nit: This should be RequestValidation() and should just set
|validation_requested_| (which replaces |has_pending_requests_|) to true.
There's no good reason to ever switch back to false.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
File chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.h
(right):

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.h:47:
const StatusCallback& callback);
nit: blank line before comment

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.h:73:
class ServiceErrorStatusObserver {
This should be renamed to ValidationStatusDelegate (Observers are for 1-to-many
relationships)

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.h:75:
virtual void OnServiceError(GoogleServiceAuthError::State error) {}
And this to OnValidationCompleted

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
File chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc
(right):

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:135:
static_cast<DeviceOAuth2TokenServiceDelegate*>(
This is much cleaner if you just create the DeviceOAuth2TokenServiceDelegate
instance and hold on to it in a local variable, then set
max_refresh_token_validation_retries_ = 0, then pass the pointer on to the
DeviceOAuth2TokenService ctor.

[gogerald1, 2015-07-01 15:51:21]

Patchset #23 (id:1680001) has been deleted

[gogerald1, 2015-07-01 15:51:46]

Patchset #23 (id:1700001) has been deleted

[gogerald1, 2015-07-01 16:17:42]

Patchset #23 (id:1720001) has been deleted

[gogerald1, 2015-07-01 16:50:42]

Patchset #24 (id:1760001) has been deleted

[gogerald1, 2015-07-01 17:58:38]

gogerald@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[gogerald1, 2015-07-01 17:58:42]

Address previous comments,

mnissler@chromium.org: Please review changes in ChromeOS.

phajdan.jr@chromium.org: Please review changes in 
chrome/test/base/testing_profile.cc

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
google_apis/gaia/oauth2_token_service.h:133:
OAuth2TokenService(OAuth2TokenServiceDelegate* delegate);
On 2015/06/26 18:15:53, xiyuan wrote:
> nit: explicit and use scoped_ptr to make ownership transfer more clear,
> 
> i.e.
>   explicit OAuth2TokenService(scoped_ptr<OAuth2TokenServiceDelegate>
delegate);

Acknowledged.

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
google_apis/gaia/oauth2_token_service.h:182: void RevokeAllCredentials();
On 2015/06/29 08:20:06, Mattias Nissler wrote:
> Would be good to put a comment stating what this does. Drop all access tokens?
> Drop even refresh tokens?

Done.

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
google_apis/gaia/oauth2_token_service.h:207: OAuth2TokenServiceDelegate*
GetDelegate();
On 2015/06/30 20:42:51, Mattias Nissler wrote:
> On 2015/06/30 19:55:30, Roger Tawa wrote:
> > On 2015/06/29 09:01:29, Mattias Nissler wrote:
> > > On 2015/06/29 08:49:44, msarda wrote:
> > > > On 2015/06/29 08:20:06, Mattias Nissler wrote:
> > > > > From an architectural perspective, it seems wrong to have this
function
> in
> > > the
> > > > > public interface. The CL description suggests that the delegate makes
it
> > > > easier
> > > > > to implement the platform-specific logic for token service
> implementations
> > > > > separate from OAuth2TokenService. I thus expected that the delegate is
> > > merely
> > > > an
> > > > > implementation detail of O2TS (and subclasses) that consumers wouldn't
> > need
> > > to
> > > > > know about. However, this function means that all the complexity of
> having
> > > to
> > > > > deal with a delegate is exposed, and in fact some code does make calls
> > > > directly
> > > > > into the delegate.
> > > > > 
> > > > > Roger, what are your thoughts regarding this?
> > > > 
> > > > I agree that from architectural perspective having the delegate public
may
> > > look
> > > > strange. However, on iOS we have specific logic in PO2TS_IOS to reload
> > > > credentials. That logic is now being moved to the delegate. So if we do
> not
> > > want
> > > > any of that logic in the token service and we do not want to subclass
the
> > > PO2TS
> > > > any more, then we need this delegate to be public.
> > > 
> > > Who calls that logic to reload credentials? If that's an action which is
> > > relevant for consumers in general, it may make sense to lift the API into
> > PO2TS.
> > > If the call comes from IOS-specific code, it may make sense for that code
to
> > > hold on to the delegate pointer that it passed to PO2TS at construction
time
> > so
> > > it can make the call without relying on the getter here. I don't know your
> use
> > > case though, so these suggestions are merely a stab in the dark.
> > 
> > There are a few types of token services in chrome, depending on platform. 
On
> > desktop, the token service implementation does it all.  It mints and caches
> > access tokens, manages refresh tokens/accounts, and updates the access
tokens
> > from refresh tokens.
> > 
> > On mobile, the token service implementation is a thin wrapper around
> non-chrome
> > services.  In these cases, the token service mints and caches access tokens
> but
> > has no way to manipulate refresh tokens (i.e., add and/or remove accounts
from
> > the device).  The delegate updates access tokens when needed, as that is
> > platform specific.  Furthermore, there are other mobile specific things that
> > need to be done, and that code now belongs to the delegate as well.
> > 
> > Cros is slightly different.  The token service always has at least one
refresh
> > token matching the signed in user.  This token cannot be removed.  But when
> > account consistency is enabled, cros can manager secondary accounts.
> > 
> > This CL keeps the interface for requesting access tokens and the caching of
> > these tokens in the OAuth2TokenService class.  The delegate handles updating
> the
> > access tokens from the refresh tokens.  The management of refresh tokens is
> also
> > done by the delegate if needed.  There is no longer a need to have
> > OAuth2TokenService derivative classes per platform.
> 
> That's awesome, and I'm very happy to hear this. Does that mean the plan is to
> remove all OAuth2TokenService derivatives in follow-up CLs?
> 
> > 
> > An alternative could be to save the delegate pointer somewhere and then get
it
> > directly, as you suggest.  Maybe the delegate should even be its own PKS. 
But
> I
> > don't think it's a problem for the token service to have a public
> GetDelegate()
> > method.  The token service is always backed by some delegate.
> 
> As long as regular consumers (i.e. services that just request access tokens)
> don't have to be aware of the fact that there's a delegate, I'm fine with
> keeping the public GetDelegate() function. My main concern was to avoid making
> the OAuth2TokenService API more complex to use for consumers.
> 
> In that spirit, I suggest to simplify FakeOAuth2TokenService to provide
> functions that allow consumer to set up fake tokens? That way,
> FakeOAuth2TokenService consumers don't need to be aware of the presence of the
> delegate, and in fact we may change the interface between OAuth2TokenService
and
> OAuth2TokenServiceDelegate with less friction (i.e. tests to update) in the
> future.
> 
> > 
> > If you feel strongly about this, I could open a bug to track how to better
> > manage the token service delegates as a follow up CL.  This CL is already
> large
> > enough.  Is that OK?
> 
> I trust you to make a good decision on this architectural question as you know
> this code much better than I do. I was just trying to point out what looked
like
> an architectural flaw from a perspective of OAuth2TokenService consumers.
Please
> don't consider this comment thread informative, i.e. I'm not meaning to hold
the
> CL over it.
> 

Acknowledged.

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
File google_apis/gaia/oauth2_token_service_delegate.h (right):

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
google_apis/gaia/oauth2_token_service_delegate.h:19: //
ProfileOAuth2TokenService. It allows token to be fetched from other external
On 2015/06/29 08:20:07, Mattias Nissler wrote:
> nit: Why is this talking about *Profile*OAuth2TokenService(Delegate) and why
is
> the delegate an "alaternative" to the token service? IIUC, you still need both
> for the system to be functional.

Done.

https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
google_apis/gaia/oauth2_token_service_delegate.h:82: int batch_change_depth_;
On 2015/06/29 08:20:07, Mattias Nissler wrote:
> nit: DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/1143323005/diff/1380001/ios/public/provider/c...
File ios/public/provider/chrome/browser/keyed_service_provider.h (right):

https://codereview.chromium.org/1143323005/diff/1380001/ios/public/provider/c...
ios/public/provider/chrome/browser/keyed_service_provider.h:63: virtual
KeyedServiceBaseFactory* GetProfileOAuth2TokenServiceIOSFactory() = 0;
On 2015/06/29 11:55:53, droger wrote:
> Can we also rename the functions here (remove IOS):
> GetProfileOAuth2TokenServiceFactory()
> GetProfileOAuth2TokenServiceForBrowserState()

Done.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:64: // Implement
DeviceOAuth2TokenServiceDelegate::ServiceErrorStatusObserver
On 2015/07/01 12:34:13, Mattias Nissler wrote:
> nit: Line 52 uses "Implementation of XYZ." for this type of comment. The style
> you're using here is rather rare AFAICT and missing a terminating period.

Done.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
File chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc
(right):

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:69: if
(service_error_status_observer_)
On 2015/07/01 12:34:13, Mattias Nissler wrote:
> nit: add curly braces for multi-line conditional statement.

Done.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:134: if
(service_error_status_observer_)
On 2015/07/01 12:34:13, Mattias Nissler wrote:
> nit: braces

Done.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:145: if
(service_error_status_observer_)
On 2015/07/01 12:34:13, Mattias Nissler wrote:
> nit: braces

Done.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:250: if
(service_error_status_observer_)
On 2015/07/01 12:34:13, Mattias Nissler wrote:
> nit: braces

Done.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:259: if
(service_error_status_observer_)
On 2015/07/01 12:34:13, Mattias Nissler wrote:
> nit: braces

Done.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:270: if
(service_error_status_observer_)
On 2015/07/01 12:34:13, Mattias Nissler wrote:
> nit: braces
> 
> Given the frequency of this code block, it might be a good idea to introduce a
> helper:
> 
> void
DeviceOAuthTokenServiceDelegate::ReportServiceError(GoogleAuthServiceError
> error) {
>  if (service_error_status_observer_)
>    service_error_status_observer_->OnServiceError(error);
> }

Acknowledged. might be more efficient to inline short code.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:324:
void DeviceOAuth2TokenServiceDelegate::HasPendingRequests(bool has) {
On 2015/07/01 12:34:13, Mattias Nissler wrote:
> nit: This should be RequestValidation() and should just set
> |validation_requested_| (which replaces |has_pending_requests_|) to true.
> There's no good reason to ever switch back to false.

Done.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
File chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.h
(right):

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.h:47:
const StatusCallback& callback);
On 2015/07/01 12:34:13, Mattias Nissler wrote:
> nit: blank line before comment

Done.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.h:73:
class ServiceErrorStatusObserver {
On 2015/07/01 12:34:14, Mattias Nissler wrote:
> This should be renamed to ValidationStatusDelegate (Observers are for
1-to-many
> relationships)

Done.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.h:75:
virtual void OnServiceError(GoogleServiceAuthError::State error) {}
On 2015/07/01 12:34:14, Mattias Nissler wrote:
> And this to OnValidationCompleted

Done.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
File chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc
(right):

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:135:
static_cast<DeviceOAuth2TokenServiceDelegate*>(
On 2015/07/01 12:34:14, Mattias Nissler wrote:
> This is much cleaner if you just create the DeviceOAuth2TokenServiceDelegate
> instance and hold on to it in a local variable, then set
> max_refresh_token_validation_retries_ = 0, then pass the pointer on to the
> DeviceOAuth2TokenService ctor.

Done.

[Mattias Nissler (ping if slow), 2015-07-01 18:50:04]

policy and device_oauth2_token_service stuff LGTM.

Decision on whether to make the helper or not is up to you, I don't feel
strongly.

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
File chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc
(right):

https://codereview.chromium.org/1143323005/diff/1640001/chrome/browser/chrome...
chrome/browser/chromeos/settings/device_oauth2_token_service_delegate.cc:270: if
(service_error_status_observer_)
On 2015/07/01 17:58:41, gogerald1 wrote:
> On 2015/07/01 12:34:13, Mattias Nissler wrote:
> > nit: braces
> > 
> > Given the frequency of this code block, it might be a good idea to introduce
a
> > helper:
> > 
> > void
> DeviceOAuthTokenServiceDelegate::ReportServiceError(GoogleAuthServiceError
> > error) {
> >  if (service_error_status_observer_)
> >    service_error_status_observer_->OnServiceError(error);
> > }
> 
> Acknowledged. might be more efficient to inline short code.

I don't think efficiency is of any concern in this particular case. Given that,
readability is key here IMHO.

[Marijn Kruisselbrink, 2015-07-01 21:12:54]

extensions changes lgtm

[Roger Tawa OOO till Jul 10th, 2015-07-02 14:01:04]

On 2015/06/30 20:42:52, Mattias Nissler wrote:
> Here's a reply to the discussion regarding GetDelegate() being public. I will
> take a closer look at the DeviceOAuth2TokenService changes tomorrow (it's 11pm
> here and my brain isn't up to that task right now).
> 
>
https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
> File google_apis/gaia/oauth2_token_service.h (right):
> 
>
https://codereview.chromium.org/1143323005/diff/1380001/google_apis/gaia/oaut...
> google_apis/gaia/oauth2_token_service.h:207: OAuth2TokenServiceDelegate*
> GetDelegate();
> On 2015/06/30 19:55:30, Roger Tawa wrote:
> > On 2015/06/29 09:01:29, Mattias Nissler wrote:
> > > On 2015/06/29 08:49:44, msarda wrote:
> > > > On 2015/06/29 08:20:06, Mattias Nissler wrote:
> > > > > From an architectural perspective, it seems wrong to have this
function
> in
> > > the
> > > > > public interface. The CL description suggests that the delegate makes
it
> > > > easier
> > > > > to implement the platform-specific logic for token service
> implementations
> > > > > separate from OAuth2TokenService. I thus expected that the delegate is
> > > merely
> > > > an
> > > > > implementation detail of O2TS (and subclasses) that consumers wouldn't
> > need
> > > to
> > > > > know about. However, this function means that all the complexity of
> having
> > > to
> > > > > deal with a delegate is exposed, and in fact some code does make calls
> > > > directly
> > > > > into the delegate.
> > > > > 
> > > > > Roger, what are your thoughts regarding this?
> > > > 
> > > > I agree that from architectural perspective having the delegate public
may
> > > look
> > > > strange. However, on iOS we have specific logic in PO2TS_IOS to reload
> > > > credentials. That logic is now being moved to the delegate. So if we do
> not
> > > want
> > > > any of that logic in the token service and we do not want to subclass
the
> > > PO2TS
> > > > any more, then we need this delegate to be public.
> > > 
> > > Who calls that logic to reload credentials? If that's an action which is
> > > relevant for consumers in general, it may make sense to lift the API into
> > PO2TS.
> > > If the call comes from IOS-specific code, it may make sense for that code
to
> > > hold on to the delegate pointer that it passed to PO2TS at construction
time
> > so
> > > it can make the call without relying on the getter here. I don't know your
> use
> > > case though, so these suggestions are merely a stab in the dark.
> > 
> > There are a few types of token services in chrome, depending on platform. 
On
> > desktop, the token service implementation does it all.  It mints and caches
> > access tokens, manages refresh tokens/accounts, and updates the access
tokens
> > from refresh tokens.
> > 
> > On mobile, the token service implementation is a thin wrapper around
> non-chrome
> > services.  In these cases, the token service mints and caches access tokens
> but
> > has no way to manipulate refresh tokens (i.e., add and/or remove accounts
from
> > the device).  The delegate updates access tokens when needed, as that is
> > platform specific.  Furthermore, there are other mobile specific things that
> > need to be done, and that code now belongs to the delegate as well.
> > 
> > Cros is slightly different.  The token service always has at least one
refresh
> > token matching the signed in user.  This token cannot be removed.  But when
> > account consistency is enabled, cros can manager secondary accounts.
> > 
> > This CL keeps the interface for requesting access tokens and the caching of
> > these tokens in the OAuth2TokenService class.  The delegate handles updating
> the
> > access tokens from the refresh tokens.  The management of refresh tokens is
> also
> > done by the delegate if needed.  There is no longer a need to have
> > OAuth2TokenService derivative classes per platform.
> 
> That's awesome, and I'm very happy to hear this. Does that mean the plan is to
> remove all OAuth2TokenService derivatives in follow-up CLs?
> 
> > 
> > An alternative could be to save the delegate pointer somewhere and then get
it
> > directly, as you suggest.  Maybe the delegate should even be its own PKS. 
But
> I
> > don't think it's a problem for the token service to have a public
> GetDelegate()
> > method.  The token service is always backed by some delegate.
> 
> As long as regular consumers (i.e. services that just request access tokens)
> don't have to be aware of the fact that there's a delegate, I'm fine with
> keeping the public GetDelegate() function. My main concern was to avoid making
> the OAuth2TokenService API more complex to use for consumers.
> 
> In that spirit, I suggest to simplify FakeOAuth2TokenService to provide
> functions that allow consumer to set up fake tokens? That way,
> FakeOAuth2TokenService consumers don't need to be aware of the presence of the
> delegate, and in fact we may change the interface between OAuth2TokenService
and
> OAuth2TokenServiceDelegate with less friction (i.e. tests to update) in the
> future.
> 
> > 
> > If you feel strongly about this, I could open a bug to track how to better
> > manage the token service delegates as a follow up CL.  This CL is already
> large
> > enough.  Is that OK?
> 
> I trust you to make a good decision on this architectural question as you know
> this code much better than I do. I was just trying to point out what looked
like
> an architectural flaw from a perspective of OAuth2TokenService consumers.
Please
> don't consider this comment thread informative, i.e. I'm not meaning to hold
the
> CL over it.

Hi Mattias,

Regular consumers that just want access tokens don't need the know about the
delegate.  Only special, sign in related classes like SigninManager need to
worry about that.

Wrt getting rid of OAuth2TokenService derivatives, I should have been a little
clearer.  Ganggui said he would look at getting rid of the iOS one in this CL. 
With this change, the O2TS defines the access token API and how they are cached,
whereas the delegate defines refresh token management and updating access tokens
from them.  Some tests modify the access token caching, and in particular
DeviceOAuth2TokenService modifies the access tokens fetch algorithm.  So these
derivatives may need to continue to exist.

[gogerald1, 2015-07-02 14:16:24]

gogerald@chromium.org changed reviewers:
+ nyquist@chromium.org

[gogerald1, 2015-07-02 14:16:27]

Hi, 

nyquist@ please help review changes in 
chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java
chrome/browser/android/chrome_jni_registrar.cc
chrome/browser/android/signin/signin_manager_android.cc

[Paweł Hajdan Jr., 2015-07-02 15:34:56]

chrome/test LGTM

[nyquist, 2015-07-02 18:33:08]

chrome/android/java and chrome/browser/android lgtm

However, I think it would be beneficial to update the commit message for this
CL.

- Change to just BUG=490882
- Add a single header line: "Refactor AO2TS to make it easier to componentize."
at the top of the comment, then a newline.

https://codereview.chromium.org/1143323005/diff/1780001/chrome/browser/androi...
File chrome/browser/android/signin/signin_manager_android.cc (right):

https://codereview.chromium.org/1143323005/diff/1780001/chrome/browser/androi...
chrome/browser/android/signin/signin_manager_android.cc:268: // New Mirror code
path that just fires the events and let the
I thought usages of code names were discouraged? Could this comment be
simplified to something like:
// Let account Reconcilor handle everyhing.
?

[gogerald1, 2015-07-02 19:57:29]

The CQ bit was checked by gogerald@chromium.org

[gogerald1, 2015-07-02 19:57:30]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
msarda@chromium.org, rogerta@chromium.org, pavely@chromium.org,
msw@chromium.org, caitkp@chromium.org, treib@chromium.org, zea@chromium.org,
vitalybuka@chromium.org, mnissler@chromium.org, nyquist@chromium.org,
phajdan.jr@chromium.org, mek@chromium.org
Link to the patchset: https://codereview.chromium.org/1143323005/#ps1800001
(title: "Address final comments")

[commit-bot: I haz the power, 2015-07-02 19:57:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1143323005/1800001

[commit-bot: I haz the power, 2015-07-02 20:04:06]

Committed patchset #25 (id:1800001)

[commit-bot: I haz the power, 2015-07-02 20:05:01]

Patchset 25 (id:??) landed as
https://crrev.com/63376b55f05201ea493fe6c26d3b9a8f26bc2f16
Cr-Commit-Position: refs/heads/master@{#337248}

[gogerald1, 2015-08-13 20:49:06]

Patchset #26 (id:1820001) has been deleted