Bug 419544 - "places menupopups options ("open all in tabs", "open <feed website>") do not update correctly" [p=mak77@supereva.it (Marco Bonardo [mak77]) r=Mano a=blocking-firefox3+]
authorreed@reedloden.com
Wed, 12 Mar 2008 04:08:08 -0700
changeset 12934 287ffb4319d8cdabf7b1b0c823d730b65d00d393
parent 12933 a100d6e900f89e908247b18e60f0f3176f5066be
child 12935 163bc1d4f4b6e54083db44d02df0607c2f1682b2
push id1
push userbsmedberg@mozilla.com
push dateThu, 20 Mar 2008 16:49:24 +0000
treeherdermozilla-central@61007906a1f8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMano, blocking-firefox3
bugs419544
milestone1.9b5pre
Bug 419544 - "places menupopups options ("open all in tabs", "open <feed website>") do not update correctly" [p=mak77@supereva.it (Marco Bonardo [mak77]) r=Mano a=blocking-firefox3+]
browser/base/content/browser-places.js
browser/components/places/content/menu.xml
browser/components/places/content/toolbar.xml
browser/components/places/content/utils.js
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -632,79 +632,96 @@ var BookmarksEventHandler = {
    * Handler for popupshowing event for an item in bookmarks toolbar or menu.
    * If the item isn't the main bookmarks menu, add an "Open All in Tabs"
    * menuitem to the bottom of the popup.
    * @param event 
    *        DOMEvent for popupshowing
    */
   onPopupShowing: function BM_onPopupShowing(event) {
     var target = event.originalTarget;
-    if (target.localName == "menupopup" &&
-        target.id != "bookmarksMenuPopup" &&
-        target.getAttribute("anonid") != "chevronPopup") {
-      // Add the "Open All in Tabs" menuitem if there are
-      // at least two menuitems with places result nodes.
-      // Add the "Open (Feed Name)" menuitem if it's a livemark with a siteURI.
-      var numNodes = 0;
-      var hasMultipleEntries = false;
-      var currentChild = target.firstChild;
-      while (currentChild) {
-        if (currentChild.localName == "menuitem" && currentChild.node)
-          numNodes++;
-
-        // If the menuitem already exists, do nothing.
-        if (currentChild.getAttribute("openInTabs") == "true")
-          return;
-        if (currentChild.hasAttribute("siteURI"))
-          return;
-
-        currentChild = currentChild.nextSibling;
-      }
-      if (numNodes > 1)
-        hasMultipleEntries = true;
+    if (!target.hasAttribute("placespopup"))
+      return;
 
-      var itemId = target._resultNode.itemId;
-      var siteURIString = "";
-      if (itemId != -1 && PlacesUtils.livemarks.isLivemark(itemId)) {
-        var siteURI = PlacesUtils.livemarks.getSiteURI(itemId);
-        if (siteURI)
-          siteURIString = siteURI.spec;
-      }
-
-      if (hasMultipleEntries || siteURIString) {
-        var separator = document.createElement("menuseparator");
-        target.appendChild(separator);
-
-        if (siteURIString) {
-          var openHomePage = document.createElement("menuitem");
-          openHomePage.setAttribute("siteURI", siteURIString);
-          openHomePage.setAttribute("oncommand",
-              "openUILink(this.getAttribute('siteURI'), event);");
-          // If a user middle-clicks this item we serve the oncommand event
-          // We are using checkForMiddleClick because of Bug 246720
-          // Note: stopPropagation is needed to avoid serving middle-click 
-          // with BT_onClick that would open all items in tabs
-          openHomePage.setAttribute("onclick",
-              "checkForMiddleClick(this, event); event.stopPropagation();");
-          openHomePage.setAttribute("label",
-              PlacesUtils.getFormattedString("menuOpenLivemarkOrigin.label",
-              [target.parentNode.getAttribute("label")]));
-          target.appendChild(openHomePage);
-        }
-
-        if (hasMultipleEntries) {
-          var openInTabs = document.createElement("menuitem");
-          openInTabs.setAttribute("openInTabs", "true");
-          openInTabs.setAttribute("oncommand",
-                                  "PlacesUtils.openContainerNodeInTabs(this.parentNode._resultNode, event);");
-          openInTabs.setAttribute("label",
-                     gNavigatorBundle.getString("menuOpenAllInTabs.label"));
-          target.appendChild(openInTabs);
+    // Check if the popup contains at least 2 menuitems with places nodes
+    var numNodes = 0;
+    var hasMultipleURIs = false;
+    var currentChild = target.firstChild;
+    while (currentChild) {
+      if (currentChild.localName == "menuitem" && currentChild.node) {
+        if (++numNodes == 2) {
+          hasMultipleURIs = true;
+          break;
         }
       }
+      currentChild = currentChild.nextSibling;
+    }
+
+    var itemId = target._resultNode.itemId;
+    var siteURIString = "";
+    if (itemId != -1 && PlacesUtils.livemarks.isLivemark(itemId)) {
+      var siteURI = PlacesUtils.livemarks.getSiteURI(itemId);
+      if (siteURI)
+        siteURIString = siteURI.spec;
+    }
+
+    if (!siteURIString && target._endOptOpenSiteURI) {
+        target.removeChild(target._endOptOpenSiteURI);
+        target._endOptOpenSiteURI = null;
+    }
+
+    if (!hasMultipleURIs && target._endOptOpenAllInTabs) {
+      target.removeChild(target._endOptOpenAllInTabs);
+      target._endOptOpenAllInTabs = null;
+    }
+
+    if (!(hasMultipleURIs || siteURIString)) {
+      // we don't have to show any option
+      if (target._endOptSeparator) {
+        target.removeChild(target._endOptSeparator);
+        target._endOptSeparator = null;
+        target._endMarker = -1;
+      }
+      return;
+    }
+
+    if (!target._endOptSeparator) {
+      // create a separator before options
+      target._endOptSeparator = document.createElement("menuseparator");
+      target._endOptSeparator.setAttribute("builder", "end");
+      target._endMarker = target.childNodes.length;
+      target.appendChild(target._endOptSeparator);
+    }
+
+    if (siteURIString && !target._endOptOpenSiteURI) {
+      // Add "Open (Feed Name)" menuitem if it's a livemark with a siteURI
+      target._endOptOpenSiteURI = document.createElement("menuitem");
+      target._endOptOpenSiteURI.setAttribute("siteURI", siteURIString);
+      target._endOptOpenSiteURI.setAttribute("oncommand",
+          "openUILink(this.getAttribute('siteURI'), event);");
+      // If a user middle-clicks this item we serve the oncommand event
+      // We are using checkForMiddleClick because of Bug 246720
+      // Note: stopPropagation is needed to avoid serving middle-click 
+      // with BT_onClick that would open all items in tabs
+      target._endOptOpenSiteURI.setAttribute("onclick",
+          "checkForMiddleClick(this, event); event.stopPropagation();");
+      target._endOptOpenSiteURI.setAttribute("label",
+          PlacesUtils.getFormattedString("menuOpenLivemarkOrigin.label",
+          [target.parentNode.getAttribute("label")]));
+      target.appendChild(target._endOptOpenSiteURI);
+    }
+
+    if (hasMultipleURIs && !target._endOptOpenAllInTabs) {
+        // Add the "Open All in Tabs" menuitem if there are
+        // at least two menuitems with places result nodes.
+        target._endOptOpenAllInTabs = document.createElement("menuitem");
+        target._endOptOpenAllInTabs.setAttribute("oncommand",
+            "PlacesUtils.openContainerNodeInTabs(this.parentNode._resultNode, event);");
+        target._endOptOpenAllInTabs.setAttribute("label",
+            gNavigatorBundle.getString("menuOpenAllInTabs.label"));
+        target.appendChild(target._endOptOpenAllInTabs);
     }
   },
 
   fillInBTTooltip: function(aTipElement) {
     // Fx2XP: Don't show tooltips for bookmarks under sub-folders
     if (aTipElement.localName != "toolbarbutton")
       return false;
 
--- a/browser/components/places/content/menu.xml
+++ b/browser/components/places/content/menu.xml
@@ -73,16 +73,18 @@
 
       <!-- markers for start and end of valid places items -->
       <field name="_startMarker">-1</field>
       <field name="_endMarker">-1</field>
 
       <!-- This is the view that manage the popup -->
       <field name="_rootView">PlacesUtils.getViewForNode(this);</field>
 
+      <field name="_built">false</field>
+
       <method name="onDragOver">
         <parameter name="aEvent"/>
         <parameter name="aFlavour"/>
         <parameter name="aDragSession"/>
         <body><![CDATA[
           PlacesControllerDragHelper.currentDropTarget = aEvent.target;
           // check if we have a valid dropPoint
           var dropPoint = this._getDropPoint(aEvent);
@@ -226,24 +228,24 @@
         <parameter name="aEvent"/>
         <body><![CDATA[
           var target = aEvent.target;
 
           // in some view we have _startMarker and _endMarker, we should not
           // draw the drop indicator outside of them
           var betweenMarkers = true;
           if (this._startMarker != -1 &&
-              target.boxObject.y < this.childNodes[this._startMarker].boxObject.y)
+              target.boxObject.y <= this.childNodes[this._startMarker].boxObject.y)
             betweenMarkers = false;
           if (this._endMarker != -1 &&
-              target.boxObject.y > this.childNodes[this._endMarker].boxObject.y)
+              target.boxObject.y >= this.childNodes[this._endMarker].boxObject.y)
             betweenMarkers = false;
 
           // hide the dropmarker if current node is not a places bookmark item
-          return !(target && betweenMarkers && this.canDrop());
+          return !(target && target.node && betweenMarkers && this.canDrop());
         ]]></body>
       </method>
 
       <!-- This function returns information about where to drop when
            dragging over this popup insertion point -->
       <method name="_getDropPoint">
         <parameter name="aEvent"/>
           <body><![CDATA[
@@ -489,24 +491,23 @@
           this._initialized = true;
         ]]></body>
       </method>
 
       <property name="controller"
                 readonly="true"
                 onget="return this._controller;"/>
 
-      <field name="_built">false</field>
-
       <method name="onPopupShowing">
         <parameter name="aEvent"/>
         <body><![CDATA[
           var popup = aEvent.target;
           var resultNode = popup._resultNode;
-          resultNode.containerOpen = true;
+          if (!resultNode.containerOpen)
+            resultNode.containerOpen = true;
           if (!popup._built)
             this._rebuild(popup);
         ]]></body>
       </method>
 
       <!-- nsIPlacesView -->
       <method name="getResult">
         <body><![CDATA[
@@ -517,68 +518,16 @@
       <!-- nsIPlacesView -->
       <method name="getResultNode">
         <body><![CDATA[
           this._ensureInitialized();
           return this._resultNode;
         ]]></body>
       </method>
 
-      <method name="_cleanMenu">
-        <parameter name="aPopup"/>
-        <body><![CDATA[
-          // Find static menuitems that should go at the start
-          // and end of the menu, marked by builder="start" and
-          // builder="end" attributes, and keep track of their indices.
-          // All of the items between the start and end should be removed.
-          var items = [];
-          aPopup._startMarker = -1;
-          aPopup._endMarker = -1;
-          for (var i = 0; i < aPopup.childNodes.length; ++i) {
-            var item = aPopup.childNodes[i];
-            if (item.getAttribute("builder") == "start") {
-              aPopup._startMarker = i;
-              continue;
-            }
-            if (item.getAttribute("builder") == "end") {
-              aPopup._endMarker = i;
-              continue;
-            }
-            if ((aPopup._startMarker != -1) && (aPopup._endMarker == -1))
-              items.push(item);
-          }
-
-          // If static items at the beginning were found, remove all items between
-          // them and the static content at the end.
-          for (var i = 0; i < items.length; ++i) { 
-            // skip the empty menu item
-            if (aPopup._emptyMenuItem != items[i]) {
-              aPopup.removeChild(items[i]);
-              if (this._endMarker > 0)
-                --this._endMarker;
-            }
-          }
-
-          // If no static items were found at the beginning, remove all items before
-          // the static items at the end.
-          if (aPopup._startMarker == -1) {
-            var end = aPopup._endMarker == -1 ?
-                      aPopup.childNodes.length - 1 : aPopup._endMarker - 1;
-            for (var i = end; i >=0; i--) {
-              // skip the empty menu item
-              if (aPopup._emptyMenuItem != aPopup.childNodes[i]) {
-                aPopup.removeChild(aPopup.childNodes[i]);
-                if (aPopup._endMarker > 0)
-                  --aPopup._endMarker;
-              }
-            }
-          }
-        ]]></body>
-      </method>
-
       <method name="removeItem">
         <parameter name="child"/>
         <body><![CDATA[
           if (PlacesUtils.nodeIsContainer(child.node)) {
             for (var i=0; i < this._containerNodesMap.length; i++) {
               if (this._containerNodesMap[i].resultNode == child.node) {
                 this._containerNodesMap.splice(i, 1);
                 break;
@@ -632,17 +581,17 @@
           aPopup._emptyMenuItem.setAttribute("disabled", true);
           aPopup.appendChild(aPopup._emptyMenuItem);
         ]]></body>
       </method>
 
       <method name="_rebuild">
         <parameter name="aPopup"/>
         <body><![CDATA[
-          this._cleanMenu(aPopup);
+          PlacesUtils.cleanPlacesPopup(aPopup);
 
           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);
--- a/browser/components/places/content/toolbar.xml
+++ b/browser/components/places/content/toolbar.xml
@@ -978,53 +978,60 @@
 
       <method name="insertNewItemToPopup">
         <parameter name="aChild"/>
         <parameter name="aParentPopup"/>
         <parameter name="aBefore"/>
         <body><![CDATA[
           var element =
             PlacesUtils.createMenuItemForNode(aChild, this._containerNodesMap);
+
           if (aBefore)
             aParentPopup.insertBefore(element, aBefore);
-          else
-            aParentPopup.appendChild(element);
+          else {
+            // Add the new element to the menu.  If there is static content at
+            // the end of the menu, add the element before that.  Otherwise,
+            // just add to the end.
+            if (aParentPopup._endMarker != -1) {
+              aParentPopup.insertBefore(element,
+                                        aParentPopup.childNodes[aParentPopup._endMarker++]);
+            }
+            else
+              aParentPopup.appendChild(element);
+          }
         ]]></body>
       </method>
 
       <method name="_containerPopupShowing">
         <parameter name="aPopup"/>
         <body><![CDATA[
           if (aPopup._built)
             return;
 
-          // remove previous menu items
-          while (aPopup.hasChildNodes())
-            aPopup.removeChild(aPopup.firstChild);
-          // restore the empty-menu item if has been created already
-          if (aPopup._emptyMenuItem)
-            aPopup.appendChild(aPopup._emptyMenuItem);
+          PlacesUtils.cleanPlacesPopup(aPopup);
 
           var resultNode = aPopup._resultNode;
           if (!resultNode.containerOpen)
             resultNode.containerOpen = true;
 
           var cc = resultNode.childCount;
           if (cc > 0) {
             if (aPopup._emptyMenuItem)
               aPopup._emptyMenuItem.hidden = true;
 
             for (var i = 0; i < cc; ++i) {
               var child = resultNode.getChild(i);
               this.insertNewItemToPopup(child, aPopup, null);
             }
           }
           else {
-            // add element to show it is empty.
-            this._showEmptyMenuItem(aPopup);
+            // This menu is empty.  If there is no static content, add
+            // an element to show it is empty.
+            if (aPopup._startMarker == -1 && aPopup._endMarker == -1)
+              this._showEmptyMenuItem(aPopup);
           }
           aPopup._built = true;
         ]]></body>
       </method>
 
       <method name="_isChevronChild">
         <parameter name="aChild"/>
         <body><![CDATA[
--- a/browser/components/places/content/utils.js
+++ b/browser/components/places/content/utils.js
@@ -1891,16 +1891,64 @@ var PlacesUtils = {
         element.setAttribute("image", iconURISpec);
     }
     element.node = aNode;
     element.node.viewIndex = 0;
 
     return element;
   },
 
+  cleanPlacesPopup: function PU_cleanPlacesPopup(aPopup) {
+    // Find static menuitems at the start and at the end of the menupopup,
+    // marked by builder="start" and builder="end" attributes, and set
+    // markers to keep track of their indices.
+    var items = [];
+    aPopup._startMarker = -1;
+    aPopup._endMarker = -1;
+    for (var i = 0; i < aPopup.childNodes.length; ++i) {
+      var item = aPopup.childNodes[i];
+      if (item.getAttribute("builder") == "start") {
+        aPopup._startMarker = i;
+        continue;
+      }
+      if (item.getAttribute("builder") == "end") {
+        aPopup._endMarker = i;
+        continue;
+      }
+      if ((aPopup._startMarker != -1) && (aPopup._endMarker == -1))
+        items.push(item);
+    }
+
+    // If static items at the beginning were found, remove all items between
+    // them and the static content at the end.
+    for (var i = 0; i < items.length; ++i) {
+      // skip the empty menu item
+      if (aPopup._emptyMenuItem != items[i]) {
+        aPopup.removeChild(items[i]);
+        if (this._endMarker > 0)
+          --this._endMarker;
+      }
+    }
+
+    // If no static items were found at the beginning, remove all items before
+    // the static items at the end.
+    if (aPopup._startMarker == -1) {
+      var end = aPopup._endMarker == -1 ?
+                aPopup.childNodes.length - 1 : aPopup._endMarker - 1;
+      for (var i = end; i >= 0; i--) {
+        // skip the empty menu item
+        if (aPopup._emptyMenuItem != aPopup.childNodes[i]) {
+          aPopup.removeChild(aPopup.childNodes[i]);
+          if (aPopup._endMarker > 0)
+            --aPopup._endMarker;
+        }
+      }
+    }
+  },
+
   getBestTitle: function PU_getBestTitle(aNode) {
     var title;
     if (!aNode.title && this.uriTypes.indexOf(aNode.type) != -1) {
       // if node title is empty, try to set the label using host and filename
       // this._uri() will throw if aNode.uri is not a valid URI
       try {
         var uri = this._uri(aNode.uri);
         var host = uri.host;