436942 - URL Bar history should have a separate sanitize option

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Stefan [:stefanh]]

I think the "Clear Location Bar" section in the History pane can be killed, since the new clear private data basically does the same thing (clears all history, including location bar history). The "Remember visited pages ..." section will then be a bit lonely....

Comment 1

[[e-mail]]

What if someone wants to clear one but not the other?

Comment 2

[Stefan [:stefanh]]

(In reply to comment #1)
> What if someone wants to clear one but not the other?

Can the sanitize panel be tweaked to handle that?

Comment 3

[Robert Kaiser]

And then, I think it still would be good to have that button there as people probably would be looking for that there. But it's surely possible if someone writes up a patch for it.

Comment 4

[Ian Neal]

As KaiRo said most people would expect it under History pane.
Seems strange to just remove the one button as well, surely you would remove the Clear History button for the same reasons.

Looks fairly straight forward to split url bar history from the main history in Sanitizer.jsm though.

Comment 5

[Ian Neal]

Attachment: Add separate privacy item for urlbar location patch v0.1

This patch:
* Adds separate sanitize option for location bar history.
* Splits testing of location bar history from browsing history.

Comment 6

[[e-mail]]

Comment on attachment 397673 [details] [diff] [review]
Add separate privacy item for urlbar location patch v0.1

>         <checkbox label="&itemHistory.label;"
>                   accesskey="&itemHistory.accesskey;"
>                   preference="privacy.item.history"/>
>+        <checkbox label="&itemUrlBar.label;"
>+                  accesskey="&itemUrlBar.accesskey;"
>+                  preference="privacy.item.urlbar"/>
>         <checkbox label="&itemDownloads.label;"
>                   accesskey="&itemDownloads.accesskey;"
>                   preference="privacy.item.downloads"/>
>         <checkbox label="&itemFormSearchHistory.label;"
>                   accesskey="&itemFormSearchHistory.accesskey;"
>                   preference="privacy.item.formdata"/>
[Bah, the list of privacy items seems to be in a different order every time.]

>+      get canClear() {
>+        var file = Components.classes["@mozilla.org/file/directory_service;1"]
>+                             .getService(Components.interfaces.nsIProperties)
>+                             .get("ProfD", Components.interfaces.nsIFile);
>+        file.append("urlbarhistory.sqlite");
>+        return file.exists();
Nit: also needs to check the open location pref; additionally, pref-history.js does a slightly more thorough check, although that makes the test harder.

>         // old xpfe history implementation!
We ought to remove these tests! sr- because of this.

Comment 7

[Ian Neal]

Attachment: Add separate privacy item for urlbar location and remove old xpfe code patch v0.1a

Changes since v0.1:
* Removes old xpfe code.
* Checks open location pref is not locked.
* Checks relevant table exists in the urlbar history file.
* Creates relevant table in the urlbar history file for the test.

Comment 8

[[e-mail]]

Comment on attachment 398747 [details] [diff] [review]
Add separate privacy item for urlbar location and remove old xpfe code patch v0.1a

>+      get canClear() {
>+        var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                              .getService(Components.interfaces.nsIPrefBranch2);
>+        var ableToClear;
>+        try {
>+          ableToClear = !prefs.prefIsLocked("general.open_location.last_url");
>+          if (ableToClear) {
>+            var file = Components.classes["@mozilla.org/file/directory_service;1"]
>+                                 .getService(Components.interfaces.nsIProperties)
>+                                 .get("ProfD", Components.interfaces.nsIFile);
>+            file.append("urlbarhistory.sqlite");
>+            ableToClear = file.exists();
>+          }
>+          if (ableToClear) {
>+            var connection = Components.classes["@mozilla.org/storage/service;1"]
>+                                       .getService(Components.interfaces.mozIStorageService)
>+                                       .openDatabase(file);
>+            ableToClear = connection.tableExists("urlbarhistory");
>+            connection.close();
>+          }
>+        } catch(ex) {
>+          ableToClose = false;
>+        }
>+        return ableToClose;
Hmm, your variable name changed halfway through?
I'm not sure this test is quite right. Mind you, I'm not sure the test in pref-history.js is right either. That test goes like this:
1. If the preference is locked then the urlbar history cannot be cleared.
2. If the preference has a user value then the urlbar history can be cleared.
3. If the urlbarhistry.sqlite does not exists then it cannot be cleared.
4. If it has the urlbarhistory table then it can be cleared.
5. Otherwise, it cannot be cleared.
What's confusing me is that pref-history.js disables the button if the pref is locked even if the urlbarhistory table exists. (And to think I wrote it too...)
Anyway, you don't seem to have taken step 2 into account.

>       if (history.addPageWithDetails) {
>         // toolkit history interfaces can be used
Don't need this check.

>         history.addPageWithDetails(uri, "Sanitizer!", (new Date()).getTime());
[Could change to Date.now()]

>+      if (Components.classes["@mozilla.org/browser/nav-history-service;1"]) {
>+        // toolkit history interfaces can be used
And again.

>+      if (!file.exists()) {
>+        file.create(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0644);
openDatabase should create it.

>+      return locDialog;
Nit: could check that the file has gone.

Comment 9

[Ian Neal]

(In reply to comment #8)
> I'm not sure this test is quite right. Mind you, I'm not sure the test in
> pref-history.js is right either. That test goes like this:
> 1. If the preference is locked then the urlbar history cannot be cleared.
> 2. If the preference has a user value then the urlbar history can be cleared.
> 3. If the urlbarhistry.sqlite does not exists then it cannot be cleared.
> 4. If it has the urlbarhistory table then it can be cleared.
> 5. Otherwise, it cannot be cleared.
So which option should we be able to clear relevant items on?
1) The preference is not locked and has a user value, and the urlbarhistory.sqlite exists and has the relevant table
2) Either the preference is not locked and has a user value or the urlbarhistory.sqlite exists and has the relevant table

Comment 10

[[e-mail]]

(In reply to comment #9)
> 2) Either the preference is not locked and has a user value or the
> urlbarhistory.sqlite exists and has the relevant table
Correct.

Comment 11

[Ian Neal]

I'm trying to do:
        connection.createTable("urlbarhistory", "url TEXT");
        connection.executeSimpleSQL(
          "INSERT INTO urlbarhistory (url) VALUES ('Sanitizer')");
        connection.commitTransaction();
        connection.close();
but when I run the test I get:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/modules/test/browser_sanitizer.js | Exception thrown - [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageConnection.commitTransaction]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://mochikit/content/browser/suite/modules/test/browser_sanitizer.js :: anonymous :: line 144"  data: no]

What am I doing wrong?

Comment 12

[[e-mail]]

Missing connection.beginTransaction()? Or for the purposes of the test you probably don't need either of the begin/endTransaction().

Comment 13

[Ian Neal]

Attachment: Test including inserting data in urlbarhistory table v0.1b

Changes since v0.1a:
* Adds inserting and testing for data in the urlbarhistory table.
* Checks for either rather than both in canClear.

Comment 14

[[e-mail]]

Comment on attachment 399127 [details] [diff] [review]
Test including inserting data in urlbarhistory table v0.1b

>+        var able;
>+        try {
>+          able = !prefs.prefIsLocked("general.open_location.last_url") &&
>+                 prefs.prefHasUserValue("general.open_location.last_url");
>+        } catch(ex) {
>+          able = false;
>+        }
>+        if (able)
>+          return true;
These shouldn't throw, so you can just write
if (!prefs.prefIsLocked("general.open_location.last_url") &&
    prefs.prefHasUserValue("general.open_location.last_url"))
  return true;
r+sr=me with this fixed.

>+        able = connection.tableExists("urlbarhistory");
[Name this var urlbarhistoryExists perhaps?]

>       return this.check();
>     },
>     check: function() {
This is annoying. When setting, you want to ensure that they are both set, while when clearing, you want to ensure that they are both clear. I was toying with the following idea:
  return this.check(true);
},
check: function(aCheckAll) {
  var locDialog = false;
...
  if (locDialog == !aCheckAll)
    return locDialog;
...
[Note: != aAll doesn't work when it's undefined.]

Comment 15

[Ian Neal]

Attachment: Add separate privacy item for urlbar location with aCheckAll patch v0.1c

Revised patch, requesting r= to confirm it is correct.

Comment 16

[[e-mail]]

Comment on attachment 400142 [details] [diff] [review]
Add separate privacy item for urlbar location with aCheckAll patch v0.1c

>+      if (!file.exists())
>+        return false;
>+
>+      if (!aCheckAll)
>+        return true;
Interesting idea... you're checking for the table after creating it, but not when sanitising, because the file shouldn't exist anyway? That sounds like a good idea, but in that case you might as well make the sanitiser claim that it can clear urlbarhistory if the file exists whether or not there is any history in it.

Comment 17

[Ian Neal]

Attachment: Add separate privacy item for urlbar location with reduced canClear patch v0.1d [Checkin: Comment 18]

Requesting a= for low risk patch.

Comment 18

[Ian Neal]

Comment on attachment 400262 [details] [diff] [review]
Add separate privacy item for urlbar location with reduced canClear patch v0.1d [Checkin: Comment 18]

http://hg.mozilla.org/comm-central/rev/724d7a70033a