Closed Bug 495511 Opened 13 years ago Closed 10 years ago

Remove All Cookies button is enabled when no cookies are present

Categories

(Firefox :: Preferences, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 10

People

(Reporter: aakashd, Assigned: foss)

References

Details

(Whiteboard: [3.5testday])

Attachments

(2 files, 2 obsolete files)

Attached image 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
Whiteboard: [3.5testday]
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: nobody → leofigueres
Attached patch patch (obsolete) — Splinter Review
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)
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
With some changes.
Attachment #566593 - Attachment is obsolete: true
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+
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.
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
(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
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [3.5testday][fixed-in-fx-team] → [3.5testday]
Target Milestone: --- → Firefox 10
Depends on: 705422
You need to log in before you can comment on or make changes to this bug.