Closed Bug 474431 Opened 13 years ago Closed 6 years ago

Clear Recent History dialog should have smarter canClear for site prefs

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: johnath, Unassigned)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 7 obsolete files)

Bug 380852 adds support for clearing site-specific prefs from the Clear Recent History dialog, but the code in sanitize.js which checks whether there are any prefs to clear just returns true right now.  The user-visible result of this is that the checkbox, which should grey out when there's nothing to do, remains active, possibly confusing a user who doesn't expect any such entries to exist.

We should implement canClear to just return 

(does nsIPermissionManager have any entries?) || (does nsIContentPrefService have any entries?)

nsIPermissionManager already has an enumerator available, which can be used to determine if any entries exist.  nsIContentPrefService does not currently have such an interface, though.  Fixing this bug will involve adding some interface to nsIContentPrefService to either return all prefs (getPrefs() already exists to do this for a particular URI) or at least a boolean to return whether or not any prefs are stored (though this seems a lot less useful than the first approach).
Whiteboard: [good first bug]
Once this is done, we should also update the test at:

  browser/base/content/test/browser_sanitize-sitepermissions.js 

to ensure that we are correctly clearing nsIContentPrefService prefs as well (see gavin's bug 380852 comment 19).
Attached patch patch for bug (obsolete) — Splinter Review
This is my proposition to solve this bug. I added to nsIContentPrefService hasGroupedPrefs function which return boolean. This is second approach. I choose it because it is faster. I also think that returning all preferences in nsIPropertyBag2 is useless because why don't know to which site this setting belong.
It will be better to add another function getGroups to return existing groups but I don't know if there is interface for groups.
Attachment #620770 - Flags: review?(johnath)
Attachment #620770 - Flags: checkin+
Hey Michal, thanks for the patch! Looks like exactly what we need. We're currently re-working the nsContentsPrefService API in bug 699859, though, so we might need to coordinate landing this change with that work.
The async API we have in bug 699859 doesn't cover this functionality in a performant way, so we'll need to add a new method there.
Unfortunately canClear is synchronous, so that's not just matter of adding an async method and using it, some of sanitize.js will have to be rewritten, and this bug would end up being blocked on that rewrite for quite some time.
So, I guess we may just proceed here regardless bug 699859; there we'll handle the need when we reach the point we replace call points to the old API.
Attachment #620770 - Flags: review?(mak77)
Attachment #620770 - Flags: review?(johnath)
Attachment #620770 - Flags: checkin+
Assignee: nobody → mjaskurzynski
Status: NEW → ASSIGNED
(Thank you for stealing that review, mak)

You're in good hands, Michal! And thanks for contributing!
Comment on attachment 620770 [details] [diff] [review]
patch for bug

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

Sorry if it took a bit of time, so first of all thanks for looking into this issue.
Looks like this is your first patch, so there is some code convention you don't know about our codebase, that I'll try to explain.
And there is some performance issue to look into, yet, but globally the approach is fine.

::: browser/base/content/sanitize.js
@@ +397,5 @@
>        },
>        
>        get canClear()
>        {
> +    	  var pm = Components.classes["@mozilla.org/permissionmanager;1"]

globally: looks like you have some tabs in indentations.  You should setup your text editor to replace tabs with spaces, we indent with 2 spaces in this file (and more generally in most of the codebase we only use spaces, some rare file may differ).

@@ +398,5 @@
>        
>        get canClear()
>        {
> +    	  var pm = Components.classes["@mozilla.org/permissionmanager;1"]
> +    	                             .getService(Components.interfaces.nsIPermissionManager);

globally: please align .getService to .classes, per convention in this file (usually just look at the surrounding code if you're in doubt about which style to follow)

@@ +400,5 @@
>        {
> +    	  var pm = Components.classes["@mozilla.org/permissionmanager;1"]
> +    	                             .getService(Components.interfaces.nsIPermissionManager);
> +    	  if(pm.enumerator.hasMoreElements())
> +    	  {

globally: if (condition) {

@@ +413,5 @@
> +    	  }
> +    	  
> +    	  var pwmgr = Components.classes["@mozilla.org/login-manager;1"]
> +    	                                .getService(Components.interfaces.nsILoginManager);
> +    	  if(pwmgr.getAllDisabledHosts().length > 0)

this is quite not performant, it has to query for each entry, return all of them, and then count.  I'd not care for the dialog, but I think we invoke this code in the shutdown path when you want to clear on shutdown, that is a bit more critical path.

I think we need a faster method in nsILoginManager to check if there's any disabled host to clear, similar to what you did for contentPrefs, basically.

::: dom/interfaces/base/nsIContentPrefService.idl
@@ +163,5 @@
>     */
>    void removeGroupedPrefs();
> +  
> +  /**
> +   * Check whether or not there is at least one group

"Check whether or not there is any grouped pref."

@@ +166,5 @@
> +  /**
> +   * Check whether or not there is at least one group
> +   */ 
> +  
> +  boolean hasGroupedPrefs();

please remove the newline before the method, the javadoc should immediately precede it

since you are changing the interface, you have to generate a new uuid and replace the current one at the top of this interface (http://mozilla.pettay.fi/cgi-bin/mozuuid.pl).

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +567,5 @@
>    },
> +  
> +  hasGroupedPrefs: function ContentPrefService_hasGroupedPrefs() {
> +	  
> +	  var hasGroupedPrefRes = false;

you likely don't need this var, just return from the try and return false at the end, the finally is executed regardless.

@@ +569,5 @@
> +  hasGroupedPrefs: function ContentPrefService_hasGroupedPrefs() {
> +	  
> +	  var hasGroupedPrefRes = false;
> +	  
> +	  var selectGroupAmountStmt = this._dbCreateStatement('SELECT count() as amount FROM groups');

please use double quotes for strings, per convention in this file (and more generally).

count() is not performant in SQLite (the number of rows per table is not cached anywhere), so you'd better do just
"SELECT 1 FROM groups"
later you can just: return stmt.executeStep(); in the try, if there are no rows it will return false.

@@ +579,5 @@
> +	  finally {
> +		  selectGroupAmountStmt.reset();
> +	  }
> +	  
> +	  return hasGroupedPrefRes;

and here just return false; will be hit only if there's an exception
Attachment #620770 - Flags: review?(mak77)
Attached patch revise bug patch (obsolete) — Splinter Review
I revised my bug patch.
Attachment #620770 - Attachment is obsolete: true
Attachment #623426 - Flags: review?(mak77)
Comment on attachment 623426 [details] [diff] [review]
revise bug patch

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

So, this is OK, but it's lacking tests. Shouldn't be too hard to add a couple simple ones, see https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests

::: browser/base/content/sanitize.js
@@ +399,5 @@
>        get canClear()
>        {
> +        var pm = Components.classes["@mozilla.org/permissionmanager;1"]
> +                           .getService(Components.interfaces.nsIPermissionManager);
> +        if(pm.enumerator.hasMoreElements()){

please fix spacing as "if () {"
this is valid globally, so not going to repeat it

::: dom/interfaces/base/nsIContentPrefService.idl
@@ +165,5 @@
> +  
> +  /**
> +   * Check whether or not there is any grouped pref.
> +   */ 
> +  boolean hasGroupedPrefs();

Would be cool to have a simple xpcshell test that ensures this API works properly (thus checks it's false, adds something and checks it's true, removes and checks it's false again)

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +569,5 @@
> +  hasGroupedPrefs: function ContentPrefService_hasGroupedPrefs() {
> +
> +    var selectGroupAmountStmt = this._dbCreateStatement("SELECT 1 FROM groups");
> +    
> +    try {

please, remove all of the useless newlines here

::: toolkit/components/passwordmgr/nsILoginManager.idl
@@ +143,5 @@
>  
>      /**
> +     * Check whether or not there is any disabled host.
> +     */
> +    boolean hasDisabledHost();

As well has here would be cool to have a simple xpcshell test ensuring this API works.

::: toolkit/components/passwordmgr/storage-mozStorage.js
@@ +766,5 @@
>  
>      /*
> +     * hasDisabledHost
> +     *
> +     */

I don't see any value in this comment, I know other methods do the same, but it's just dumb, so let's not promote it to new code.
We may add documentation to it, but that's what the idl is for, so really, I don't see a reason :)

@@ +770,5 @@
> +     */
> +    hasDisabledHost : function () {
> +      let selectDisabledHostStmt = this._dbCreateStatement("SELECT 1 FROM moz_disabledHosts");
> +      
> +      try {

please remove useless newline

@@ +778,5 @@
> +        selectDisabledHostStmt.reset();
> +      }
> +      
> +      return false; 
> +    },

you should also add this method to storage-Legacy.js (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/storage-Legacy.js) since both storages implement nsILoginManagerStorage.
Attachment #623426 - Flags: review?(mak77) → feedback+
Attached patch revise bug patch with tests (obsolete) — Splinter Review
Hi,
I corrected my patch
Attachment #623426 - Attachment is obsolete: true
Comment on attachment 627571 [details] [diff] [review]
revise bug patch with tests

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

I'm really sorry this took so much, in case I don't hear back I'll provide to apply the changes and submit your patch to the tree, the changes look good, just a lot of trailing spaces to fix, an error and a couple improvements.
Getting a tryserver run would be good to verify the tests are stable. Finally the next fixed version will need a superreview for the idl changes.

::: browser/base/content/sanitize.js
@@ +402,5 @@
> +                           .getService(Components.interfaces.nsIPermissionManager);
> +        if (pm.enumerator.hasMoreElements()) {
> +          return true;
> +        }
> +        

Trailing spaces

@@ +408,5 @@
> +                            .getService(Components.interfaces.nsIContentPrefService);
> +        if (cps.hasGroupedPrefs()) {
> +          return true;
> +        }
> +        

Trailing spaces

@@ +414,5 @@
> +                              .getService(Components.interfaces.nsILoginManager);
> +        if (pwmgr.hasDisabledHost()) {
> +          return true;
> +        }
> +        

Trailing spaces

::: dom/interfaces/base/nsIContentPrefService.idl
@@ +165,5 @@
> +  
> +  /**
> +   * Check whether or not there is any grouped pref.
> +   */ 
> +  boolean hasGroupedPrefs();

Trailing spaces in the row above the javadoc and on its end

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +565,5 @@
>        throw ex;
>      }
>    },
> +  
> +  hasGroupedPrefs: function ContentPrefService_hasGroupedPrefs() {

trailing spaces above

@@ +571,5 @@
> +    try {
> +      return selectGroupAmountStmt.executeStep();
> +    }
> +    finally {
> +      selectGroupAmountStmt.reset();

this should be .finalize() since the query is not cached anywhere

::: toolkit/components/passwordmgr/storage-Legacy.js
@@ +415,5 @@
>          return result;
>      },
>  
> +    
> +    hasDisabledHost : function () {

trailing spaces above

@@ +419,5 @@
> +    hasDisabledHost : function () {
> +        for (var hostname in this._disabledHosts) {
> +            return true;
> +        }
> +        return false;

return Object.keys(this._disabledHosts).length > 0

@@ +425,2 @@
>  
> +      

trailing spaces

::: toolkit/components/passwordmgr/storage-mozStorage.js
@@ +769,5 @@
> +        try {
> +            return selectDisabledHostStmt.executeStep();
> +        }
> +        finally {
> +            selectDisabledHostStmt.reset();

this reset() is instead _correct_ since storage-mozStorage.js caches the statements... pay attention to not change the wrong one :)

@@ +771,5 @@
> +        }
> +        finally {
> +            selectDisabledHostStmt.reset();
> +        }
> +        return false; 

trailing space

::: toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_1.js
@@ +297,5 @@
> +/* ========== 14 ========== */
> +testnum++;
> +
> +testdesc = "checking that hasDisabledHost works"
> +	

trailing spaces
Attachment #627571 - Flags: feedback?(mak77) → review+
Flagging myself just to remember to check this back.
Flags: needinfo?(mak77)
Attached patch revised patch (obsolete) — Splinter Review
Attachment #627571 - Attachment is obsolete: true
Attachment #671071 - Flags: review?(mak77)
Comment on attachment 671071 [details] [diff] [review]
revised patch

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

Thank you, it looks good to me.

Please get a tryserver run, if you have level1 hg access. Otherwise just flag me for needinfo and I'll do that for you.

Finally, this just needs a SR for the idl changes, flagging Gavin, feel free to foward in case.
Attachment #671071 - Flags: superreview?(gavin.sharp)
Attachment #671071 - Flags: review?(mak77)
Attachment #671071 - Flags: review+
Flags: needinfo?(mak77)
Flags: needinfo?(mak77)
Whiteboard: [good first bug] → [good first bug][autoland-try]
Whiteboard: [good first bug][autoland-try] → [good first bug][autoland-in-queue]
looks like autoland has been disabled, will do manually.
Flags: needinfo?(mak77)
Whiteboard: [good first bug][autoland-in-queue] → [good first bug]
Try run for 2fd797b9d4eb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2fd797b9d4eb
Results (out of 187 total builds):
    exception: 2
    success: 160
    warnings: 20
    failure: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mak77@bonardo.net-2fd797b9d4eb
looks like there is a failure in browser_sanitizeDialog.js browser-chrome test, are you able to reproduce it locally?
Flags: needinfo?(mjaskurzynski)
Yes, I can reproduce it locally but I need some time to investigate.
Flags: needinfo?(mjaskurzynski)
time is not a big deal, it's possible the test just needs to be updated.
Attached patch revised patch (obsolete) — Splinter Review
I changed order of tests because bug 527820 test step rely on fact that site prefs returns true in canClear function.
Attachment #671071 - Attachment is obsolete: true
Attachment #671071 - Flags: superreview?(gavin.sharp)
Attachment #676212 - Flags: review?(mak77)
Comment on attachment 676212 [details] [diff] [review]
revised patch

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

thanks, I'm pushing this again to try to verify the test fix.
Attachment #676212 - Flags: superreview?(gavin.sharp)
Attachment #676212 - Flags: review?(mak77)
Attachment #676212 - Flags: review+
unfortunately looks like the test is still unhappy

https://tbpl.mozilla.org/php/getParsedLog.php?id=16617445&tree=Try#error0
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_sanitizeDialog.js | Window should be tall enough to fit warning panel and item list
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_sanitizeDialog.js | Dialog's OK button should not be disabled
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_sanitizeDialog.js | Details should be hidden but were actually shown - Got false, expected true
Try run for 79b08a184052 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=79b08a184052
Results (out of 188 total builds):
    exception: 1
    success: 158
    warnings: 21
    failure: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mak77@bonardo.net-79b08a184052
Attached patch revised patch (obsolete) — Splinter Review
Hi Marco,

Could you push it to try server?
Attachment #676212 - Attachment is obsolete: true
Attachment #676212 - Flags: superreview?(gavin.sharp)
Attachment #682868 - Flags: review?(mak77)
Comment on attachment 682868 [details] [diff] [review]
revised patch

>diff --git a/dom/interfaces/base/nsIContentPrefService.idl b/dom/interfaces/base/nsIContentPrefService.idl

>   /**
>+   * Check whether or not there is any grouped pref.
>+   */
>+  boolean hasGroupedPrefs();

Why is this a method rather than a readonly attribute?

comment nit: "Returns true if there are any grouped prefs."

>diff --git a/toolkit/components/passwordmgr/nsILoginManager.idl b/toolkit/components/passwordmgr/nsILoginManager.idl
>diff --git a/toolkit/components/passwordmgr/nsILoginManagerStorage.idl b/toolkit/components/passwordmgr/nsILoginManagerStorage.idl

Same two comments apply to the changes here as well (comment should be "Returns true if there are any disabled hosts.").

Looks good otherwise, sr=me with those addressed.
Attachment #682868 - Flags: superreview+
Attached patch revised patch (obsolete) — Splinter Review
Attachment #682868 - Attachment is obsolete: true
Attachment #682868 - Flags: review?(mak77)
Attachment #682901 - Flags: superreview?(gavin.sharp)
Attachment #682901 - Flags: review?(mak77)
Comment on attachment 682901 [details] [diff] [review]
revised patch

I think Marco mentioned this already, but the comments before the method in nsLoginManager.js and storage-Legacy.js seem unnecessary (the method's implementation is pretty self-explanatory, and the IDL comments are sufficient otherwise). I know you're just copying surrounding code, but let's not spread that mistake :)

Thanks - you don't need to re-request (super-)review once it's been granted, assuming you've only made the requested changes.
Attachment #682901 - Flags: superreview?(gavin.sharp) → superreview+
pushed to try. Will review once it's done.
Looks like Try server bot fell asleep and didn't post results... Oh well, here it is:
https://tbpl.mozilla.org/?tree=Try&rev=af8d0ab67c60
Looks like the test is now properly passing.
Comment on attachment 682901 [details] [diff] [review]
revised patch

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

::: browser/base/content/sanitizeDialog.js
@@ +52,5 @@
>        if (!s.canClearItem(name)) {
>          prefItem.preference = null;
>          prefItem.checked = false;
>          prefItem.disabled = true;
> +        prefs.setBoolPref(prefName, true);

Why are you doing this? At first glance looks like a workaround for something, but doesn't seem sane to set pref without user intervention. Maybe you want to invoke updatePrefs() before creating the Sanitizer(), like we do in sanitize()?

An added comment would be extremely appreciated.

::: dom/interfaces/base/nsIContentPrefService.idl
@@ +165,5 @@
>  
>    /**
> +   * Returns true if there are any grouped prefs.
> +   */
> +  readonly attribute boolean hasGroupedPrefs;

this is no more a method, so "returns" doesn't sound right. Just "Whether there is any grouped pref."

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +515,5 @@
>        throw ex;
>      }
>    },
> +  
> +  get hasGroupedPrefs() {

trailing space before the getter

@@ +516,5 @@
>      }
>    },
> +  
> +  get hasGroupedPrefs() {
> +    var selectGroupAmountStmt = this._dbCreateStatement("SELECT 1 FROM groups");

please use let

::: toolkit/components/passwordmgr/nsILoginManager.idl
@@ +110,5 @@
>  
>  
>      /**
> +     * Returns true if there are any disabled hosts.
> +     */

"Whether there is any disabled host."

::: toolkit/components/passwordmgr/nsILoginManagerStorage.idl
@@ +174,5 @@
>                        [retval, array, size_is(count)] out wstring hostnames);
>  
>  
>      /**
> +     * Returns true if there are any disabled hosts.

ditto

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +437,5 @@
> +     * hasDisabledHost
> +     *
> +     * Returns true if there are any disabled hosts.
> +     */
> +    get hasDisabledHost() {

please avoid the function name repetition

we actually don't even need a javadoc at all here since the idl comment wins.

::: toolkit/components/passwordmgr/storage-Legacy.js
@@ +386,5 @@
>  
>      /*
> +     * Returns true if there are any disabled hosts.
> +     */
> +    get hasDisabledHost() {

Doesn't need a javadoc
Attachment #682901 - Flags: review?(mak77)
::: browser/base/content/sanitizeDialog.js
@@ +52,5 @@
>        if (!s.canClearItem(name)) {
>          prefItem.preference = null;
>          prefItem.checked = false;
>          prefItem.disabled = true;
> +        prefs.setBoolPref(prefName, true);

In test case browser_sanitize-timespans.js on line 42 "siteSettings" pref is set to false and this affects test case browser_sanitizeDialog.js. I used "prefs.setBoolPref(prefName, true);" in "!s.canClearItem(name)" case because it is default value in that situation and resets previous wrong values. "updatePrefs()" won't solve the problem.
Hi Marco,

what do you think about that? Can I leave it (adding comment)?
Flags: needinfo?(mak77)
(In reply to Michal Jaskurzynski from comment #30)
> In test case browser_sanitize-timespans.js on line 42 "siteSettings" pref is
> set to false and this affects test case browser_sanitizeDialog.js.

If a browser-chrome test is changing anything from the default values, its its duty to clean up the environment, in this case browser_sanitize-timespans.js should indeed call RegisterCleanupFunction and in the function call clearUserPref on all of the prefs it modifies.  Would this fix your problem?

> I used
> "prefs.setBoolPref(prefName, true);" in "!s.canClearItem(name)" case because
> it is default value in that situation and resets previous wrong values.
> "updatePrefs()" won't solve the problem.

I didn't have enough time to look deeper into the consequences of such a change, but the fact we can't clear the item doesn't mean it should be cleared, the user may have unchecked it and we'd override his choice. Unless I missed some black magic in this piece of code.
Flags: needinfo?(mak77)
Michal, have you had a chance to look into the test further here? It would be great to get this bug landed.
I will work on it during weekend.
Try run for af8d0ab67c60 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=af8d0ab67c60
Results (out of 188 total builds):
    success: 175
    warnings: 12
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mak77@bonardo.net-af8d0ab67c60
wow, this try build started on 19 Nov and was notified now...
Attached patch revised patchSplinter Review
Try results:
https://tbpl.mozilla.org/?tree=Try&rev=fdd31ff8cc4a
Attachment #682901 - Attachment is obsolete: true
Attachment #698017 - Flags: review?(mak77)
Comment on attachment 698017 [details] [diff] [review]
revised patch

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

still a small cleanup to the test, then I think we're fine.
The failures on try don't seem related to the patch, maybe you just pushed on top of an unlucky changeset.

::: browser/base/content/test/browser_sanitize-timespans.js
@@ +18,5 @@
> +    let s = new Sanitizer();
> +    s.prefDomain = "privacy.cpd.";
> +    var itemPrefs = gPrefService.getBranch(s.prefDomain);
> +    itemPrefs.setBoolPref("siteSettings", true);
> +  });

I think you should instead define a const global like

const SANITIZE_PREFS = {
  "history": true,
  "downloads": true,
  ... (Copy prefs and values from below code)
}

then instaead of this:

34   itemPrefs.setBoolPref("history", true);
35   itemPrefs.setBoolPref("downloads", true);
36   itemPrefs.setBoolPref("cache", false);
37   itemPrefs.setBoolPref("cookies", false);
38   itemPrefs.setBoolPref("formdata", true);
39   itemPrefs.setBoolPref("offlineApps", false);
40   itemPrefs.setBoolPref("passwords", false);
41   itemPrefs.setBoolPref("sessions", false);
42   itemPrefs.setBoolPref("siteSettings", false);

do:

for (let pref in SANITIZE_PREFS) {
  itemPrefs.setBoolPref(pref, SANITIZE_PREFS[pref]);
}

in RegisterCleanupFunction then you should be able to just:

let prefBranch = gPrefService.getBranch("privacy.cpd.");
Object.keys(SANITIZE_PREFS).forEach(prefBranch.clearUserPref);
Attachment #698017 - Flags: review?(mak77)
Michal, are you still working on this bug? I'm going through older "good first bugs" that have been inactive for a while. 
Do you need help to keep on working through this? I think Marco could help you.
Flags: needinfo?(mjaskurzynski)
It's passed a long time since last Michal response, I'm changing the status to NEW, maybe someone else is interested in working on this bug.
Status: ASSIGNED → NEW
Assignee: mjaskurzynski → nobody
Flags: needinfo?(mjaskurzynski)
I’d like to work on this bug. I looked at the patch and the comments and I wonder if it needs major rework considering bug 699859. nsIContentPrefService is deprecated https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIContentPrefService so it might be better to rewrite the patch to use nsIContentPrefService2?
Hi, I'm not sure it's worth spending time on this.
The fact is, the only thing currently returning a meaningful canClear value is formData, and it has a quite complicated code. Here we'd be adding more APIs just to support the second case.

the only scope here is to disable checkboxes in the dialog to not "surprise" the user thinking he doesn't have data... But for years the only checkbox showing a proper status was form data. I honestly didn't see many reports of confused users.

I wonder if the premise is not that good, could be we assumed the user really cares about data existing or not when looking at this dialog, but he doesn't really. Users who care about having no data are likely to use permanent private browsing. Others just want to clear existing data, no matter what.
If we could just remove the whole canClear thing, we would have a simpler code to maintain, and we'd just lose the form data checkbox disabling. all services should be more than able to no-op when there's no data.
Fwiw, other browsers are doing the same, and their users don't really look confused.

If I can find some agreement on this, it's likely we can wontfix this bug and file a new one to remove canClear completely, and you could work on that.

I'm setting some needinfos to see what some other browser peers think about this proposal.
Flags: needinfo?(dolske)
Flags: needinfo?(dao)
Sounds good to me.
Flags: needinfo?(dao)
Yeah, make sense. I'd actually expect people to be more likely confused by the disabled checkbox ("why won't it let me clear X?!") than by offering to clear something that doesn't actually exist.
Flags: needinfo?(dolske)
So, ariasuni, if you wish to work on this, please move to bug 1211849. I'm going to wontfix this bug.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(perso+bugmoz)
Resolution: --- → WONTFIX
Flags: needinfo?(perso+bugmoz)
You need to log in before you can comment on or make changes to this bug.