Closed Bug 1020339 Opened 10 years ago Closed 10 years ago

Add UI for disabling and clearing Visited Link/Browsing History

Categories

(Thunderbird :: Preferences, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 33.0

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #920118 +++

With bug 920118 establishing the backend for using Places to register links opened in an external browser, some additions to the UI are needed:

 - sanitize.xul needs to be extended for "Visited Link History"
 - privacy.xul should get some option "Remember links I've followed"

Hopefully this will be mostly copy-pasting from the respective Firefox implementation.
Attached patch Proposed patch (obsolete) — Splinter Review
Indeed, the backend part for sanitize.js could be simply copy-pasted from Firefox's "history:" implementation and it works out of the box. I don't know what exactly the "os" and "seer" functions are doing, but they are executed without any exceptions, thus I assume that they either do something useful or nothing as necessary. Removal of partial history by time works as well, links older than the specified threshold are retained as desired.

The labels reflect that both websites (e.g., for "Learn more" tabs or with an extension like Thunderbrowse installed) as well as visited links can set an entry for the history. "Browsing history" isn't entirely correct, though visiting a link could be considered browsing as well.

Drive-by fixes: There were several "\r\n" Windows line endings in sanitize.* which I converted to "\n" as required. If the patch doesn't apply, remove all "\r" from both the patch and the two affected files and it should work.

Secondly, the "A" accesskey was assigned twice in the Privacy pane, I have resolved that.
Attachment #8434636 - Flags: ui-review?(bwinton)
Attachment #8434636 - Flags: review?(mkmelin+mozilla)
Attached image Screenshots
Privacy preference pane and Sanitize dialog.
Note: For the patch to fully function, apply bug 920118 attachment 8433815 [details] [diff] [review] first (obviously).
Status: NEW → ASSIGNED
Comment on attachment 8434636 [details] [diff] [review]
Proposed patch

>+          var seer = Components.classes["@mozilla.org/network/seer;1"]
>+                               .getService(Components.interfaces.nsINetworkSeer);
>+          seer.reset();

And Firefox has just changed that part in bug 1016622, which means I'll have to post an updated patch.  :-\
Attached patch Unbitrotted patch (v2) (obsolete) — Splinter Review
Updated against tip, modified for name changes made by bug 1016622 and white-space cleanup by bug 1015779.
Attachment #8434636 - Attachment is obsolete: true
Attachment #8434636 - Flags: ui-review?(bwinton)
Attachment #8434636 - Flags: review?(mkmelin+mozilla)
Attachment #8435790 - Flags: ui-review?(bwinton)
Attachment #8435790 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8435790 [details] [diff] [review]
Unbitrotted patch (v2)

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

I'm not sure we actually need the "enable history" checkbox in the UI. For a browser there are very different use cases... 
If we have the option to clear in Clear History dialog + it can disabled from the config editor, so that should be enough IMHO. The option is well hidden in Firefox too.

::: mail/locales/en-US/chrome/messenger/sanitize.dtd
@@ +23,5 @@
>       480169 -->
>  <!ENTITY detailsProgressiveDisclosure.label     "Details">
>  <!ENTITY detailsProgressiveDisclosure.accesskey "e">
>  
> +<!ENTITY itemHistory.label                 "Website &amp; Visited Link History">

Maybe we could use "Browsing History" or "Visited Links History"
So if history is enabled in Tb, I'd like to convert feed favicons to use the places db for permanent storage, if clearing history is exposed.  

I also think the privacy tab for Tb should have exactly the same UI as Fx (the non mail specific parts).  Not just for feeds but any web content opened in a content tab, and user familiarity and unity of product as well.
(In reply to Magnus Melin from comment #6)
> I'm not sure we actually need the "enable history" checkbox in the UI. For a
> browser there are very different use cases... 

It's a privacy issue in either case (user actions tracked and written to the disk), so...

> If we have the option to clear in Clear History dialog + it can disabled
> from the config editor, so that should be enough IMHO. The option is well
> hidden in Firefox too.

It is on the same level as the cache and cookie settings in Firefox, "hidden" in a sense that you need to enable the custom configuration first to see the individual options. In Thunderbird we are already offering the cookie and DNT settings in the Privacy tab (which are strictly web-related as well). Thus, it appears that the history deserves the same rank.

I would still offer it in the preferences UI.

> Maybe we could use "Browsing History" or "Visited Links History"

As said, I had a bit trouble to find a nice label here myself, given that it may be a "true" browsing history (e.g., with Thunderbrowse or a similar extension installed) or just the history of links followed from messages. I'm fine with "Visited Links History" as it expresses the main function of the feature in the Thunderbird context (is it "Links" or "Link" though?; some argue that "Add-ons Manager" is actually bad grammar and should be relabeled back to "Add-on Manager").
(In reply to alta88 from comment #7)
> So if history is enabled in Tb, I'd like to convert feed favicons to use the
> places db for permanent storage, if clearing history is exposed.  

Sorry, I'm not using Feeds and thus don't know what this implies. It sounds like something that should go into a separate bug, where it's not clear to me what the implications to the preferences or sanitize dialog are (if any, or was this just a general comment?).

> I also think the privacy tab for Tb should have exactly the same UI as Fx
> (the non mail specific parts).  Not just for feeds but any web content
> opened in a content tab, and user familiarity and unity of product as well.

Firefox has three top-level settings in their Privacy pane:

 - Remember history (default, everything is enabled)
 - Never remember history (private browsing mode, which Thunderbird doesn't have)
 - Use custom settings (present all options to pick and choose)

With the second option not being applicable, I don't see the benefit of requiring the use to go through another step of changing the menu to see the options as they are. Also, since we don't have a PB mode, that top-level checkbox isn't applicable, thus the list of options is easier even with the checkbox for the browsing/visited-link(s) history added.
(In reply to rsx11m from comment #9)
> (In reply to alta88 from comment #7)
> > So if history is enabled in Tb, I'd like to convert feed favicons to use the
> > places db for permanent storage, if clearing history is exposed.  
> 
> Sorry, I'm not using Feeds and thus don't know what this implies. It sounds
> like something that should go into a separate bug, where it's not clear to
> me what the implications to the preferences or sanitize dialog are (if any,
> or was this just a general comment?).
> 

A general comment to note that enabling history will allow other use cases which had avoided permanent storage (favicons), due to lack of UI for user history clearing.  It appears you're implementing that UI so that's great.

> > I also think the privacy tab for Tb should have exactly the same UI as Fx
> > (the non mail specific parts).  Not just for feeds but any web content
> > opened in a content tab, and user familiarity and unity of product as well.
> 
> Firefox has three top-level settings in their Privacy pane:
> 
>  - Remember history (default, everything is enabled)
>  - Never remember history (private browsing mode, which Thunderbird doesn't
> have)
>  - Use custom settings (present all options to pick and choose)
> 
> With the second option not being applicable, I don't see the benefit of
> requiring the use to go through another step of changing the menu to see the
> options as they are. Also, since we don't have a PB mode, that top-level
> checkbox isn't applicable, thus the list of options is easier even with the
> checkbox for the browsing/visited-link(s) history added.

I meant that all the (non PB) items in the Use Custom Settings menuitem should be included, as well as all the items in Clear History dialog.  Enabling history triggers storing of any loaded url's traces, such as in Tb content tabs or feed web pages; this means things like form history could be kept around, etc. in addition to history visits.  Some of this will be unexpected by users in a 'mail' client and making it look familiar is better.

By enabling history, there's a good argument to be made for implementing Private Browsing as well.  Note that the ThunderBrowse extension is reasonably popular.
Good point, but let's not overburden this bug. Going through the list:

 - Browsing history is covered by the current patch.
 - Download history is deleted when exiting Thunderbird.
 - Location bar history isn't applicable due to the lack of such a bar.
 - Saved form history is enabled per bug 510378 and may be relevant.

The next items shouldn't be kept in Places and thus aren't affected:

 - Cache and Cookies are already available.
 - Offline Website Data, I don't think offline-apps are supported... are they?
   If yes, they are enabled by default without prompt now per bug 892488.
 - Saved passwords, that probably wouldn't change by using Places elsewhere.
 - Authenticated Sessions, may not be applicable (remote content in a message?).
 - Site preferences, not sure how those may be used (downloading remote content?).

Supporting Private Browsing in Thunderbird would certainly be a separate bug.

The Firefox code for clearing the form and search history doesn't look trivial and is located at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#238 - it has a couple of browser-window specific operations there, so this may not work out of the box.

What exactly is the use case here, independent from using an add-on like Thunderbrowse? There are only limited cases where a content tab is opened (and usually we have control over their content, unless the user decides to pick a custom website as start page). Those aren't really intended to continue browsing in that tab or entering form data (don't know how feeds or chat are using it). Links are usually opening in the default browser instead.
(In reply to rsx11m from comment #11)
> Good point, but let's not overburden this bug. Going through the list:
> 
>  - Browsing history is covered by the current patch.
>  - Download history is deleted when exiting Thunderbird.
>  - Location bar history isn't applicable due to the lack of such a bar.
>  - Saved form history is enabled per bug 510378 and may be relevant.
> 
> The next items shouldn't be kept in Places and thus aren't affected:
> 
>  - Cache and Cookies are already available.
>  - Offline Website Data, I don't think offline-apps are supported... are
> they?
>    If yes, they are enabled by default without prompt now per bug 892488.
>  - Saved passwords, that probably wouldn't change by using Places elsewhere.
>  - Authenticated Sessions, may not be applicable (remote content in a
> message?).
>  - Site preferences, not sure how those may be used (downloading remote
> content?).

Site preferences aren't affected by history being enabled.  It refers to things like per site download locations (works in Tb), per site zoom levels (Bug 458948) and char encoding (don't work).  You can inspect content-prefs.sqlite with SQLite Manager extension for more.

> 
> Supporting Private Browsing in Thunderbird would certainly be a separate bug.
> 

Of course.

> The Firefox code for clearing the form and search history doesn't look
> trivial and is located at
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.
> js#238 - it has a couple of browser-window specific operations there, so
> this may not work out of the box.
> 
> What exactly is the use case here, independent from using an add-on like
> Thunderbrowse? There are only limited cases where a content tab is opened
> (and usually we have control over their content, unless the user decides to
> pick a custom website as start page). Those aren't really intended to
> continue browsing in that tab or entering form data (don't know how feeds or
> chat are using it). Links are usually opening in the default browser instead.

It would be an edge case, in a clean profile, but it can happen.  It doesn't need to be in this bug.

The idea was to inventory ramifications and UI needed for turning on history, which you've done.  If a particular piece is an easy copy/paste to port, it's probably a good future proof idea to include it, imo.  The details are left to your judgement.

I use an extension to open links in tabs and couldn't do without it.  I think we should implement opening links in a Tb tab natively; links in mails and so forth seem to be a large point of support churn.  Most link opens are likely fast one offs that are far more comfortably done in a Tb tab than another browser window.  So there certainly could be stronger default use cases in future.
Blocks: 1026068
Comment on attachment 8435790 [details] [diff] [review]
Unbitrotted patch (v2)

Seems good, based on the screenshots…
Attachment #8435790 - Flags: ui-review?(bwinton) → ui-review+
Thanks, Blake.

Magnus, given that bug 920118 is already in the checkin queue (and alta88 opened bug 1026068 for the favicon issue), I'd stick with the current patch covering visited link history except for the label change (comment #8 last paragraph, which label do you prefer?). We can add the form history in a next cycle if it's deemed useful (should be doable but may need some work).
Comment on attachment 8435790 [details] [diff] [review]
Unbitrotted patch (v2)

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

Ok, I'd go with "Browsing History". r=mkmelin
Attachment #8435790 - Flags: review?(mkmelin+mozilla) → review+
Attached patch Final patch (v3)Splinter Review
Ok, sanitize.dtd so changed.
Attachment #8435790 - Attachment is obsolete: true
Attachment #8451132 - Flags: ui-review+
Attachment #8451132 - Flags: review+
Keywords: checkin-needed
Summary: Add UI for disabling and clearing Visited Link history → Add UI for disabling and clearing Visited Link/Browsing History
https://hg.mozilla.org/comm-central/rev/ef74c5454b5d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
(In reply to Magnus Melin from comment #15)
> Comment on attachment 8435790 [details] [diff] [review]
> Unbitrotted patch (v2)
> 
> Review of attachment 8435790 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, I'd go with "Browsing History". r=mkmelin

except that this option will also clear Downloads.  the firefox string is 'Browsing & Download History'.
No, it doesn't. I've double-checked this before posting the patch.

Firefox has just combined those two items to a single checkbox and a single entry in sanitize.xul (they are still separate in SeaMonkey's implementation).

Thunderbird deletes download history on exiting based on the different default retention setting, http://mxr.mozilla.org/comm-central/search?string=browser.download.manager.retention&find=\.js
Download history is cleared in Firefox based on the following function (not ported here): http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#308

and the underlying clear-private-data preference settings a kept in sync by Firefox simply by this: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitizeDialog.js#203

(In reply to rsx11m from comment #19)
> No, it doesn't. I've double-checked this before posting the patch.

I opened a link in a content tab, clicked Save As on a random image or link, the download dialog appeared with a successful download.  I could also see the item in downloads.sqlite.

Then the Browser History was cleared.  The item disappeared from the downloads dialog and from downloads.sqlite.

So I'm afraid it does.
Then I don't know what's causing this. Please open a new bug report.
Depends on: 1037986
Magnus, I don't think we've fully vetted the ramifications of turning on history.  This action should be announced, transparently, in tb-planning and discussed if necessary.
I had opened a thread in http://forums.mozillazine.org/viewtopic.php?f=29&t=2851181 after the backend patch checked in, already with a couple of posts regarding future plans and other items to add or make accessible in the Privacy preferences.

As said, bug 920118 won't hit the 31.x releases, we are 33.0 now, thus have time (30 weeks) to get it done right. In the worst case, the default for enabling history could be switched back to not using it for the releases if there are doubts that it's sufficiently working for all cases.

Since we have at least three bugs now on history usage with likely more to come, it should also be worth to create a meta bug to properly follow these developments.
Blocks: tb-places
If you have any concerns, by all means, discuss them in tb-planning.
(In reply to Magnus Melin from comment #25)
> If you have any concerns, by all means, discuss them in tb-planning.

When a major new feature implementation is considered in Fx for example, the requestor posts an Intent to Implement to m.d.platform where it is publicly discussed.  Before the implementation.  Not many decisions in Tb need to rise to that level, but this one does.  And I'm sure we all agree a meta bug in bugzilla in not the correct venue for such a policy topic.

If this change becomes more scrutinized, it will be correctly noted that it was done without prior notice or even senior review.
Again, this is not the place to Monday-morning quarterback the decision to enable Places (which was bug 920118, by the way, not this one; here, only the front-end additions were implemented). Places as a feature itself has been included with Thunderbird for quite a while, just wasn't enabled by default. The motivation for enabling it at this time was to allow visited links to be marked as such (bug 920118 comment #12 and following), nothing more was intended at that point.

As stated in comment #24, the advantage of being on the ESR branch with the releases is that we have time to investigate the can of worms (if any was opened) and to take care of it, and there is an easy fix if we don't want this to hit the releases (while retaining UI options to enable it for users who are interested).
You continue not to get it, so I will be more direct: as the driver of this change, you are requested to post notice that places history has been enabled to tb-planning, which *is* the correct place for sunday *and* monday quarterbacking.  

Are you refusing to do so?  Do you not feel it should be more widely known/discussed among Thunderbird peers?
So, go ahead already and open a thread yourself where you think more discussion is necessary since you are the one voicing the concerns here. My main venue is mozillaZine, and that's where I've opened an announcement thread and listen to the feedback. I don't intend to discuss anything further in this bug report given that it's closed.
Sent a note to tb-planning. I don't know what concerns you have, so discussion is open.
Thanks.
Since this has reached now the aurora channel, and thus a wider audience, I've posted a somewhat more detailed note with references to the other locations of discussion and the meta bug to the newsgroup:

https://groups.google.com/forum/#!topic/mozilla.dev.apps.thunderbird/hlNvdAe62LQ
You need to log in before you can comment on or make changes to this bug.