Fix missing entry showing number of cookies in first domain of chrome://settings/cookies

Fix missing entry showing number of cookies in first domain of chrome://settings/cookies


BUG=130759
TEST=check that the chrome://settings/cookies dialog continues to work as expected


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152309

[battre, 2012-08-09 15:31:57]

James or Evan, could either one of you (who feels more familiar with the
chrome://settings/cookies dialog) please review this CL.

The issue is pretty subtle (side-effects of a constructor) and I have tried to
describe it in the CL.

Thanks,
Dominic

[James Hawkins, 2012-08-09 15:40:04]

I suggest pinging mdm@ to see if he's able to review (he's not full-time on
Chrome).

[battre, 2012-08-09 16:02:12]

+Mike as suggested reviewer by James

[Mike Mammarella, 2012-08-09 16:13:59]

On 2012/08/09 16:02:12, battre wrote:
> +Mike as suggested reviewer by James

I'm also on vacation this week. :) I can look next week though.

[Mike Mammarella, 2012-08-14 00:50:55]

https://chromiumcodereview.appspot.com/10832224/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options2/cookies_list.js (right):

https://chromiumcodereview.appspot.com/10832224/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/cookies_list.js:600: if (this.parent_ ==
undefined || parent.listIndex != -1) {
I'm not quite sure I understand the first part of this condition. Why not just
"if (parent.listIndex != -1) {" here? Is the idea that if we have no parent
right now, then the fake -1 index CookieListItem is OK, but otherwise we want to
ignore attempting to set it on top of the previous parent?

[battre, 2012-08-17 15:59:05]

http://codereview.chromium.org/10832224/diff/1/chrome/browser/resources/optio...
File chrome/browser/resources/options2/cookies_list.js (right):

http://codereview.chromium.org/10832224/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/cookies_list.js:600: if (this.parent_ ==
undefined || parent.listIndex != -1) {
On 2012/08/14 00:50:56, Mike Mammarella wrote:
> I'm not quite sure I understand the first part of this condition. Why not just
> "if (parent.listIndex != -1) {" here? Is the idea that if we have no parent
> right now, then the fake -1 index CookieListItem is OK, but otherwise we want
to
> ignore attempting to set it on top of the previous parent?

This is actually necessary. This way the parent with an invalid index can be
registered once and update the link via the 'listIndexChange' event that we
subscribe to below. If we remove the first condition, no summaries are shown at
all.

[Mike Mammarella, 2012-08-17 18:03:48]

LGTM

[commit-bot: I haz the power, 2012-08-17 18:38:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10832224/1

[commit-bot: I haz the power, 2012-08-17 18:38:21]

Presubmit check for 10832224-1 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/resources/options2

Presubmit checks took 1.5s to calculate.

[battre, 2012-08-17 18:39:31]

James, could you please do the OWNERs review and submit it into the CQ if it
LGTY?

Thanks,
Dominic

On 2012/08/17 18:38:21, I haz the power (commit-bot) wrote:
> Presubmit check for 10832224-1 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome/browser/resources/options2
> 
> Presubmit checks took 1.5s to calculate.

[James Hawkins, 2012-08-19 06:18:18]

LGTM

[commit-bot: I haz the power, 2012-08-19 06:18:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10832224/1

[commit-bot: I haz the power, 2012-08-19 06:18:31]

Failed to apply patch for chrome/browser/resources/options2/cookies_list.js:
While running patch -p1 --forward --force;
A         chrome/browser/resources/options2
patching file chrome/browser/resources/options2/cookies_list.js
Hunk #1 FAILED at 597.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/resources/options2/cookies_list.js.rej

[commit-bot: I haz the power, 2012-08-20 07:51:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10832224/3002

[commit-bot: I haz the power, 2012-08-20 10:07:01]

Change committed as 152309