Introduce FakeChromeIdentity and FakeChromeIdentityService.

Introduce FakeChromeIdentity and FakeChromeIdentityService.

By using those two fakes, unit tests won't have to depends on the
fake of the downstream implementations. This gives a better abstraction
and removes one of the dependencies for upstreaming.

BUG=626233
Committed: https://crrev.com/ba5e2399f383c6ec18e90e741183bff56204d683
Cr-Commit-Position: refs/heads/master@{#404372}

[bzanotti, 2016-07-07 11:29:48]

bzanotti@chromium.org changed reviewers:
+ msarda@chromium.org, sdefresne@chromium.org

[bzanotti, 2016-07-07 11:29:49]

Mihai: Please take a look.

Sylvain: Please do an OWNERS review of the non-signin provider files.

[msarda, 2016-07-07 11:58:06]

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity.h (right):

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity.h:13: // Returns
a new fake ChromeIdentity based on |email|, |gaiaID| and |name|.
s/a new/an autoreleased

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.h
(right):

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.h:24:
static FakeChromeIdentityService* GetInstance();
When I see a static method name GetInstance, I think that this will return a
global instance of type FakeChromeIdentityService. This makes me think there is
a single instance of this variable for all the tests, which is bad.

I think it would be better to remove this method and make each test that needs
this method instantiate its own FakeChromeIdentityService.

If you prefer to keep it, please rename it to match the implementation (like
GetInstanceFromChromeProvider) and explain what it does in a comment.

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm
(right):

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm:40:
return [[obj userEmail] isEqualToString:userEmail];
In general, emails need to be sanitized and canonicalized before comparing them.

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm:79: if
(callback) {
I think we should use the Chromium style of posting task on the main loop (using
POST_TASK). Then the test can run the message loop using the same syntax as all
the rest of the Chromium code.

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm:80:
dispatch_async(dispatch_get_main_queue(), ^{
Why do we dispatch here. Wouldn't it be easier to just call the callback
synchronously?

[sdefresne, 2016-07-07 12:12:40]

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity.h (right):

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity.h:14: // The
hashedGaiaID will be derived from |name|.
nit: The hashed GaiaID

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity.mm (right):

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity.mm:19: return
[[[[self class] alloc] initWithEmail:email gaiaID:gaiaID name:name]
Do we subclass FakeChromeIdentify? If not, the [self class] can be replaced with
FakeChromeIdentity (I'd be tempted to say we can also replace it even if we do
not subclass it).

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm
(right):

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm:80:
dispatch_async(dispatch_get_main_queue(), ^{
On 2016/07/07 11:58:06, msarda wrote:
> Why do we dispatch here. Wouldn't it be easier to just call the callback
> synchronously?

+1, please add a comment explaining why if it cannot be synchronous.

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm:100: }
}  // namespace ios

and add a blank line before

[bzanotti, 2016-07-07 12:48:12]

PTAL

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity.h (right):

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity.h:13: // Returns
a new fake ChromeIdentity based on |email|, |gaiaID| and |name|.
On 2016/07/07 11:58:06, msarda wrote:
> s/a new/an autoreleased

Done.

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity.h:14: // The
hashedGaiaID will be derived from |name|.
On 2016/07/07 12:12:40, sdefresne wrote:
> nit: The hashed GaiaID

I was referring to the property |hashedGaiaID|. Updated to make this more clear.

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity.mm (right):

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity.mm:19: return
[[[[self class] alloc] initWithEmail:email gaiaID:gaiaID name:name]
On 2016/07/07 12:12:40, sdefresne wrote:
> Do we subclass FakeChromeIdentify? If not, the [self class] can be replaced
with
> FakeChromeIdentity (I'd be tempted to say we can also replace it even if we do
> not subclass it).

Done.

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.h
(right):

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.h:24:
static FakeChromeIdentityService* GetInstance();
On 2016/07/07 11:58:06, msarda wrote:
> When I see a static method name GetInstance, I think that this will return a
> global instance of type FakeChromeIdentityService. This makes me think there
is
> a single instance of this variable for all the tests, which is bad.
> 
> I think it would be better to remove this method and make each test that needs
> this method instantiate its own FakeChromeIdentityService.
> 
> If you prefer to keep it, please rename it to match the implementation (like
> GetInstanceFromChromeProvider) and explain what it does in a comment.

Renamed to GetInstanceFromChromeProvider. Instantiating there own
FakeChromeIdentityService is already possible and is *NOT* what should be done
generally.

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm
(right):

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm:40:
return [[obj userEmail] isEqualToString:userEmail];
On 2016/07/07 11:58:06, msarda wrote:
> In general, emails need to be sanitized and canonicalized before comparing
them.

Good point, but this was causing some dependencies pain.

I removed the method anyway because we don't want to fake this method, we
actually want to remove it altogether (getting an identity via an email should
go through the AccountTrackerService to get the gaia ID).

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm:79: if
(callback) {
On 2016/07/07 11:58:06, msarda wrote:
> I think we should use the Chromium style of posting task on the main loop
(using
> POST_TASK). Then the test can run the message loop using the same syntax as
all
> the rest of the Chromium code.

The RunLoop already support the iOS dispatch mechanism.

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm:80:
dispatch_async(dispatch_get_main_queue(), ^{
On 2016/07/07 12:12:40, sdefresne wrote:
> On 2016/07/07 11:58:06, msarda wrote:
> > Why do we dispatch here. Wouldn't it be easier to just call the callback
> > synchronously?
> 
> +1, please add a comment explaining why if it cannot be synchronous.

Added a comment here. Basically, ForgetIdentity is an asynchronous operation, so
I think it's better when testing to enforce this.

https://codereview.chromium.org/2123273003/diff/1/ios/public/provider/chrome/...
ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.mm:100: }
On 2016/07/07 12:12:40, sdefresne wrote:
> }  // namespace ios
> 
> and add a blank line before

Done.

[sdefresne, 2016-07-07 12:58:02]

lgtm

https://codereview.chromium.org/2123273003/diff/20001/ios/public/provider/chr...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity.h (right):

https://codereview.chromium.org/2123273003/diff/20001/ios/public/provider/chr...
ios/public/provider/chrome/browser/signin/fake_chrome_identity.h:14: // The
|hashedGaiaID| will be derived from |name|.
nit: can you say "|hashedGaiaID| property" then?

https://codereview.chromium.org/2123273003/diff/20001/ios/public/provider/chr...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity.mm (right):

https://codereview.chromium.org/2123273003/diff/20001/ios/public/provider/chr...
ios/public/provider/chrome/browser/signin/fake_chrome_identity.mm:33: [[NSString
stringWithFormat:@"%@_hashID", name] retain]);
nit: retain -> copy?

https://codereview.chromium.org/2123273003/diff/20001/ios/public/provider/chr...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.h
(right):

https://codereview.chromium.org/2123273003/diff/20001/ios/public/provider/chr...
ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.h:61: }
nit: }  //  namespace ios

+ blank line before

[bzanotti, 2016-07-07 13:09:32]

https://codereview.chromium.org/2123273003/diff/20001/ios/public/provider/chr...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity.h (right):

https://codereview.chromium.org/2123273003/diff/20001/ios/public/provider/chr...
ios/public/provider/chrome/browser/signin/fake_chrome_identity.h:14: // The
|hashedGaiaID| will be derived from |name|.
On 2016/07/07 12:58:02, sdefresne wrote:
> nit: can you say "|hashedGaiaID| property" then?

Done.

https://codereview.chromium.org/2123273003/diff/20001/ios/public/provider/chr...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity.mm (right):

https://codereview.chromium.org/2123273003/diff/20001/ios/public/provider/chr...
ios/public/provider/chrome/browser/signin/fake_chrome_identity.mm:33: [[NSString
stringWithFormat:@"%@_hashID", name] retain]);
On 2016/07/07 12:58:02, sdefresne wrote:
> nit: retain -> copy?

Why? This would be a useless copy as stringWithFormat: returns a new string just
used here, no?

https://codereview.chromium.org/2123273003/diff/20001/ios/public/provider/chr...
File ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.h
(right):

https://codereview.chromium.org/2123273003/diff/20001/ios/public/provider/chr...
ios/public/provider/chrome/browser/signin/fake_chrome_identity_service.h:61: }
On 2016/07/07 12:58:02, sdefresne wrote:
> nit: }  //  namespace ios
> 
> + blank line before

Done.

[msarda, 2016-07-07 13:28:31]

lgtm

[bzanotti, 2016-07-07 17:59:06]

The CQ bit was checked by bzanotti@chromium.org

[bzanotti, 2016-07-07 17:59:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2123273003/#ps40001
(title: "Nits")

[commit-bot: I haz the power, 2016-07-07 17:59:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 18:27:26]

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

[commit-bot: I haz the power, 2016-07-07 18:27:27]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[bzanotti, 2016-07-08 07:47:24]

The CQ bit was checked by bzanotti@chromium.org

[commit-bot: I haz the power, 2016-07-08 07:48:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bzanotti, 2016-07-08 07:58:34]

The CQ bit was unchecked by bzanotti@chromium.org

[bzanotti, 2016-07-08 08:00:03]

The CQ bit was checked by bzanotti@chromium.org

[bzanotti, 2016-07-08 08:00:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, msarda@chromium.org
Link to the patchset: https://codereview.chromium.org/2123273003/#ps60001
(title: "Add missing dependency on gmock")

[commit-bot: I haz the power, 2016-07-08 08:00:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 14:23:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-08 14:23:49]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 14:25:58]

Description was changed from

==========
Introduce FakeChromeIdentity and FakeChromeIdentityService.

By using those two fakes, unit tests won't have to depends on the
fake of the downstream implementations. This gives a better abstraction
and removes one of the dependencies for upstreaming.

BUG=626233
==========

to

==========
Introduce FakeChromeIdentity and FakeChromeIdentityService.

By using those two fakes, unit tests won't have to depends on the
fake of the downstream implementations. This gives a better abstraction
and removes one of the dependencies for upstreaming.

BUG=626233

Committed: https://crrev.com/ba5e2399f383c6ec18e90e741183bff56204d683
Cr-Commit-Position: refs/heads/master@{#404372}
==========

[commit-bot: I haz the power, 2016-07-08 14:25:59]

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