Show GAIA/cookie accounts on the signin-internals page.

Show GAIA/cookie accounts on the signin-internals page.

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

[Mike Lerman, 2014-08-05 20:47:19]

Hi Roger,

Can I get an initial review on this? I'll start working on feedback as I wait
for nickk. Thanks!

Mike

[Roger Tawa OOO till Jul 10th, 2014-08-06 20:29:42]

lgtm

https://codereview.chromium.org/421583002/diff/40001/chrome/browser/signin/ch...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/421583002/diff/40001/chrome/browser/signin/ch...
chrome/browser/signin/chrome_signin_client.cc:202: DCHECK(!callbacks_.empty());
Maybe don't need this DCHECK anymore?

https://codereview.chromium.org/421583002/diff/40001/chrome/browser/signin/ch...
File chrome/browser/signin/chrome_signin_client.h (right):

https://codereview.chromium.org/421583002/diff/40001/chrome/browser/signin/ch...
chrome/browser/signin/chrome_signin_client.h:78: base::CallbackList<void(const
net::CanonicalCookie* cookie)> callbacks_;
Should this be:  base::CallbackList<CookieChangedCallback>

https://codereview.chromium.org/421583002/diff/40001/components/signin/core/b...
File components/signin/core/browser/about_signin_internals.h (right):

https://codereview.chromium.org/421583002/diff/40001/components/signin/core/b...
components/signin/core/browser/about_signin_internals.h:149: bool removed_;
Don't need trailing _ for struct.  Or put it on all members.

[Mike Lerman, 2014-08-07 16:39:26]

Thanks, Roger.

Evan, could I get a review on the rest, c/b/resources and c/b/ui/webui?

Thanks!

https://codereview.chromium.org/421583002/diff/40001/chrome/browser/signin/ch...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/421583002/diff/40001/chrome/browser/signin/ch...
chrome/browser/signin/chrome_signin_client.cc:202: DCHECK(!callbacks_.empty());
On 2014/08/06 20:29:41, Roger Tawa (build sheriff) wrote:
> Maybe don't need this DCHECK anymore?

It still seems like valid for sanity - we should never be set to observe unless
we've added at least one registration and callback.

https://codereview.chromium.org/421583002/diff/40001/chrome/browser/signin/ch...
File chrome/browser/signin/chrome_signin_client.h (right):

https://codereview.chromium.org/421583002/diff/40001/chrome/browser/signin/ch...
chrome/browser/signin/chrome_signin_client.h:78: base::CallbackList<void(const
net::CanonicalCookie* cookie)> callbacks_;
On 2014/08/06 20:29:42, Roger Tawa (build sheriff) wrote:
> Should this be:  base::CallbackList<CookieChangedCallback>

The type for the CallbackList has to be the function signature, not a
Base::Callback type.

https://codereview.chromium.org/421583002/diff/40001/components/signin/core/b...
File components/signin/core/browser/about_signin_internals.h (right):

https://codereview.chromium.org/421583002/diff/40001/components/signin/core/b...
components/signin/core/browser/about_signin_internals.h:149: bool removed_;
On 2014/08/06 20:29:42, Roger Tawa (build sheriff) wrote:
> Don't need trailing _ for struct.  Or put it on all members.

Removed the whole struct, not even using it!

[Evan Stade, 2014-08-08 19:33:54]

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/signin_internals/signin_index.css (right):

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/resources...
chrome/browser/resources/signin_internals/signin_index.css:33: div#token-info
div#cookie-info {
this selector doesn't make much sense. I think you're missing a comma.

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/signin_internals/signin_internals.js (right):

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/resources...
chrome/browser/resources/signin_internals/signin_internals.js:177: var
cookieInfoDiv = $('cookie-info');
nit: inline

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/resources...
chrome/browser/resources/signin_internals/signin_internals.js:189:
chrome.signin.onCookieAccountsFetched.addListener(function(info) {
why not:
chrome.signin.onCookieAccountsFetched.addListener(updateCookieAccounts);

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin_internals_ui.cc (right):

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin_internals_ui.cc:92:
"chrome.signin.onCookieAccountsFetched.fire";
nit: just inline the string literal (here and 2 places above)

[Mike Lerman, 2014-08-08 20:02:04]

Thanks Evan!

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/signin_internals/signin_index.css (right):

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/resources...
chrome/browser/resources/signin_internals/signin_index.css:33: div#token-info
div#cookie-info {
On 2014/08/08 19:33:53, Evan Stade wrote:
> this selector doesn't make much sense. I think you're missing a comma.

I gave cookie-info its own section. When I tried to put a comma, lint yelled at
me:

** Presubmit Warnings **
chrome\browser\resources\signin_internals\signin_index.css:
- One selector per line (what not to do: a, b {}).
    div#token-info, div#cookie-info {

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/signin_internals/signin_internals.js (right):

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/resources...
chrome/browser/resources/signin_internals/signin_internals.js:177: var
cookieInfoDiv = $('cookie-info');
On 2014/08/08 19:33:54, Evan Stade wrote:
> nit: inline

Done.

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/resources...
chrome/browser/resources/signin_internals/signin_internals.js:189:
chrome.signin.onCookieAccountsFetched.addListener(function(info) {
On 2014/08/08 19:33:54, Evan Stade wrote:
> why not:
> chrome.signin.onCookieAccountsFetched.addListener(updateCookieAccounts);

Done.

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin_internals_ui.cc (right):

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin_internals_ui.cc:92:
"chrome.signin.onCookieAccountsFetched.fire";
On 2014/08/08 19:33:54, Evan Stade wrote:
> nit: just inline the string literal (here and 2 places above)

Done.

[Evan Stade, 2014-08-08 20:05:20]

lgtm

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/signin_internals/signin_index.css (right):

https://codereview.chromium.org/421583002/diff/80001/chrome/browser/resources...
chrome/browser/resources/signin_internals/signin_index.css:33: div#token-info
div#cookie-info {
On 2014/08/08 20:02:04, Mike Lerman wrote:
> On 2014/08/08 19:33:53, Evan Stade wrote:
> > this selector doesn't make much sense. I think you're missing a comma.
> 
> I gave cookie-info its own section. When I tried to put a comma, lint yelled
at
> me:
> 
> ** Presubmit Warnings **
> chrome\browser\resources\signin_internals\signin_index.css:
> - One selector per line (what not to do: a, b {}).
>     div#token-info, div#cookie-info {

just put a line return after the comma

[Mike Lerman, 2014-08-08 20:07:32]

The CQ bit was checked by mlerman@chromium.org

[Mike Lerman, 2014-08-08 20:07:54]

On 2014/08/08 20:05:20, Evan Stade wrote:
> lgtm
> 
>
https://codereview.chromium.org/421583002/diff/80001/chrome/browser/resources...
> File chrome/browser/resources/signin_internals/signin_index.css (right):
> 
>
https://codereview.chromium.org/421583002/diff/80001/chrome/browser/resources...
> chrome/browser/resources/signin_internals/signin_index.css:33: div#token-info
> div#cookie-info {
> On 2014/08/08 20:02:04, Mike Lerman wrote:
> > On 2014/08/08 19:33:53, Evan Stade wrote:
> > > this selector doesn't make much sense. I think you're missing a comma.
> > 
> > I gave cookie-info its own section. When I tried to put a comma, lint yelled
> at
> > me:
> > 
> > ** Presubmit Warnings **
> > chrome\browser\resources\signin_internals\signin_index.css:
> > - One selector per line (what not to do: a, b {}).
> >     div#token-info, div#cookie-info {
> 
> just put a line return after the comma

Cool, thanks Evan. I don't do much CSS these days.

[commit-bot: I haz the power, 2014-08-08 20:09:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/421583002/120001

[Mike Lerman, 2014-08-08 20:54:09]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-08-08 20:56:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/421583002/140001

[commit-bot: I haz the power, 2014-08-09 02:26:09]

Change committed as 288493