Add service flags to the AccountTrackerService.

Add service flags to the AccountTrackerService.

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

[anthonyvd, 2015-05-06 20:02:41]

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

[anthonyvd, 2015-05-06 20:02:42]

Hi Roger,

can you please take a look at this change that adds the Account Service Flags to
the Account Info in the Account Tracker Service?

Thanks a lot :)

[Roger Tawa OOO till Jul 10th, 2015-05-11 20:03:50]

lgtm, one nit below.

(In the case where one request fails but the other does not, it feels like we
should try to use at least what succeeded.  But I'm not sure how likely it will
be for only one to fail.  Also, once we move the the new endpoint, this will
become one fetch again, so it will just completely succeed or completely fail.)

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

https://codereview.chromium.org/1130853003/diff/20001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:586: while
(service_flags_list->GetString(index++, &flag)) {
Would be better to use for (...) loop over the service_flags_list.  Should use
pre-increment.

[anthonyvd, 2015-05-14 17:44:58]

On 2015/05/11 20:03:50, Roger Tawa wrote:
> lgtm, one nit below.
> 
> (In the case where one request fails but the other does not, it feels like we
> should try to use at least what succeeded.  But I'm not sure how likely it
will
> be for only one to fail.  Also, once we move the the new endpoint, this will
> become one fetch again, so it will just completely succeed or completely
fail.)
> 

Since service_flags aren't mandatory in the AccountInfo (maybe they should be),
the result of fetching the rest of the data would be presented as valid to the
callers. I decided to present the fetch as a single success/failure operation
because since that's what it's going to be in the future I didn't want callers
to expect only one part to fail and build around that.

>
https://codereview.chromium.org/1130853003/diff/20001/components/signin/core/...
> File components/signin/core/browser/account_tracker_service.cc (right):
> 
>
https://codereview.chromium.org/1130853003/diff/20001/components/signin/core/...
> components/signin/core/browser/account_tracker_service.cc:586: while
> (service_flags_list->GetString(index++, &flag)) {
> Would be better to use for (...) loop over the service_flags_list.  Should use
> pre-increment.

[anthonyvd, 2015-05-14 17:45:04]

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

https://codereview.chromium.org/1130853003/diff/20001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:586: while
(service_flags_list->GetString(index++, &flag)) {
On 2015/05/11 20:03:50, Roger Tawa wrote:
> Would be better to use for (...) loop over the service_flags_list.  Should use
> pre-increment.

Done.

[anthonyvd, 2015-05-15 13:34:12]

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

[anthonyvd, 2015-05-15 13:34:12]

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/1130853003/#ps40001
(title: "Address review feedback.")

[commit-bot: I haz the power, 2015-05-15 13:34:23]

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

[commit-bot: I haz the power, 2015-05-15 14:15:46]

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

[commit-bot: I haz the power, 2015-05-15 14:15:47]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2015-05-15 14:56:09]

The CQ bit was checked by anthonyvd@chromium.org

[commit-bot: I haz the power, 2015-05-15 14:56:20]

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

[commit-bot: I haz the power, 2015-05-15 15:00:18]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-15 15:01:18]

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

[brucedawson, 2015-05-18 23:43:13]

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

[brucedawson, 2015-05-18 23:43:14]

There is an extraneous line of code that should be deleted, and a space that
should perhaps be added.

https://codereview.chromium.org/1130853003/diff/40001/components/signin/core/...
File components/signin/core/browser/account_tracker_service.cc (right):

https://codereview.chromium.org/1130853003/diff/40001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:584: std::string flag;
This variable is never used. It should be deleted to avoid confusion.

https://codereview.chromium.org/1130853003/diff/40001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:585: for(base::Value*
flag: *service_flags_list) {
The Google C++ style guide recommends "for (base::Value" rather than
"for(base::Value". That is, there should be a space after the keyword 'for'.
While this file does use the no-space convention in a few other places, it uses
the convention with a space far more frequently.

Also, this 'flag' variable shadows the 'flag' variable on the previous line,
which is why I ended up looking at this code at all. Deleting the previous
declaration will solve that.