URL Bar history should have a separate sanitize option

RESOLVED FIXED in seamonkey2.0

Status

RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: stefanh, Assigned: iann_bugzilla)

Tracking

(Blocks: 1 bug, {fixed-seamonkey2.0})

Trunk
seamonkey2.0
fixed-seamonkey2.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

11 years ago
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....
(Reporter)

Updated

11 years ago
Summary: Does the History pane have a future? → "Clear Location Bar" section in History pane should be removed

Comment 1

10 years ago
What if someone wants to clear one but not the other?
(Reporter)

Comment 2

10 years ago
(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

10 years ago
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.
(Assignee)

Comment 4

9 years ago
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.
(Assignee)

Comment 5

9 years ago
Created attachment 397673 [details] [diff] [review]
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.
Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #397673 - Flags: superreview?(neil)
Attachment #397673 - Flags: review?(neil)

Comment 6

9 years ago
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.
Attachment #397673 - Flags: superreview?(neil)
Attachment #397673 - Flags: superreview-
Attachment #397673 - Flags: review?(neil)
(Assignee)

Comment 7

9 years ago
Created attachment 398747 [details] [diff] [review]
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.
Attachment #397673 - Attachment is obsolete: true
Attachment #398747 - Flags: superreview?(neil)
Attachment #398747 - Flags: review?(neil)

Comment 8

9 years ago
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.
(Assignee)

Comment 9

9 years ago
(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
(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.
(Assignee)

Updated

9 years ago
Attachment #398747 - Flags: superreview?(neil)
Attachment #398747 - Flags: review?(neil)
(Assignee)

Comment 11

9 years ago
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?
Missing connection.beginTransaction()? Or for the purposes of the test you probably don't need either of the begin/endTransaction().
(Assignee)

Comment 13

9 years ago
Created attachment 399127 [details] [diff] [review]
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.
Attachment #398747 - Attachment is obsolete: true
Attachment #399127 - Flags: review?(neil)
(Assignee)

Updated

9 years ago
Attachment #399127 - Flags: superreview?(neil)
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.]
Attachment #399127 - Flags: superreview?(neil)
Attachment #399127 - Flags: superreview+
Attachment #399127 - Flags: review?(neil)
Attachment #399127 - Flags: review+
(Assignee)

Comment 15

9 years ago
Created attachment 400142 [details] [diff] [review]
Add separate privacy item for urlbar location with aCheckAll patch v0.1c

Revised patch, requesting r= to confirm it is correct.
Attachment #399127 - Attachment is obsolete: true
Attachment #400142 - Flags: superreview+
Attachment #400142 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #400142 - Flags: review? → review?(neil)
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.
Attachment #400142 - Flags: review?(neil) → review+
(Assignee)

Comment 17

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

Requesting a= for low risk patch.
Attachment #400142 - Attachment is obsolete: true
Attachment #400262 - Flags: superreview+
Attachment #400262 - Flags: review+
Attachment #400262 - Flags: approval-seamonkey2.0?

Updated

9 years ago
Attachment #400262 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
(Assignee)

Updated

9 years ago
Summary: "Clear Location Bar" section in History pane should be removed → URL Bar history should have a separate sanitize option
(Assignee)

Comment 18

9 years ago
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
Attachment #400262 - Attachment description: Add separate privacy item for urlbar location with reduced canClear patch v0.1d → Add separate privacy item for urlbar location with reduced canClear patch v0.1d [Checkin: Comment 18]
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: fixed-seamonkey2.0
Resolution: --- → FIXED
(Reporter)

Updated

9 years ago
Target Milestone: --- → seamonkey2.0

Updated

9 years ago
Duplicate of this bug: 202910
You need to log in before you can comment on or make changes to this bug.