GaiaCookieServiceManager handles general request types

GaiaCookieServiceManager handles general request types. This is a prerequisite to the GCMS being able to handle ListAccounts requests as part if its queue. Remove the unused single-account LogOut() method, and rework LogOutInternal() method.

Still to do, in future CLs:
- Make the GaiaCookieManagerService independently check for freshness of ExternalCCConnections before executing any MergeSession (every 3 hours?)
- Move all /ListAccounts calls into the GaiaCookieManagerService, and have it publish to all observers when that fails/succeeds
- Move all logic that listens to cookie changes into GCMS, publish a notification when change happens
- Rename the MergeSessionComplete method of the signin_tracker
- Add retry (and backoff) logic for AddAccount calls
- Remove CrOS's direct calls to gaia_auth_fetcher->StartMergeSession

BUG=466799
Committed: https://crrev.com/c9f6f551ae9086ce65fb1ad04cc030c2e540e6ba
Cr-Commit-Position: refs/heads/master@{#324218}

[Mike Lerman, 2015-03-30 15:16:36]

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

[Mike Lerman, 2015-03-30 15:16:37]

Hi Roger,

Smaller CL this time :) This paves the way for moving ListAccounts to move into
this class and for this class to start listening to the cookie service. That
next CL will also have more complex caching and queue-ing logic, that
incorporates ListAccounts.

[Roger Tawa OOO till Jul 10th, 2015-03-30 18:57:23]

Smaller CL, but possible more comments :-)  Please see below.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
File components/signin/core/browser/gaia_cookie_manager_service.cc (right):

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:250: const
ListAccountsCallback& callback) {
|callback| is never used.  Should this function have a NOTREACHED() macro until
it's fully implemented?

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:260: }
Don't need { }.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:262: 
nit: remove blank line.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:280: it !=
requests_.end(); it++) {
use ++it

Should fit one line?

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:288: //
ADD_ACCOUNT requests should be removed.
Comment does not seem to match the code, but I think it's a moot point, see
comment at line 305.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:305: }
I don't understand this if block.  The new code for LogOutAllAccounts() should
behave like the old code for the case LogOutInternal("",
std::vector<std::string>()).  This means that:

1/ all pending ADD_ACCOUNTs should never be re-added

2/ if we get to line 301, only want to keep the currently pending request, so
lines 301-305 should be replaced with |requests_.resize(1)|.

You don't need the local var |requests_to_keep|.

Please keep the comment at old line 249 and put right above line 301.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:312:
StartLogOutUrlFetch();
If a logout was already pending, then you get here and with a |request| size of
1.  This will start a new fetch when not needed.  You need to move line 312 into
the if block at line 309.  No need to check |request.size()|.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:313: }
I'm not sure this passes the unit tests, did you run them?

I think you should consider rewriting LogOutAllAccounts() by starting with the
existing LogOutInternal().  Make the assumption that |account_id| and |accounts|
are always empty and remove dead code.  Then adapt it from a list of strings to
a list of GaiaCookieRequests.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
File components/signin/core/browser/gaia_cookie_manager_service.h (right):

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.h:18: class
GaiaCookieRequest;
What is this forward declare for?

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.h:67:
list_accounts_callback);
Should these be called CreateXXX() ?

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
File components/signin/core/browser/gaia_cookie_manager_service_unittest.cc
(right):

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service_unittest.cc:241:
TEST_F(GaiaCookieManagerServiceTest, LogOut) {
Rename test to LogOutAllAccounts.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service_unittest.cc:256: }
You should write tests to cover more cases:

1/ call LogOutAllAccounts() when no requests queued.
2/ call LogOutAllAccounts() when one add account request queued.
3/ call LogOutAllAccounts() when multiple add account requests queued.
4/ call LogOutAllAccounts() when list account request queued.
5/ call LogOutAllAccounts() when logout request queued.
6/ call LogOutAllAccounts() when logout request queued followed by queued add
account requests.

[Mike Lerman, 2015-04-01 16:39:35]

Hi Roger,

Tests written, comments addressed :)

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
File components/signin/core/browser/gaia_cookie_manager_service.cc (right):

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:250: const
ListAccountsCallback& callback) {
On 2015/03/30 18:57:22, Roger Tawa wrote:
> |callback| is never used.  Should this function have a NOTREACHED() macro
until
> it's fully implemented?

Sure, will do. Good call.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:260: }
On 2015/03/30 18:57:22, Roger Tawa wrote:
> Don't need { }.

Done.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:262: 
On 2015/03/30 18:57:23, Roger Tawa wrote:
> nit: remove blank line.

Done.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:280: it !=
requests_.end(); it++) {
On 2015/03/30 18:57:23, Roger Tawa wrote:
> use ++it
> 
> Should fit one line?

Ah yes. I didn't used to have auto, and it had needed 2 lines.
Thanks.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:288: //
ADD_ACCOUNT requests should be removed.
On 2015/03/30 18:57:23, Roger Tawa wrote:
> Comment does not seem to match the code, but I think it's a moot point, see
> comment at line 305.

Done.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:305: }
On 2015/03/30 18:57:23, Roger Tawa wrote:
> I don't understand this if block.  The new code for LogOutAllAccounts() should
> behave like the old code for the case LogOutInternal("",
> std::vector<std::string>()).  This means that:
> 
> 1/ all pending ADD_ACCOUNTs should never be re-added
> 
> 2/ if we get to line 301, only want to keep the currently pending request, so
> lines 301-305 should be replaced with |requests_.resize(1)|.
> 
> You don't need the local var |requests_to_keep|.
> 
> Please keep the comment at old line 249 and put right above line 301.

We discussed offline.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:312:
StartLogOutUrlFetch();
On 2015/03/30 18:57:23, Roger Tawa wrote:
> If a logout was already pending, then you get here and with a |request| size
of
> 1.  This will start a new fetch when not needed.  You need to move line 312
into
> the if block at line 309.  No need to check |request.size()|.

Good point. Done.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.cc:313: }
On 2015/03/30 18:57:23, Roger Tawa wrote:
> I'm not sure this passes the unit tests, did you run them?
> 
> I think you should consider rewriting LogOutAllAccounts() by starting with the
> existing LogOutInternal().  Make the assumption that |account_id| and
|accounts|
> are always empty and remove dead code.  Then adapt it from a list of strings
to
> a list of GaiaCookieRequests.

I did ;) And I'll write more as you suggested.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
File components/signin/core/browser/gaia_cookie_manager_service.h (right):

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.h:18: class
GaiaCookieRequest;
On 2015/03/30 18:57:23, Roger Tawa wrote:
> What is this forward declare for?

For... no good reason.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service.h:67:
list_accounts_callback);
On 2015/03/30 18:57:23, Roger Tawa wrote:
> Should these be called CreateXXX() ?

Sure.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
File components/signin/core/browser/gaia_cookie_manager_service_unittest.cc
(right):

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service_unittest.cc:241:
TEST_F(GaiaCookieManagerServiceTest, LogOut) {
On 2015/03/30 18:57:23, Roger Tawa wrote:
> Rename test to LogOutAllAccounts.

Done.

https://codereview.chromium.org/1044933002/diff/1/components/signin/core/brow...
components/signin/core/browser/gaia_cookie_manager_service_unittest.cc:256: }
On 2015/03/30 18:57:23, Roger Tawa wrote:
> You should write tests to cover more cases:
> 
> 1/ call LogOutAllAccounts() when no requests queued.
> 2/ call LogOutAllAccounts() when one add account request queued.
> 3/ call LogOutAllAccounts() when multiple add account requests queued.
> 4/ call LogOutAllAccounts() when list account request queued.
> 5/ call LogOutAllAccounts() when logout request queued.
> 6/ call LogOutAllAccounts() when logout request queued followed by queued add
> account requests.

I'll do the ListAccounts part once it's implemented. Right now it would hit a
NOTREACHED(). All others done.

[Roger Tawa OOO till Jul 10th, 2015-04-01 16:50:38]

lgtm

Thanks for the clarification Mike, I did misunderstand some of the code.

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

https://codereview.chromium.org/1044933002/diff/20001/components/signin/core/...
components/signin/core/browser/gaia_cookie_manager_service.cc:300: // Remove all
put the executing request. Re-add all requests being kept.
put --> but

[Mike Lerman, 2015-04-02 12:43:15]

Thanks Roger.

I'm going to wait until after branch point to land this.

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

https://codereview.chromium.org/1044933002/diff/20001/components/signin/core/...
components/signin/core/browser/gaia_cookie_manager_service.cc:300: // Remove all
put the executing request. Re-add all requests being kept.
On 2015/04/01 16:50:38, Roger Tawa wrote:
> put --> but

Done.

[Mike Lerman, 2015-04-07 12:31:00]

The CQ bit was checked by mlerman@chromium.org

[Mike Lerman, 2015-04-07 12:31:00]

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

[commit-bot: I haz the power, 2015-04-07 12:31:07]

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

[commit-bot: I haz the power, 2015-04-07 12:34:58]

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

[commit-bot: I haz the power, 2015-04-07 12:34:59]

Try jobs failed on following builders:
  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...)

[Mike Lerman, 2015-04-07 20:48:41]

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

[Mike Lerman, 2015-04-07 20:48:41]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/1044933002/#ps60001
(title: "Rebase (and fixes)")

[commit-bot: I haz the power, 2015-04-07 20:49:18]

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

[commit-bot: I haz the power, 2015-04-07 21:44:05]

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

[commit-bot: I haz the power, 2015-04-07 21:44:06]

This issue passed the CQ dry run.

[Mike Lerman, 2015-04-08 12:26:58]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2015-04-08 12:27:18]

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

[commit-bot: I haz the power, 2015-04-08 12:28:18]

Committed patchset #4 (id:60001)

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

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