Closed Bug 386603 Opened 13 years ago Closed 13 years ago

Reload live bookmark does not actually show the feed being refreshed

Categories

(Firefox :: Bookmarks & History, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3 alpha8

People

(Reporter: marcia, Assigned: stevewon)

References

Details

(Keywords: regression)

Attachments

(1 file, 5 obsolete files)

Seen using the A6 candidate, Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a6) Gecko/2007062911 GranParadiso/3.0a6.

STR
1. Add a feed, such as livejournal.com
2. Go to Organize bookmarks and right click to expose the context menu. Select Reload Live Bookmark.

Although the live bookmarks updates, I don't see the "refresh" messaging (Live Bookmark loading) that I see on the 2.0.0.x branch.
RSS D&P only... discovers and previews ;) - off to Places, whose department it is once you livemark it.
Component: RSS Discovery and Preview → Places
QA Contact: rss.preview → places
Keywords: regression
Target Milestone: --- → Firefox 3 beta1
Where is the "refresh" messaging supposed to appear?
I can't seem to find that message in fx2 while trying with planet.mozilla.org feed.
I usually see it right below the top post on the feed (mozillazine.org is another one to try, I am also refreshing in Organize Bookmarks).  Also, it doesn't seem to show the "refresh" action like it does in 2.0.0.x, which I think is important visual feedback for the user.

(In reply to comment #2)
> Where is the "refresh" messaging supposed to appear?
> I can't seem to find that message in fx2 while trying with planet.mozilla.org
> feed.
> 

Can I get a screenshot, if there is one?
Thank you.
Steve: I will try, hard to capture it with grab even with the timer. I will also have to compare the MBP machine I am running on now, which is blazing compared to the slower machines I have. That could be one reason why I am not seeing it.

(In reply to comment #4)
> Can I get a screenshot, if there is one?
> Thank you.
> 

Maybe I can make it a little more clear, with some help from http://dev.philringnalda.com/livemarkreload/ which waits 20 seconds before it loads to give you plenty of time to see what's happening.

STR:

1. In a 2.0.0.x build, load http://dev.philringnalda.com/livemarkreload/
2. Add a livemark to the toolbar
3. Click the newly added livemark, to open the menu while it loads

At this point, you'll see a "Live bookmark loading..." item, then when it loads you'll see three items (minus the Open in tabs item, bug 257285). Close and reopen, you'll see the three items, the top one titled with the time of your fetch, plus Open in tabs.

4. Right click the livemark folder, select Reload Live Bookmark, then click the folder to open the menu while it loads

At this point, you'll only see the Live Bookmark loading... item, because 2.0.0.x removed all items before reloading, which was a misfeature that left the livemark empty if a reload failed, but you'll know it's reloading. Once it loads, you'll see the three items, with the first item having a new time.

5. Open the Bookmarks Manager, and in the right pane open the livemark folder
6. Right click the livemark folder, select Reload Live Bookmark

At this point, things will proceed as with the toolbar - only a Live Bookmark loading... item (with a tacky "about:livemark-loading" for a location), then the three items, with a new time for the first item.

Then:

1. In a trunk Places build, load http://dev.philringnalda.com/livemarkreload/
2. Add a livemark to the toolbar
3. Click the newly added livemark, to open the menu while it loads

At this point, things are still the same, Live Bookmark loading... eventually replaced by items.

4. Right click the livemark folder, select Reload Live Bookmark, then click the folder to open the menu while it loads

At this point, if you had a Places build from last spring, as bug 330867 says you would have the existing items (an improvement over 2.0.0.x) plus a Live Bookmark loading... item at the bottom to tell you it's loading. However, with a current build, you just have to guess at whether you haven't seen any changes because the feed hasn't changed, or because it hasn't loaded yet, because there's nothing to tell you it's loading.

5. Open the Bookmarks Manager, and in the right pane open the livemark folder
6. Right click the livemark folder, select Reload Live Bookmark

Again, note the lack of any visual indication whether it's loading, or has loaded unchanged, or checked the cache expiration time and decided there was no point in trying to load.

So, 2.0 had the bad habit of removing items, but told you it was reloading, Places Mark I kept the items, but also told you it was reloading, which silver didn't like, and Places Mark II keeps the items, but doesn't tell you it's reloading, and Marcia wants it back like Mark I. Me, I mostly want it to be an actual UI decision, rather than happenstance of the new code.
Flags: blocking-firefox3?
Ah, thank you for the clarification and a test link!
Assignee: nobody → swon
Yeah, this blocks, mostly because in cases where the user has large network latency or has gone offline, we need to show something in the drop-down.

Without adding any new icons or throbbers (which would get pretty complicated and visually probably a little too messy), I think we should ..:

 1. when refreshing, show "Refreshing Live Bookmark..."
 2. it would be nice if new items were contributed to the popup even while it's open, so that the user could *see* it refreshing instead of having to close and re-open
 3. if we can't do #2, then we should put "Refreshing Live Bookmark..." at the top

Also, if we hit the timeout, we should make sure we're still showing "Live Bookmark failed to load" or some indication of that failure.
Flags: blocking-firefox3? → blocking-firefox3+
Re: Beltzner

When a rss feed is first created, "Live Bookmark loading..." is displayed. 
When refreshing in fx2, "Live Bookmark loading..." is shown also.

Should I be using "Live Bookmark loading..." or "Refreshing Live Bookmark..."?
Thank you.
Status: NEW → ASSIGNED
Currently in nsLivemarksService.js, when "Reload live bookmark" is called, there is a check to see if there is a need to refresh or not. Even if the user clicks "Reload live bookmark", it may not be refreshing. 

So,
1. Should the check in nsLivemarksService.js be removed or modified so that the live bookmark is actually refreshed when user clicks "Reload live bookmark"?

or

2. Should there be a visual notification indicating that the refreshing has finished or that there is no need to refresh?

(In reply to comment #9)
> Should I be using "Live Bookmark loading..." or "Refreshing Live Bookmark..."?
> Thank you.

Actually, "Loading Live Bookmark ..."

> 1. Should the check in nsLivemarksService.js be removed or modified so that the
> live bookmark is actually refreshed when user clicks "Reload live bookmark"?

Yes, I think it should be, we should force our way through the cache.

> 2. Should there be a visual notification indicating that the refreshing has
> finished or that there is no need to refresh?

Nothing beyond the labels that we're injecting, no.
Whiteboard: [swag: 1d]
Attached patch Patch (obsolete) — Splinter Review
Attachment #273051 - Flags: review?(dietrich)
Whiteboard: [swag: 1d] → [swag: 1d][need review dietrich]
Comment on attachment 273051 [details] [diff] [review]
Patch


>@@ -230,16 +233,19 @@ LivemarkService.prototype = {
>       var uriChannel = gIoService.newChannel(livemark.feedURI.spec, null, null);
>       uriChannel.loadGroup = loadgroup;
>       uriChannel.loadFlags |= Ci.nsIRequest.LOAD_BACKGROUND | 
>                               Ci.nsIRequest.VALIDATE_ALWAYS;
>       var httpChannel = uriChannel.QueryInterface(Ci.nsIHttpChannel);
>       httpChannel.requestMethod = "GET";
>       httpChannel.setRequestHeader("X-Moz", "livebookmarks", false);
> 
>+      this.deleteLivemarkChildren(livemark.folderId);
>+      this._loadingId = this.insertLivemarkLoadingItem(this._bms, livemark.folderId);
>+
>       // Stream the result to the feed parser with this listener
>       var listener = new LivemarkLoadListener(livemark);
>       httpChannel.asyncOpen(listener, null);
>     }
>     catch (ex) {
>       livemark.locked = false;
>       LOG("exception: " + ex);
>       throw ex;

this implements the Fx2 functionality (remove all items, insert the loading item).

however, as some of the comments on this bug pointed out, i think the right fix is to instead leave the current items, and insert the loading item as the first item. you can do this by setting the position to 0 when inserting the loading item. then, clear the old items only once we know the feed was fetched successfully.

>@@ -457,20 +463,20 @@ LivemarkLoadListener.prototype = {
>     var result = aUserData.QueryInterface(Ci.nsIFeedResult);
> 
>     // We need this to make sure the item links are safe
>     var secMan = Cc[SEC_CONTRACTID].getService(Ci.nsIScriptSecurityManager);
>       
>     // Clear out any child nodes of the livemark folder, since
>     // they're about to be replaced.
>     var lmService = Cc[LS_CONTRACTID].getService(Ci.nsILivemarkService);
>-    this.deleteLivemarkChildren(this._livemark.folderId);
> 
>     // Enforce well-formedness because the existing code does
>     if (!result || !result.doc || result.bozo) {
>+      this.deleteLivemarkChildren(this._livemark.folderId);
>       this.insertLivemarkFailedItem(this._livemark.folderId);
>       this._ttl = gExpiration;
>       throw Cr.NS_ERROR_FAILURE;
>     }
> 
>     var title, href, entry;
>     var feed = result.doc.QueryInterface(Ci.nsIFeed);
>     // Loop through and check for a link and a title

first, you could have made this more efficient by removing the loading item specifically (since you cleared the folder before making the request, you know that it's the only item there). however, now that we're going to leave the old items there during loading (and in the event of failure), we certainly want to remove only the loading item, not all the children of the folder.

secondly, should we modify insertLivemarkFailedItem to insert the failed item as the first item in the folder (as we're going to do w/ the loading item)?
Attachment #273051 - Flags: review?(dietrich) → review-
(In reply to comment #11)
> > 1. Should the check in nsLivemarksService.js be removed or modified so that the
> > live bookmark is actually refreshed when user clicks "Reload live bookmark"?
> 
> Yes, I think it should be, we should force our way through the cache.

can you either add this fix to your patch if it's trivial to do, or file a follow up on this?
>secondly, should we modify insertLivemarkFailedItem to insert the failed item
>as the first item in the folder (as we're going to do w/ the loading item)?

That sounds like a better way than appending to the end, since we wouldn't be clearing the livemark folder anymore.
Attached patch Patch (obsolete) — Splinter Review
In this patch...
* Old children of a livemark are removd after the update children are inserted.
* "Live Bookmark loading..." still appears on top.
* insertLivemarkFailedItem inserts the bookmark on top instead of appending it. Refer to comment #15.
*
(In reply to comment #14)
> (In reply to comment #11)
> > > 1. Should the check in nsLivemarksService.js be removed or modified so that the
> > > live bookmark is actually refreshed when user clicks "Reload live bookmark"?
> > 
> > Yes, I think it should be, we should force our way through the cache.
> 
> can you either add this fix to your patch if it's trivial to do, or file a
> follow up on this?
> 

I will file a follow up on this after looking at it a bit longer to understand the problem better.
Attachment #273051 - Attachment is obsolete: true
Attachment #273661 - Flags: review?(dietrich)
Comment on attachment 273661 [details] [diff] [review]
Patch

Index: toolkit/components/places/src/nsLivemarkService.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.js,v
>retrieving revision 1.19
>diff -u -8 -p -r1.19 nsLivemarkService.js
>--- toolkit/components/places/src/nsLivemarkService.js	11 Jul 2007 09:58:00 -0000	1.19
>+++ toolkit/components/places/src/nsLivemarkService.js	24 Jul 2007 22:10:12 -0000
>@@ -114,22 +114,24 @@ function LivemarkService() {
>     var prefs = Cc[PS_CONTRACTID].getService(Ci.nsIPrefBranch);
>     var livemarkRefresh = 
>       prefs.getIntPref("browser.bookmarks.livemark_refresh_seconds");
>     // Reset global expiration variable to reflect hidden pref (in ms)
>     // with a lower limit of 1 minute (60000 ms)
>     gExpiration = Math.max(livemarkRefresh * 1000, 60000);
>   } 
>   catch (ex) { }
>-    
>+
>+  gLivemarkService = this;
>   // [ {folderId:, folderURI:, feedURI:, loadGroup:, locked: } ];
>   this._livemarks = [];
> 
>   this._iconURI = gIoService.newURI(LIVEMARK_ICON_URI, null, null);
>   this._loading = GetString("bookmarksLivemarkLoading") || DEFAULT_LOAD_MSG;
>+  this._loadingId = -1;

hrm, i missed this last time around, but the loading id is specific to each livemark. it cannot be a property of the service itself. it should be a property of the livemark. for example: if 2 livemarks are being updated, and one has super high latency, then the loadingId for the first might be used/removed/overwritten by the other.

>@@ -187,18 +189,24 @@ LivemarkService.prototype = {
>   },
> 
>   deleteLivemarkChildren: function LS_deleteLivemarkChildren(folderId) {
>     this._bms.removeFolderChildren(folderId);
>   },
> 
>   insertLivemarkLoadingItem: function LS_insertLivemarkLoading(bms, folderId) {
>     var loadingURI = gIoService.newURI("about:livemark-loading", null, null);
>-    var id = bms.insertBookmark(folderId, loadingURI, bms.DEFAULT_INDEX,
>-                                this._loading);
>+    var idArr = bms.getBookmarkIdsForURI(loadingURI, {});
>+    var id;
>+    if (idArr.length > 0)
>+      id = idArr[0];
>+    else
>+      id = bms.insertBookmark(folderId, loadingURI, 0, this._loading);
>+
>+    return id;
>   },
> 

this doesn't work, as getBookmarkIdsForURI is not specific to a particular folder.

i think it's ok to just insert here, and manage removal in the load listener.

>@@ -213,33 +221,35 @@ LivemarkService.prototype = {
>       if (!forceUpdate && exprTime > Date.now()) {
>         // no need to refresh
>         livemark.locked = false;
>         return;
>       }
>     } 
>     catch (ex) {
>       // This livemark has never been loaded, since it has no expire time.
>-      this.insertLivemarkLoadingItem(this._bms, livemark.folderId);
>+      this._loadingId = this.insertLivemarkLoadingItem(this._bms, livemark.folderId);
>     }

again, livemark.loadingId instead of this._loadingId

> 
>     var loadgroup;
>     try {
>       // Create a load group for the request.  This will allow us to
>       // automatically keep track of redirects, so we can always
>       // cancel the channel.
>       loadgroup = Cc[LG_CONTRACTID].createInstance(Ci.nsILoadGroup);
>       var uriChannel = gIoService.newChannel(livemark.feedURI.spec, null, null);
>       uriChannel.loadGroup = loadgroup;
>       uriChannel.loadFlags |= Ci.nsIRequest.LOAD_BACKGROUND | 
>                               Ci.nsIRequest.VALIDATE_ALWAYS;
>       var httpChannel = uriChannel.QueryInterface(Ci.nsIHttpChannel);
>       httpChannel.requestMethod = "GET";
>       httpChannel.setRequestHeader("X-Moz", "livebookmarks", false);
> 
>+      this._loadingId = this.insertLivemarkLoadingItem(this._bms, livemark.folderId);
>+

ditto


>@@ -264,17 +274,17 @@ LivemarkService.prototype = {
>     return livemarkID;
>   },
> 
>   createLivemarkFolderOnly:
>   function LS_createLivemarkFolderOnly(bms, folder, name, siteURI,
>                                        feedURI, index) {
>     var livemarkID = this._createFolder(bms, folder, name, siteURI, feedURI,
>                                         index);
>-    this.insertLivemarkLoadingItem(bms, livemarkID);
>+    this._loadingId = this.insertLivemarkLoadingItem(bms, livemarkID);
>     this._pushLivemark(livemarkID, feedURI);

ditto

>@@ -457,26 +467,30 @@ LivemarkLoadListener.prototype = {
>     var result = aUserData.QueryInterface(Ci.nsIFeedResult);
> 
>     // We need this to make sure the item links are safe
>     var secMan = Cc[SEC_CONTRACTID].getService(Ci.nsIScriptSecurityManager);
>       
>     // Clear out any child nodes of the livemark folder, since
>     // they're about to be replaced.
>     var lmService = Cc[LS_CONTRACTID].getService(Ci.nsILivemarkService);
>-    this.deleteLivemarkChildren(this._livemark.folderId);
> 

so, instead of removeOldChildren(), i think it'd be fine to instead just move this call to after the parse-error-handling block below. ie: it's fine to remove all the children after the point at which we know the feed was requested successfully and is parse-able.

>     // Enforce well-formedness because the existing code does
>     if (!result || !result.doc || result.bozo) {
>+      if (gLivemarkService._loadingId != -1) {
>+        this._bms.removeItem(gLivemarkService._loadingId);
>+        gLivemarkService._laadingId = -1;
>+      }
>+

typo, and should be a property of the livemark not the service

>       this.insertLivemarkFailedItem(this._livemark.folderId);
>       this._ttl = gExpiration;
>       throw Cr.NS_ERROR_FAILURE;
>     }
> 
>-    var title, href, entry;
>+    var title, href, entry, firstIndex;
>     var feed = result.doc.QueryInterface(Ci.nsIFeed);
>     // Loop through and check for a link and a title
>     // as the old code did
>     for (var i = 0; i < feed.items.length; ++i) {
>       entry = feed.items.queryElementAt(i, Ci.nsIFeedEntry);
>       if (entry.title)
>         title = entry.title.plainText();
>       else if (entry.updated)
>@@ -486,19 +500,37 @@ LivemarkLoadListener.prototype = {
>         try {
>           secMan.checkLoadURIStr(this._livemark.feedURI.spec, entry.link.spec,
>                                  SEC_FLAGS);
>           href = entry.link;
>         }
>         catch (ex) { }
>       }
> 
>-      if (href && title)
>-        this.insertLivemarkChild(this._livemark.folderId, href, title);
>+      if (href && title) {
>+        var thisId = this.insertLivemarkChild(this._livemark.folderId, href, title);
>+        if (i == 0)
>+          firstIndex = this._bms.getItemIndex(thisId);
>+      }
>     }
>+    this.removeOldChildren(this._livemark.folderId, firstIndex);
>+
>+  },
>+
>+  /**
>+   * Removes old children from a livemark container after "Reload Live Bookmark".
>+   * @param folderId
>+   *        Id of the livemark container id.
>+   * @param index
>+   *        Index of the first updated child in the livemark container.
>+   */
>+  removeOldChildren: function LLL_removeOldChildren(folderId, index) {
>+    for (var i = 0; i < index; i++)
>+      this._bms.removeChildAt(folderId, i);
>+    gLivemarkService._loadingId = -1;
>   },
> 
>   /**
>    * See nsIFeedResultListener.idl
>    */
>   handleResult: function LLL_handleResult(result) {
>     if (this._isAborted) {
>       this._livemark.locked = false;
>@@ -514,26 +546,26 @@ LivemarkLoadListener.prototype = {
>       this._livemark.locked = false;
>     }
>   },
>   
>   deleteLivemarkChildren: LivemarkService.prototype.deleteLivemarkChildren,
> 
>   insertLivemarkFailedItem: function LS_insertLivemarkFailed(folderId) {
>     var failedURI = gIoService.newURI("about:livemark-failed", null, null);
>-    var id = this._bms.insertBookmark(folderId, failedURI, this._bms.DEFAULT_INDEX,
>-                                      this._failed);
>+    var id = this._bms.insertBookmark(folderId, failedURI, 0, this._failed);
>   },
> 
>   insertLivemarkChild:
>   function LS_insertLivemarkChild(folderId, uri, title) {
>     var id = this._bms.insertBookmark(folderId, uri, this._bms.DEFAULT_INDEX,
>                                       title);
>     this._ans.setItemAnnotation(id, LMANNO_BMANNO, uri.spec, 0,
>                                 this._ans.EXPIRE_NEVER);
>+    return id;
>   },
> 
>   /**
>    * See nsIStreamListener.idl
>    */
>   onDataAvailable: function LLL_onDataAvailable(request, context, inputStream, 
>                                                 sourceOffset, count) {
>     this._processor.onDataAvailable(request, context, inputStream,
Attachment #273661 - Flags: review?(dietrich) → review-
Whiteboard: [swag: 1d][need review dietrich] → [swag: 1d]
Attached patch Patch (obsolete) — Splinter Review
Attachment #273661 - Attachment is obsolete: true
Attachment #273704 - Flags: review?(dietrich)
Attachment #273704 - Flags: review?(dietrich)
Attached patch Patch (obsolete) — Splinter Review
Attachment #273704 - Attachment is obsolete: true
Attachment #273705 - Flags: review?(dietrich)
Whiteboard: [swag: 1d] → [swag: 1d][need review dietrich]
Comment on attachment 273705 [details] [diff] [review]
Patch

r=me, given these changes

> 
>-  insertLivemarkLoadingItem: function LS_insertLivemarkLoading(bms, folderId) {
>+  insertLivemarkLoadingItem: function LS_insertLivemarkLoading(bms, lvmk) {
>     var loadingURI = gIoService.newURI("about:livemark-loading", null, null);
>-    var id = bms.insertBookmark(folderId, loadingURI, bms.DEFAULT_INDEX,
>-                                this._loading);
>+    var id;
>+    if (!lvmk.loadingId || lvmk.loadingId == -1) {
>+      id = bms.insertBookmark(lvmk.folderId, loadingURI, 0, this._loading);
>+      lvmk.loadingId = id;
>+    }
>+    else
>+      id = lvmk.loadingId;

nit: please expand lvmk to livemark

and you could probably simplify that whole block like:

if (!lvmk.loadingId || lvmk.loadingId == -1)
  lvmk.loadingId = bms.insertBookmark(lvmk.folderId, loadingURI, 0, this._loading)
Attachment #273705 - Flags: review?(dietrich) → review+
Attached patch Nits fixed. Need checkin (obsolete) — Splinter Review
Attachment #273705 - Attachment is obsolete: true
Whiteboard: [swag: 1d][need review dietrich] → [needs checkin]
Keywords: checkin-needed
Missing the freeze, moving out.
Target Milestone: Firefox 3 M7 → Firefox 3 M8
When is this going to be checked-in.  I still can't see it in my M8 checkout from Monday.
Checking in toolkit/components/places/src/nsLivemarkService.js;
/cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.js,v  <--  nsLivemarkService.js
new revision: 1.22; previous revision: 1.21
done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attached patch as checked inSplinter Review
Attachment #273892 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [needs checkin]
Depends on: 392984
verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007112604 Minefield/3.0b2pre
Status: RESOLVED → VERIFIED
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.