Remove All Cookies button is enabled when no cookies are present

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Preferences
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: aakashd, Assigned: Javi Rueda)

Tracking

Trunk
Firefox 10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [3.5testday])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 380501 [details]
bug screenshot

Build Id: trunk and shiretoko nightlies

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090528 Minefield/3.6a1pre ID:20090528112613

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090529 Shiretoko/3.5pre ID:20090529033917


Steps to Reproduce:
1. Go to the Privacy Pane in the Preferences/Options Dialog Window
2. Use custom settings for history and click on the Show Cookies button
3. Click on the Remove All Cookies button
4. Click on the Remove All Cookies button again

Actual Results:
The Remove All Cookies button is still enabled once all cookies have been removed

Expected Results:
The Remove All Cookies button should be grayed out
(Reporter)

Updated

8 years ago
Whiteboard: [3.5testday]
Created attachment 402961 [details] [diff] [review]
Disables "Remove All Cookies" button when no cookies exist.

New code disables "Remove All Cookies" button when no cookies exist after either "Remove Cookies" or "Remove All Cookies" buttons are pressed.
Attachment #402961 - Flags: review?(gavin.sharp)
Comment on attachment 402961 [details] [diff] [review]
Disables "Remove All Cookies" button when no cookies exist.

This dialog deals with dynamic changes to the cookie store while it's open, so you can't just do this in the button actions. See e.g. _handleCookieAdded and the "observe" method.

You'd need to make sure you update the button correctly when cookies are otherwise added/removed.
Attachment #402961 - Flags: review?(gavin.sharp) → review-
Version: Trunk → 3.5 Branch
(Assignee)

Updated

6 years ago
Assignee: nobody → leofigueres
(Assignee)

Comment 3

6 years ago
Created attachment 566593 [details] [diff] [review]
patch

Disables button "Remove All Cookies" when populating list view and when the list is cleared. Using the Observe method.

Re-enables it when there ia, at least, 1 item at the cookies list.
Attachment #402961 - Attachment is obsolete: true
Attachment #566593 - Flags: review?(gavin.bugzilla)
(Assignee)

Updated

6 years ago
Attachment #566593 - Flags: review?(gavin.bugzilla) → review?(gavin.sharp)
Attachment #566593 - Flags: review?(gavin.sharp) → review?(jwein)
Comment on attachment 566593 [details] [diff] [review]
patch

Review of attachment 566593 [details] [diff] [review]:
-----------------------------------------------------------------

Overall approach looks good. r+ with the following changes.

::: browser/components/preferences/cookies.js
@@ +113,4 @@
>          this._handleCookieAdded(aCookie, strippedHost);
> +        if (this._view._rowCount > 0)
> +          document.getElementById("removeAllCookies").disabled = false;
> +      }

_handleCookieAdded already updates the disabled state of the removeAllCookies button. Can you update the _handleCookieAdded function to also check for this._view._rowCount when setting the disabled property?

@@ +133,5 @@
>        this._populateList(false);
> +      if (this._view._rowCount > 0)
> +        document.getElementById("removeAllCookies").disabled = false;
> +      else
> +        document.getElementById("removeAllCookies").disabled = true;

These lines should be moved inside of populateList and combined with the code that was added there. Further, this could be simplified by doing
|document.getElementById("removeAllCookies").disabled = this._view._rowCount == 0;|
Attachment #566593 - Flags: review?(jwein) → review+
Status: NEW → ASSIGNED
Version: 3.5 Branch → Trunk
(Assignee)

Comment 5

6 years ago
Created attachment 567901 [details] [diff] [review]
Patch for bug 495511

With some changes.
Attachment #566593 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
Comment on attachment 567901 [details] [diff] [review]
Patch for bug 495511

addressed reviewer comments about my previous patch, carrying forward their r+.
Attachment #567901 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Comment on attachment 567901 [details] [diff] [review]
Patch for bug 495511

Review of attachment 567901 [details] [diff] [review]:
-----------------------------------------------------------------

I will push to try for you and if all tests pass will check it in for you.

::: browser/components/preferences/cookies.js
@@ +112,2 @@
>          this._handleCookieAdded(aCookie, strippedHost);
> +      }

Nit: please remove these curly brackets now since they are unnecessary. Sorry for not mentioning it earlier when I requested the refactoring.
Keywords: checkin-needed
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=5198c5a002dd

Comment 9

6 years ago
Try run for 5198c5a002dd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5198c5a002dd
Results (out of 148 total builds):
    success: 132
    warnings: 3
    failure: 13
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-5198c5a002dd
(Assignee)

Comment 10

6 years ago
(In reply to Mozilla RelEng Bot from comment #9)
> Try run for 5198c5a002dd is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=5198c5a002dd
> Results (out of 148 total builds):
>     success: 132
>     warnings: 3
>     failure: 13
> Builds available at
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-
> 5198c5a002dd

Any action needed because of this failures?

(In reply to Jared Wein [:jwein] from comment #7)
> ::: browser/components/preferences/cookies.js
> @@ +112,2 @@
> >          this._handleCookieAdded(aCookie, strippedHost);
> > +      }
> 
> Nit: please remove these curly brackets now since they are unnecessary.
> Sorry for not mentioning it earlier when I requested the refactoring.

Done. I should have realize about that earlier.
Those failures seem innocuous. JetPack builds seem to fail all the time for some reason.

I've fixed the last remaining nit and pushed to the fx-team repository. This patch should land in mozilla-central in a day or two.

Thanks for your contribution!

https://hg.mozilla.org/integration/fx-team/rev/4d5244c0a0de
Whiteboard: [3.5testday] → [3.5testday][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4d5244c0a0de
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [3.5testday][fixed-in-fx-team] → [3.5testday]
Target Milestone: --- → Firefox 10

Updated

6 years ago
Depends on: 705422
You need to log in before you can comment on or make changes to this bug.