Closed Bug 407468 Opened 17 years ago Closed 16 years ago

Livemark loading and failed to load messages should be static menuitems from menu.xml


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




Firefox 3.1b1


(Reporter: philor, Assigned: mak)




(1 file, 8 obsolete files)

Currently, we create the "Live bookmark loading..." and "Live bookmark failed to load" messages by creating bookmark items with URLs like about:livemark-loading, which requires that everything (including extensions) that deals with bookmarks remembers to exclude them when they aren't appropriate, and results in gymnastics like bug 392984 to manage them, and causes bug 333416 and bug 278247 and the others that I remember being around in the dusty corners, because we don't exclude them when it comes to loading, so we try to load "about:livemark-failed" and throw a dialog at the poor user who wondered why his feed was broken.

If instead they were just static items created by the menu binding, managing them would just be a matter of setting and unsetting an annotation on the feed, and doing either something useful or nothing at all when they are clicked would be simple.
Flags: blocking-firefox3?
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Priority: -- → P4
Target Milestone: --- → Firefox 3 M11
Blocks: 410632
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #298979 - Flags: review?(mano)
Comment on attachment 298979 [details] [diff] [review]
patch v1

>Index: browser/components/places/content/menu.xml
>RCS file: /cvsroot/mozilla/browser/components/places/content/menu.xml,v
>retrieving revision 1.95
>diff -u -p -8 -r1.95 menu.xml
>--- browser/components/places/content/menu.xml	9 Jan 2008 06:21:09 -0000	1.95
>+++ browser/components/places/content/menu.xml	24 Jan 2008 18:15:08 -0000
>@@ -108,16 +108,23 @@
>           this._ensureInitialized();
>           return this._resultNode;
>         ]]></body>
>       </method>
>       <method name="_cleanMenu">
>         <parameter name="aPopup"/>
>         <body><![CDATA[
>+          // Remove the special and static 'loading' & 'loading failed'
>+          // menuitems manually
>+          if (aPopup._loadingItem)
>+            aPopup.removeChild(aPopup._loadingItem);
>+          if (aPopup._failedItem)
>+            aPopup.removeChild(aPopup._failedItem);

Thinking about this more, It would be better to do this in _rebuild, there you should also avoid removing and re-creating these elements if they're still needed.
Attachment #298979 - Flags: review?(mano) → review-
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #298979 - Attachment is obsolete: true
Attachment #300325 - Flags: review?(mano)
Comment on attachment 300325 [details] [diff] [review]

Index: browser/components/places/content/menu.xml
RCS file: /cvsroot/mozilla/browser/components/places/content/menu.xml,v
retrieving revision 1.97
diff -u -p -8 -r1.97 menu.xml
--- browser/components/places/content/menu.xml	27 Jan 2008 05:36:44 -0000	1.97
+++ browser/components/places/content/menu.xml	30 Jan 2008 08:56:50 -0000
@@ -217,16 +217,58 @@
       <method name="_rebuild">
         <parameter name="aPopup"/>
+          // The following will determine whether the 'Loading...'

All of this should be moved to a helper method in PlacesUtils, which would take
a folder popup and create/remove the menuitems.

+          // or 'Loading Failed' menuitems need to be added and/or removed,
+          // by checking the relevant annotations.
+          // Since the startMarker is reset to -1 in the _cleanMenu function,
+          // decreasing it's value will not be needed when removing a menuitem.
+          if (PlacesUtils.nodeIsLivemarkContainer(aPopup._resultNode)) {
+          var itemId = aPopup._resultNode.itemId;

nit: indent.

+            if (!aPopup._failedItem &&
+                PlacesUtils.annotations.itemHasAnnotation(itemId,
+                                        "livemark/loadfailed")) {
+              aPopup._failedItem = document.createElement("menuitem");
+              aPopup._failedItem.setAttribute("label",
+                            PlacesUtils.getString("bookmarksLivemarkFailed"));
+              aPopup._failedItem.setAttribute("disabled", true);
+              aPopup.appendChild(aPopup._failedItem);
+              aPopup._startMarker++;
+            }
+            else if (aPopup._failedItem &&
+                    !PlacesUtils.annotations.itemHasAnnotation(itemId,
+                                            "livemark/loadfailed")) {
+              aPopup.removeChild(aPopup._failedItem);
+              aPopup._failedItem = null;
+            }
+            if (!aPopup._loadingItem && 
+                PlacesUtils.annotations.itemHasAnnotation(itemId,
+                                        "livemark/loading")) {

you should avoid checking both annotations if possible (meaningfully, make this
an |else if| block and null out the loading item elsewhere)

+              aPopup._loadingItem = document.createElement("menuitem");
+              aPopup._loadingItem.setAttribute("label",
+                            PlacesUtils.getString("bookmarksLivemarkLoading"));
+              aPopup._loadingItem.setAttribute("disabled", true);
+              aPopup.appendChild(aPopup._loadingItem);
+              aPopup._startMarker++;
+            }
+            else if (aPopup._loadingItem &&
+                    !PlacesUtils.annotations.itemHasAnnotation(itemId,
+                                            "livemark/loading")) {
+              aPopup.removeChild(aPopup._loadingItem);
+              aPopup._loadingItem = null;
+            }
+          }

nit: trailing spaces.

           var cc = aPopup._resultNode.childCount;
           if (cc > 0) {
             if (aPopup._emptyMenuItem)
               aPopup._emptyMenuItem.hidden = true;
             for (var i = 0; i < cc; ++i) {
               var child = aPopup._resultNode.getChild(i);
               this.insertNewItem(child, aPopup, null);
@@ -340,19 +382,62 @@
           var title = aNode.title;
           if (menuitem.getAttribute("label") != title)
             menuitem.setAttribute("label", title);
-          if (!menuitem.hasAttribute("livemark") &&
-              PlacesUtils.nodeIsLivemarkContainer(aNode))
-            menuitem.setAttribute("livemark", "true");
+          // The following will add and remove the 'Loading Failed'
+          // and 'Loading ...' menuitems. Additionally, the startMarker
+          // is updated accordingly.
+          if (PlacesUtils.nodeIsLivemarkContainer(aNode)) {
+            if (!menuitem.hasAttribute("livemark"))
+              menuitem.setAttribute("livemark", "true");
+            var fChild = menuitem.firstChild;

s/fChild/popup/ maybe?

+            if (!fChild._loadingItem &&
+               PlacesUtils.annotations.itemHasAnnotation(aNode.itemId,
+                                      "livemark/loading")) {
+              fChild._loadingItem = document.createElement("menuitem");
+              fChild._loadingItem.setAttribute("label",
+                            PlacesUtils.getString("bookmarksLivemarkLoading"));
+              fChild._loadingItem.setAttribute("disabled", true);
+              fChild.insertBefore(fChild._loadingItem,
+                                  fChild.childNodes[fChild._startMarker + 1]);
+              fChild._startMarker++;
+            }
+            else if (fChild._loadingItem && 
+                !PlacesUtils.annotations.itemHasAnnotation(aNode.itemId,

it would be good to read aNode.itemId just once.

Index: toolkit/components/places/src/nsLivemarkService.js
@@ -223,21 +220,19 @@ LivemarkService.prototype = {
       this._updateLivemarkChildren(i, false);
   deleteLivemarkChildren: function LS_deleteLivemarkChildren(folderId) {
-  insertLivemarkLoadingItem: function LS_insertLivemarkLoading(bms, livemark) {
-    var loadingURI = gIoService.newURI("about:livemark-loading", null, null);
-    if (!livemark.loadingId || livemark.loadingId == -1)
-      livemark.loadingId = bms.insertBookmark(livemark.folderId, loadingURI,
-                                              0, this._loading);
+  insertLivemarkLoadingItem: function LS_insertLivemarkLoading(livemark) {
+    this._ans.setItemAnnotation(livemark.folderId,
+                                LMANNO_LOADING, true, 0, this._ans.EXPIRE_NEVER);

This method should be rename as it no longer adds an item (maybe markLivemarkAsLoading?)

     if (this._isAborted) {
-      if (this._livemark.loadingId != -1) {
-        this._bms.removeItem(this._livemark.loadingId);
-        this._livemark.loadingId = -1;
-      }
+      if (this._ans.itemHasAnnotation(this._livemark.folderId, LMANNO_LOADING))
+        this._ans.removeItemAnnotation(this._livemark.folderId, LMANNO_LOADING);

You can call removeItemAnnotation without checking that.

+      if (this._ans.itemHasAnnotation(this._livemark.folderId, LMANNO_LOADING))
+        this._ans.removeItemAnnotation(this._livemark.folderId, LMANNO_LOADING);

here too.
Attachment #300325 - Flags: review?(mano) → review-
Attached patch v1.2 (obsolete) — Splinter Review
Attachment #300325 - Attachment is obsolete: true
Attachment #300516 - Flags: review?(mano)
Mano: I have made all the necessary adjustments to the code, except for one.
You mentioned the following:
"you should avoid checking both annotations if possible (meaningfully, make this
an |else if| block and null out the loading item elsewhere)"

But, in some instances - both the Loading & Loading Failed annotations exist, thus exists the need to create both menu items.  For this reason, I must check both annotations.
Attached patch v1.3 (obsolete) — Splinter Review
oops... I forgot to include nsLivemarkService in the last diff. sorry :)
Attachment #300516 - Attachment is obsolete: true
Attachment #300518 - Flags: review?(mano)
Attachment #300516 - Flags: review?(mano)
Attached patch v1.4 (obsolete) — Splinter Review
Same deal, less code redundancy..
Attachment #300518 - Attachment is obsolete: true
Attachment #301584 - Flags: review?(mano)
Attachment #300518 - Flags: review?(mano)
+  ensureLivemarkStatusMenuItems: function(aPopup, aItemId, aIsItemChanged) {
+    // The following will determine whether the 'Loading...'
+    // or 'Loading Failed' menuitems need to be added and/or removed,
+    // by checking the relevant annotations.
+    if (this.nodeIsLivemarkContainer(aPopup._resultNode) || aIsItemChanged) {
I don't understand the || here. What do you actually want to do/not to do on-item-change? Why do you want to do so even if the popup isn't a livemark container?
The reason I checked the aIsItemChanged, is because it will only by true if it received a livemark container.  During both (toolbar & menu) itemChanged, an if statement precedes the ensureLivemarkStatusMenuItems :
+          if (PlacesUtils.nodeIsLivemarkContainer(aNode)) {
+            if (!menuitem.hasAttribute("livemark"))
+              menuitem.setAttribute("livemark", "true");
+            PlacesUtils.ensureLivemarkStatusMenuItems(menuitem.firstChild,
+                                                      aNode.itemId, true);
+          }
This means, that if the function was called during an itemChaned event, it is surely a livemark container.
Attached patch v1.5 (obsolete) — Splinter Review
Mano: Thank you so much for your reviews!

- Same deal, more comments!
Attachment #301584 - Attachment is obsolete: true
Attachment #301714 - Flags: review?(mano)
Attachment #301584 - Flags: review?(mano)
Comment on attachment 301714 [details] [diff] [review]

Reviewed over IM.
Attachment #301714 - Flags: review?(mano) → review-
Target Milestone: Firefox 3 beta3 → ---
Assignee: nobody → dev
Since I moved the creation of the static menuitems to PlacesUtils (utils.js), the reterieval of both strings (loading & failed) is being made through PlacesUtils.getString.  This is a problem for me, because bug 415117 had moved both strings to a different string bundle!

So, should I:
  a. Move them back to the original string bundle?
  b. Ignore PlacesUtils.getString function, and simply retrieve both strings my self from the appropriate string bundle?
Attached patch v1.6 (obsolete) — Splinter Review
Updated patch.

Note:  In some instances, both the "Loading" and "Loading Failed" menu items appear.  This also happens prior to this patch, which leads me to believe that it is another bug all together.
Attachment #301714 - Attachment is obsolete: true
Attachment #303729 - Flags: review?(mano)
Michael: current utils.js uses the browser/ file, which is the one you've moved the strings to, haven't you?
Yes, I had moved the strings from the toolkit/ file to the browser/ file.
Michael, sorry for that, but Bug 419832 wil unbitrot this :(
Marco: Don't worry about it.. Thats been happening a lot lately, and that is also why I have been holding off with this bug for a while...
Blocks: 333416
Michael, do you have time slot to update this? I could probably bring on your work if you need, i'd like to have this into final
Marco: I'm on vacation in Phoenix until next week. So either I'll get it done by next Thursday, or if it's too late and you've got the time and will, feel free to take this bug...
Comment on attachment 303729 [details] [diff] [review]

obsoleted (bitrotted)
Attachment #303729 - Attachment is obsolete: true
Attachment #303729 - Flags: review?(mano)
Blocks: 329534
Blocks: 441786
Blocks: 453678
Attached patch v1.7 (obsolete) — Splinter Review
for now i've unbitrotted the full thing, much code has changed.

this is based on the new markers code from bug 418671, that should ensure markers are correctly updated. I can tell this works however is not a finalized patch
Assignee: dev → mak77
notice this requires a string move from toolkit to browser so we could try to put this in before the beta 1 string freeze on 26th
Attached patch v1.8Splinter Review
i've sone some cleanup, we were setting annos in too many points and we need only one status menuitem (the feed should be in status failed or loading, not both).
Attachment #339465 - Attachment is obsolete: true
Attachment #339570 - Flags: review?(mano)
PS: this fixed places transaction unit test that was not counting the "loading" item
Attachment #339570 - Flags: review?(mano) → review+
Comment on attachment 339570 [details] [diff] [review]

r=mano, thanks.
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b1
Blocks: 414817
Depends on: 459676
Depends on: 491247
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.

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