iOS factories for SigninManager and GaiaCookieManagerService

iOS factories for SigninManager and GaiaCookieManagerService

BUG=332107
Committed: https://crrev.com/ab26ab7aca5323ba83757672cc3913c3c55b8038
Cr-Commit-Position: refs/heads/master@{#340333}

[droger, 2015-07-24 11:11:01]

droger@chromium.org changed reviewers:
+ sdefresne@chromium.org

[droger, 2015-07-24 11:11:02]

Note: this CL depends on https://codereview.chromium.org/1257623002/

[sdefresne, 2015-07-24 12:38:47]

https://codereview.chromium.org/1257663002/diff/20001/ios/chrome/browser/sign...
File ios/chrome/browser/signin/fake_signin_manager_builder.cc (right):

https://codereview.chromium.org/1257663002/diff/20001/ios/chrome/browser/sign...
ios/chrome/browser/signin/fake_signin_manager_builder.cc:21: SigninManager*
manager = new FakeSigninManager(
scoped_ptr<SigninManager> manager(new FakeSigninManager(

https://codereview.chromium.org/1257663002/diff/20001/ios/chrome/browser/sign...
ios/chrome/browser/signin/fake_signin_manager_builder.cc:31: return
make_scoped_ptr(manager);
return manager.Pass();

https://codereview.chromium.org/1257663002/diff/20001/ios/chrome/browser/sign...
File ios/chrome/browser/signin/signin_manager_factory.cc (right):

https://codereview.chromium.org/1257663002/diff/20001/ios/chrome/browser/sign...
ios/chrome/browser/signin/signin_manager_factory.cc:79: SigninManagerBase*
service = nullptr;
I think it would be better to use "scoped_ptr<SigninManager> service" and
"return service.Pass()" later.

https://codereview.chromium.org/1257663002/diff/20001/ios/chrome/browser/sign...
ios/chrome/browser/signin/signin_manager_factory.cc:82: SigninClient* client =
SigninClientFactory::GetInstance()->GetForBrowserState(
SigninClient::GetForBrowserState() is a static method, so no need to call
GetInstance() here.

https://codereview.chromium.org/1257663002/diff/20001/ios/chrome/browser/sign...
ios/chrome/browser/signin/signin_manager_factory.cc:85: client,
nit: why use a variable for SigninClient? why not directly:

service = new
SigninManager(SigninClientfactory::GetForBrowserState(chrome_browser_state), ...

https://codereview.chromium.org/1257663002/diff/20001/ios/chrome/browser/sign...
ios/chrome/browser/signin/signin_manager_factory.cc:97: SigninManagerBase*
manager = static_cast<SigninManagerBase*>(
Why mix SigninManagerBase and SigninManager in the same file? You only create
SigninManger, so cast to the most specific one.

https://codereview.chromium.org/1257663002/diff/20001/ios/chrome/browser/sign...
File ios/chrome/browser/signin/signin_manager_factory.h (right):

https://codereview.chromium.org/1257663002/diff/20001/ios/chrome/browser/sign...
ios/chrome/browser/signin/signin_manager_factory.h:28: class Observer {
Can you move out of the factory so that it can be forward-declared. Maybe rename
SigninManagerFactoryObserver.

https://codereview.chromium.org/1257663002/diff/20001/ios/chrome/browser/sign...
ios/chrome/browser/signin/signin_manager_factory.h:30: // Called when a
SigninManager(Base) instance is created.
Since you are writing this interface, why not directly pass the managed as the
correct type:

virtual void SigninManagerCreated(SigninManager* manager) {}
virtual void SigninManagerShutdown(SigninManager* manager) {}

https://codereview.chromium.org/1257663002/diff/20001/ios/chrome/browser/sign...
ios/chrome/browser/signin/signin_manager_factory.h:38: virtual ~Observer() {}
I prefer to have public destructors on Observers so that code can have
scoped_ptr<> to the base class in their public interface, and restrict the
visibility of the derived class name to the implementation file.

You also need to provide a constructor (will be required once you add the
DISALLOW_COPY_AND_ASSIGN). If you want to avoid creation of base Observers, mark
it as protected.

https://codereview.chromium.org/1257663002/diff/20001/ios/chrome/browser/sign...
ios/chrome/browser/signin/signin_manager_factory.h:39: };
DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/1257663002/diff/20001/ios/chrome/browser/sign...
ios/chrome/browser/signin/signin_manager_factory.h:45: //  static const
SigninManager* GetForBrowserStateIfExists(const
Please remove.

[sdefresne, 2015-07-24 13:10:02]

Please also remember to add the factories to
ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm

[droger, 2015-07-24 13:27:13]

Patchset #3 (id:40001) has been deleted

[droger, 2015-07-24 13:29:27]

Thanks for the review, all done.

[droger, 2015-07-24 13:33:15]

CC bzanotti FYI

[droger, 2015-07-24 13:33:40]

CC bzanotti FYI

[sdefresne, 2015-07-24 15:31:36]

lgtm, thank you for adding the missing factories

https://codereview.chromium.org/1257663002/diff/120001/ios/chrome/browser/sig...
File ios/chrome/browser/signin/fake_signin_manager_builder.cc (right):

https://codereview.chromium.org/1257663002/diff/120001/ios/chrome/browser/sig...
ios/chrome/browser/signin/fake_signin_manager_builder.cc:3: // found in the
LICENSE file.   3 // found in the LICENSE file.
huh? Can you remove the extra "   3 // found in the LICENSE file." at the end of
the line...

[droger, 2015-07-24 15:57:23]

https://codereview.chromium.org/1257663002/diff/120001/ios/chrome/browser/sig...
File ios/chrome/browser/signin/fake_signin_manager_builder.cc (right):

https://codereview.chromium.org/1257663002/diff/120001/ios/chrome/browser/sig...
ios/chrome/browser/signin/fake_signin_manager_builder.cc:3: // found in the
LICENSE file.   3 // found in the LICENSE file.
On 2015/07/24 15:31:36, sdefresne wrote:
> huh? Can you remove the extra "   3 // found in the LICENSE file." at the end
of
> the line...

Good catch, done.

[droger, 2015-07-24 20:21:11]

The CQ bit was checked by droger@chromium.org

[droger, 2015-07-24 20:21:12]

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/1257663002/#ps140001
(title: "Fix copy/paste error")

[commit-bot: I haz the power, 2015-07-24 20:22:06]

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

[commit-bot: I haz the power, 2015-07-24 20:32:29]

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

[commit-bot: I haz the power, 2015-07-24 20:32:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[droger, 2015-07-24 20:39:01]

The CQ bit was checked by droger@chromium.org

[droger, 2015-07-24 20:39:01]

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/1257663002/#ps160001
(title: "Fix copyright")

[commit-bot: I haz the power, 2015-07-24 20:40:22]

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

[commit-bot: I haz the power, 2015-07-24 20:59:06]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-07-24 21:00:21]

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