Last Comment Bug 495511 - Remove All Cookies button is enabled when no cookies are present
: Remove All Cookies button is enabled when no cookies are present
Status: RESOLVED FIXED
[3.5testday]
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 10
Assigned To: Javi Rueda
:
Mentors:
Depends on: 705422
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-29 12:37 PDT by Aakash Desai [:aakashd]
Modified: 2011-11-26 11:51 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
bug screenshot (38.46 KB, image/png)
2009-05-29 12:37 PDT, Aakash Desai [:aakashd]
no flags Details
Disables "Remove All Cookies" button when no cookies exist. (769 bytes, patch)
2009-09-25 16:30 PDT, Brian Mastell (:tellmas)
gavin.sharp: review-
Details | Diff | Review
patch (2.54 KB, patch)
2011-10-12 11:00 PDT, Javi Rueda
jaws: review+
Details | Diff | Review
Patch for bug 495511 (2.75 KB, patch)
2011-10-18 15:15 PDT, Javi Rueda
leofigueres: review+
Details | Diff | Review

Description Aakash Desai [:aakashd] 2009-05-29 12:37:22 PDT
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
Comment 1 Brian Mastell (:tellmas) 2009-09-25 16:30:42 PDT
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.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-09-29 13:40:47 PDT
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.
Comment 3 Javi Rueda 2011-10-12 11:00:16 PDT
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.
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2011-10-18 11:29:07 PDT
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;|
Comment 5 Javi Rueda 2011-10-18 15:15:16 PDT
Created attachment 567901 [details] [diff] [review]
Patch for bug 495511

With some changes.
Comment 6 Javi Rueda 2011-10-18 15:30:12 PDT
Comment on attachment 567901 [details] [diff] [review]
Patch for bug 495511

addressed reviewer comments about my previous patch, carrying forward their r+.
Comment 7 Jared Wein [:jaws] (please needinfo? me) 2011-10-18 16:26:12 PDT
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.
Comment 8 Jared Wein [:jaws] (please needinfo? me) 2011-10-18 16:31:09 PDT
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=5198c5a002dd
Comment 9 Mozilla RelEng Bot 2011-10-19 03:20:22 PDT
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
Comment 10 Javi Rueda 2011-10-19 04:35:25 PDT
(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.
Comment 11 Jared Wein [:jaws] (please needinfo? me) 2011-10-19 08:23:07 PDT
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
Comment 12 Tim Taubert [:ttaubert] 2011-10-22 12:14:09 PDT
https://hg.mozilla.org/mozilla-central/rev/4d5244c0a0de

Note You need to log in before you can comment on or make changes to this bug.