Add URL parameter so that /ListAccounts returns valid json.

Add URL parameter so that /ListAccounts returns valid json.
Check the valid_session bit to determine if the account in the cookie is valid.

BUG=305249
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252240

[Roger Tawa OOO till Jul 10th, 2014-02-14 22:09:18]

Hi Alan, Bartosz,

Please take a look at reconcilor changes.

Bartosz: this should be a noop from cros point of view.  Let me know if cros
cares about whether the session is valid in the cookie.

Thanks.

[bartfab (slow), 2014-02-17 15:22:04]

LGTM

The code that I wrote does not care whether an account is marked as valid or
not. I cannot speak for other existing Chrome OS code. However, given that no
check for account validity was being done so far, I guess it was not needed.

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/oauth2_login_manager.cc (right):

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/chromeos...
chrome/browser/chromeos/login/oauth2_login_manager.cc:295:
std::vector<std::pair<std::string, bool> > accounts;
Nit: #include <utility>

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/signin/a...
File chrome/browser/signin/account_reconcilor.cc (right):

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor.cc:372: for
(std::vector<std::pair<std::string, bool> >::const_iterator i =
Nit: AFAICT, we always call the iterator |it|, not |i|.

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/signin/a...
File chrome/browser/signin/account_reconcilor.h (right):

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor.h:59: const
std::vector<std::pair<std::string, bool> >&
Nit 1: #include <string>
Nit 2: #include <utility>
Nit 3: #include <vector>

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/signin/a...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor_unittest.cc:211: "[\"foo\", [[\"b\", 0,
\"n\", \"user@gmail.com\", \"p\", 0, 0, 0]]]",
Nit: Here and throughout the file: Since you are telling GAIA to return the
extended format with valid bit now, should the tests not simulate that extended
format as well?

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/authenticated_user_email_retriever.cc
(right):

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/authenticated_user_email_retriever.cc:31:
std::vector<std::pair<std::string, bool> > accounts;
Nit: #include <utility>

https://codereview.chromium.org/166433005/diff/420001/google_apis/gaia/gaia_a...
File google_apis/gaia/gaia_auth_util.cc (right):

https://codereview.chromium.org/166433005/diff/420001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_util.cc:107: // its invalid.  Otherwise assume its
valid to remain compatible with
Nit: s/its/it's/g

https://codereview.chromium.org/166433005/diff/420001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_util.cc:110: if (account->GetSize() > 9 &&
!account->GetInteger(9, &is_email_valid))
Nit: No need to check GetSize() here. If the size is <= 9, GetInteger() will
fail, causing you to set is_email_valid = 1, which is the value it had anyway
and the value you want.

https://codereview.chromium.org/166433005/diff/420001/google_apis/gaia/gaia_a...
File google_apis/gaia/gaia_auth_util.h (right):

https://codereview.chromium.org/166433005/diff/420001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_util.h:44: std::vector<std::pair<std::string, bool>
>* accounts);
Nit: #include <utility>

[Roger Tawa OOO till Jul 10th, 2014-02-18 19:37:54]

Thanks Batosz.  Comments addressed, changed uploaded.  Over to you Alan.

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/oauth2_login_manager.cc (right):

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/chromeos...
chrome/browser/chromeos/login/oauth2_login_manager.cc:295:
std::vector<std::pair<std::string, bool> > accounts;
On 2014/02/17 15:22:04, bartfab wrote:
> Nit: #include <utility>

Done.

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/signin/a...
File chrome/browser/signin/account_reconcilor.cc (right):

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor.cc:372: for
(std::vector<std::pair<std::string, bool> >::const_iterator i =
On 2014/02/17 15:22:04, bartfab wrote:
> Nit: AFAICT, we always call the iterator |it|, not |i|.

I've seen i, it, iter.  I think I'll leave it as is here since other parts of
this file use |i| for iterator.

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/signin/a...
File chrome/browser/signin/account_reconcilor.h (right):

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor.h:59: const
std::vector<std::pair<std::string, bool> >&
On 2014/02/17 15:22:04, bartfab wrote:
> Nit 1: #include <string>
> Nit 2: #include <utility>
> Nit 3: #include <vector>

Done.

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/signin/a...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor_unittest.cc:211: "[\"foo\", [[\"b\", 0,
\"n\", \"user@gmail.com\", \"p\", 0, 0, 0]]]",
On 2014/02/17 15:22:04, bartfab wrote:
> Nit: Here and throughout the file: Since you are telling GAIA to return the
> extended format with valid bit now, should the tests not simulate that
extended
> format as well?

Yup, makes sense.

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/authenticated_user_email_retriever.cc
(right):

https://codereview.chromium.org/166433005/diff/420001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/authenticated_user_email_retriever.cc:31:
std::vector<std::pair<std::string, bool> > accounts;
On 2014/02/17 15:22:04, bartfab wrote:
> Nit: #include <utility>

Done.

https://codereview.chromium.org/166433005/diff/420001/google_apis/gaia/gaia_a...
File google_apis/gaia/gaia_auth_util.cc (right):

https://codereview.chromium.org/166433005/diff/420001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_util.cc:107: // its invalid.  Otherwise assume its
valid to remain compatible with
On 2014/02/17 15:22:04, bartfab wrote:
> Nit: s/its/it's/g

Done.

https://codereview.chromium.org/166433005/diff/420001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_util.cc:110: if (account->GetSize() > 9 &&
!account->GetInteger(9, &is_email_valid))
On 2014/02/17 15:22:04, bartfab wrote:
> Nit: No need to check GetSize() here. If the size is <= 9, GetInteger() will
> fail, causing you to set is_email_valid = 1, which is the value it had anyway
> and the value you want.

Done.

https://codereview.chromium.org/166433005/diff/420001/google_apis/gaia/gaia_a...
File google_apis/gaia/gaia_auth_util.h (right):

https://codereview.chromium.org/166433005/diff/420001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_util.h:44: std::vector<std::pair<std::string, bool>
>* accounts);
On 2014/02/17 15:22:04, bartfab wrote:
> Nit: #include <utility>

Done.

[acleung1, 2014-02-18 21:00:23]

LGTM. Just a minor comment.

https://codereview.chromium.org/166433005/diff/640001/chrome/browser/signin/a...
File chrome/browser/signin/account_reconcilor.h (right):

https://codereview.chromium.org/166433005/diff/640001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor.h:200: bool are_gaia_accounts_set_;
Can you document on what the bool represent here?

Better yet. This variables seems to only represent the last state of
ListAccount. Should we rename it or document it better?

[Roger Tawa OOO till Jul 10th, 2014-02-19 22:57:01]

Thanks Alan.  Fixed up comment.

https://codereview.chromium.org/166433005/diff/640001/chrome/browser/signin/a...
File chrome/browser/signin/account_reconcilor.h (right):

https://codereview.chromium.org/166433005/diff/640001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor.h:200: bool are_gaia_accounts_set_;
On 2014/02/18 21:00:24, acleung1 wrote:
> Can you document on what the bool represent here?
> 
> Better yet. This variables seems to only represent the last state of
> ListAccount. Should we rename it or document it better?

Added docs.

[Roger Tawa OOO till Jul 10th, 2014-02-19 22:57:12]

The CQ bit was checked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-02-19 22:57:25]

The CQ bit was unchecked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-02-19 22:58:03]

Hi Xiyuan,

Can you review the two chromeos files?  Thanks.

[xiyuan, 2014-02-19 23:16:18]

lgtm

[Roger Tawa OOO till Jul 10th, 2014-02-19 23:16:40]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-02-20 01:19:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/166433005/910001

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

Failed to trigger a try job on win_rel
HTTP Error 400: Bad Request

[Roger Tawa OOO till Jul 10th, 2014-02-20 03:58:40]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-02-20 05:08:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/166433005/1090001

[commit-bot: I haz the power, 2014-02-20 09:03:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/166433005/1090001

[commit-bot: I haz the power, 2014-02-20 12:27:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/166433005/1090001

[commit-bot: I haz the power, 2014-02-20 15:37:44]

Change committed as 252240