Closed Bug 901952 Opened 9 years ago Closed 7 years ago

Clear Recent History - Everything does not clear RSS live bookmark history: visitedness stays until restart

Categories

(Firefox :: Bookmarks & History, defect, P4)

defect

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.1 - 13 Apr
Tracking Status
firefox40 --- verified

People

(Reporter: k1devnull, Assigned: chiragbhatia2006, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130730113002

Steps to reproduce:

1. Selected an item from a RSS feed (e.g. filehippo)
2. Firefox - History - Clear Recent History - Everything
3. No change to RSS visit indicator

I noticed this bug with all versions with RSS and Clear Recent History function.



Actual results:

No change to RSS visit indicator.


Expected results:

Clear RSS visit indicator
I suppose you mean a live bookmark?  Because visited link coloring seem to work the same way with http://www.opennet.ru/opennews/opennews_all.rss and with links on an ordinary web page.
Component: Untriaged → Bookmarks & History
Mozilla/5.0 (Windows NT 6.1; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130811030225

I can confirm this issue on the latest Nightly - the RSS visit indicator is not cleared unless Firefox is restarted after clearing everything in the history.

Steps to reproduce:
1. Navigate to http://filehippo.com/
2. Go to Bookmarks menu and select Subscribe to this page
3. Click on the new added RSS and visit some of the available feeds
4. Go to History menu -> Clear Recent History -> choose to clear Everything
5. Click on the Filehippo feeds 
6. Close Firefox
7. Launch Firefox.

The RSS visit indicator should change after step 4, not after step 7.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 23 Branch → Trunk
Confirmed with
2013-08-11-03-02-25-mozilla-central-firefox-26.0a1.en-US.linux-x86_64
OS: Windows 7 → All
Summary: Clear Recent History - Everything does not clear RSS history → Clear Recent History - Everything does not clear RSS live bookmark history: visitedness stays until restart
After "Clear Recent History", "Reload Live Bookmark" also clears indicator.  So the problem seems to be (automatic) synchronization, timing.
it may just be a visual bug without privacy implications, especially considered comment 4.
and I can confirm that:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsLivemarkService.js#434

onClearHistory should clear all visited statuses.
Whiteboard: [mentor=mak][lang=js]
Mentor: mak77
Whiteboard: [mentor=mak][lang=js] → [lang=js]
Hi,

I am interested on working with this bug. So please can you assign this to me?

Thanks in advance,

Regards,
Anup
Assignee: nobody → allamsetty.anup
Status: NEW → ASSIGNED
Hi mak,

I am sorry to say that I was not able to work on this bug for a while and I can continue working on this bug after a few days because now exams are going on for us. 

Sorry for the delay.

Regards,
Anup
no problem, thanks for the notice.
Anup, please let us know if you still want to fix this. I'm unassigning the bug in the meantime.
Assignee: allamsetty.anup → nobody
Status: ASSIGNED → NEW
Hardware: x86_64 → All
Blocks: 1102808
Flags: firefox-backlog+
Priority: -- → P4
Hi mak,

I'd like to work on this bug.

Also, did your link to code in comment 6 point to link this line? http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsLivemarkService.js#475
Flags: needinfo?(mak77)
yes, we need to handle the onClearHistory notification, when we get it, we must go through all livemarks (this._livemarks)  and do something like updateURIVisitedStatus, just not limited to a URI (that is the same thing but without the == aURI check)
Flags: needinfo?(mak77)
Ok,

here's what I've tried:

onClearHistory:     function () {
    for each (let livemark in this._livemarks) {
      dump(`${livemark}\n`);
    }
  },

The dump simply prints [object Object]. I'm guessing I need to put a
livemark.updateURIVisitedStatus(); inside the for each, but I'm not sure what you mean by 'not limited to a URI without the == aURI check'.
Flags: needinfo?(mak77)
don't use for each in, use for...of

what you need to do is "similar" to updateURIVisitedStatus, but that method takes an URI and compares it with each entry.
In your case instead you want to apply it to every entry, without comparing the uri (cause every uri is now unvisited).

The simpler way to do that, would be to copy updateURIVisitedStatus to updateVisitedStatus, remove the uri condition, and then use this new version.
But I would like to avoid having 2 very similar methods.
So the best way is probably to change conditions in updateURIVisitedStatus so that if !aURI it will apply the change regardless.

For example:
if (this.children[i].uri.equals(aURI)) {
would become
if (!aURI || this.children[i].uri.equals(aURI)) {

then you can use it as updateURIVisitedStatus(null, false); in your loop.
Flags: needinfo?(mak77)
Ok, I got it to work.

You can assign me the bug so that I can upload the patch.

Btw, any reason for using 'for..of' instead of 'for each in' in this case? We were neither using the keys nor values in the loop, so just wondering if there's any other advantage.
Flags: needinfo?(mak77)
(In reply to Chirag Bhatia from comment #15)
> Btw, any reason for using 'for..of' instead of 'for each in' in this case?
> We were neither using the keys nor values in the loop, so just wondering if
> there's any other advantage.

oh sorry, I thought this was an array for which we should always use for...of. for each in is ok in this case cause it's an object used as a Map. It should ideally be a Map...
Assignee: nobody → chiragbhatia2006
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
Attached patch livemarks.patch (obsolete) — Splinter Review
I ended up using for-each-in to keep the code consistent with the functions used just below the one that I modified, and also because for-of loop failed for me for some reason.

Let me know if I got anything wrong in the patch.
Attachment #8584505 - Flags: review?(mak77)
Comment on attachment 8584505 [details] [diff] [review]
livemarks.patch

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

Did you manually test that the patch does what it's expected to do?

Your steps to reproduce and verify the fix could also be useful for QA, so please report them.

::: toolkit/components/places/nsLivemarkService.js
@@ +809,5 @@
>      }
>    },
>  
>    updateURIVisitedStatus:
>    function LM_updateURIVisitedStatus(aURI, aVisitedStatus)

please add a javadoc above this method, something like

/**
 * Updates the visited status of nodes observing this livemark.
 *
 * @param aURI
 *        If provided will update nodes having the given uri,
 *        otherwise any node.
 * @param aVisitedStatus
 *        Whether the nodes should be set as visited.
 */

@@ +812,5 @@
>    updateURIVisitedStatus:
>    function LM_updateURIVisitedStatus(aURI, aVisitedStatus)
>    {
>      for (let i = 0; i < this.children.length; i++) {
> +      if (!aURI || this.children[i].uri.equals(aURI)) {

This is ok, but there's another if (node.uri == aURI.spec) check in this same function that needs to be updated.
Attachment #8584505 - Flags: review?(mak77)
Attached patch livemarks.patch (obsolete) — Splinter Review
Hi mak,

Nice catch on the if condition. I've fixed that and added the javadoc as you mentioned in your previous comment.

Steps to reproduce and verify the fix:

1. Go to http://filehippo.com
2. Go to Bookmarks menu and select Subscribe to this page
3. Click on the new added RSS and visit some of the available feeds
4. Change the clock time of the computer to atleast 2 hours ahead
5. Go to History menu -> Clear Recent History -> choose to clear Last Hour with all checkboxes selected
6. Make sure the clicked RSS feeds still show as visited as expected (since firefox thinks they were visited 2 hours ago) (this step is to make sure we are only marking the required links as visited)
7. Repeat step 5, but select Clear Everything instead of Last Hour
8. Make sure the clicked RSS feeds show up as not visited

Let me know if any more fixes are needed.
Attachment #8584636 - Flags: review?(mak77)
Comment on attachment 8584636 [details] [diff] [review]
livemarks.patch

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

When posting a new patch, you can usually "obsolete" the previous version of it (there's a checkbox in the upload form)

::: toolkit/components/places/nsLivemarkService.js
@@ +473,5 @@
>    onTitleChanged:     function () {},
>    onDeleteVisits:     function () {},
> +
> +  // Check visited status of livemarks after history is cleared
> +  onClearHistory:     function () {

please just:
onClearHistory() {

@@ +822,1 @@
>    updateURIVisitedStatus:

please remove the newline before the name

@@ +834,5 @@
>        if (this._nodes.has(container)) {
>          let nodes = this._nodes.get(container);
>          for (let j = 0; j < nodes.length; j++) {
>            let node = nodes[j];
> +          if (aURI && (node.uri == aURI.spec)) {

nope, this should be if (!aURI || node.uri == aURI.spec)
Cause if aURI is not provided we want to update regardless...
Attachment #8584636 - Flags: review?(mak77)
Attached patch livemarks.patchSplinter Review
Here you go, I've made the changes you mentioned.

Btw, any reason for choosing 
onClearHistory() {
over
onClearHistory: function () {
?
Attachment #8584505 - Attachment is obsolete: true
Attachment #8584636 - Attachment is obsolete: true
Attachment #8585040 - Flags: review?(mak77)
(In reply to Chirag Bhatia from comment #22)
> Btw, any reason for choosing 
> onClearHistory() {
> over
> onClearHistory: function () {
> ?

nothing serious, it is a more compact (and newly available) syntax.
Comment on attachment 8585040 [details] [diff] [review]
livemarks.patch

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

Yes, now it looks good, thank you!
Attachment #8585040 - Flags: review?(mak77) → review+
https://hg.mozilla.org/integration/fx-team/rev/ce0eabb6eb5a
Keywords: checkin-needed
Whiteboard: [lang=js] → [lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ce0eabb6eb5a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][fixed-in-fx-team] → [lang=js]
Target Milestone: --- → Firefox 40
Iteration: --- → 40.1 - 13 Apr
Flags: qe-verify?
since we have steps in comment 20 let's verify this.
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
Verified as fixed using Firefox 40 beta 4 under Win 7 x64, Ubuntu 14.04 x86 and Mac OSX 10.9.5 and comment 20.

(In reply to Chirag Bhatia from comment #20)
> Steps to reproduce and verify the fix:
> 
> 1. Go to http://filehippo.com
> 2. Go to Bookmarks menu and select Subscribe to this page
> 3. Click on the new added RSS and visit some of the available feeds
> 4. Change the clock time of the computer to atleast 2 hours ahead
> 5. Go to History menu -> Clear Recent History -> choose to clear Last Hour
> with all checkboxes selected
> 6. Make sure the clicked RSS feeds still show as visited as expected (since
> firefox thinks they were visited 2 hours ago) (this step is to make sure we
> are only marking the required links as visited)
> 7. Repeat step 5, but select Clear Everything instead of Last Hour
> 8. Make sure the clicked RSS feeds show up as not visited

One question here, though: After step 4, if I open RSS feed, they appear as not visited and quickly are switched back to visited. Opening the RSS feed again doesn't show this anymore. Could this be related to the fix? Thanks
Flags: needinfo?(chiragbhatia2006)
(In reply to Petruta Rasa [QA] [:petruta] from comment #29)

> One question here, though: After step 4, if I open RSS feed, they appear as
> not visited and quickly are switched back to visited.

At step 4 history has not been cleard yet, so it's ok if they are updated asynchronously (history is asynchronous). the important thing is the final status.
Thanks, Marco! Marking as verified!
Status: RESOLVED → VERIFIED
Flags: needinfo?(chiragbhatia2006)
You need to log in before you can comment on or make changes to this bug.