Bug 1398019 - Favicons missing in Bookmarks toolbar on startup. r=past draft
authorMarco Bonardo <mbonardo@mozilla.com>
Fri, 08 Sep 2017 11:43:08 +0200
changeset 661429 eec1608c106b56b7ef090c52d6b57bedf948741d
parent 661321 50857982881ae7803ceb438fee90650a282f7f05
child 730566 a1b6734365f2a5e13edf7c1e9b158d5880e69031
push id78753
push usermak77@bonardo.net
push dateFri, 08 Sep 2017 12:31:19 +0000
reviewerspast
bugs1398019
milestone57.0a1
Bug 1398019 - Favicons missing in Bookmarks toolbar on startup. r=past This is due to the fact now we set the icons only when the nodes visibility is updated. The patch always updates nodes visibility when we rebuild the toolbar. updateChevron() is being renamed accordingly and acts regardless of chevron.collapsed. MozReview-Commit-ID: Cz1U710J42M
browser/base/content/browser-places.js
browser/components/places/content/browserPlacesViews.js
browser/components/places/tests/browser/browser_toolbar_overflow.js
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -1148,17 +1148,17 @@ var PlacesMenuDNDHandler = {
  */
 var PlacesToolbarHelper = {
   _place: "place:folder=TOOLBAR",
 
   get _viewElt() {
     return document.getElementById("PlacesToolbar");
   },
 
-  init: function PTH_init(forceToolbarOverflowCheck) {
+  init: function PTH_init() {
     let viewElt = this._viewElt;
     if (!viewElt || viewElt._placesView)
       return;
 
     // CustomizableUI.addListener is idempotent, so we can safely
     // call this multiple times.
     CustomizableUI.addListener(this);
 
@@ -1175,25 +1175,22 @@ var PlacesToolbarHelper = {
     // customizing, because that will happen when the customization is done.
     let toolbar = this._getParentToolbar(viewElt);
     if (!toolbar || toolbar.collapsed || this._isCustomizing ||
         getComputedStyle(toolbar, "").display == "none") {
       return;
     }
 
     new PlacesToolbar(this._place);
-    if (forceToolbarOverflowCheck) {
-      viewElt._placesView.updateOverflowStatus();
-    }
   },
 
   handleEvent(event) {
     switch (event.type) {
       case "toolbarvisibilitychange":
-        if (event.target == this._viewElt.parentNode.parentNode)
+        if (event.target == this._getParentToolbar(this._viewElt))
           this._resetView();
         break;
     }
   },
 
   uninit: function PTH_uninit() {
     if (this._isObservingToolbars) {
       delete this._isObservingToolbars;
@@ -1209,17 +1206,17 @@ var PlacesToolbarHelper = {
         viewElt._placesView.uninit();
     } finally {
       this._isCustomizing = true;
     }
   },
 
   customizeDone: function PTH_customizeDone() {
     this._isCustomizing = false;
-    this.init(true);
+    this.init();
   },
 
   onPlaceholderCommand() {
     let widgetGroup = CustomizableUI.getWidget("personal-bookmarks");
     let widget = widgetGroup.forWindow(window);
     if (widget.overflowed ||
         widgetGroup.areaType == CustomizableUI.TYPE_MENU_PANEL) {
       PlacesCommandHook.showPlacesOrganizer("BookmarksToolbar");
@@ -1260,17 +1257,17 @@ var PlacesToolbarHelper = {
     if (this._viewElt) {
       // It's possible that the placesView might not exist, and we need to
       // do a full init. This could happen if the Bookmarks Toolbar Items are
       // moved to the Menu Panel, and then to the toolbar with the "Add to Toolbar"
       // context menu option, outside of customize mode.
       if (this._viewElt._placesView) {
         this._viewElt._placesView.uninit();
       }
-      this.init(true);
+      this.init();
     }
   },
 };
 
 var RecentBookmarksMenuUI = {
   RECENTLY_BOOKMARKED_PREF: "browser.bookmarks.showRecentlyBookmarked",
   MAX_RESULTS: 5,
   // This timeout affects how soon the recent menu items are updated when
--- a/browser/components/places/content/browserPlacesViews.js
+++ b/browser/components/places/content/browserPlacesViews.js
@@ -1056,16 +1056,18 @@ PlacesToolbar.prototype = {
         // then compensate the measurement error by considering a larger screen
         // width. Moreover the window could be bigger than the screen.
         let size = button.clientHeight;
         let limit = Math.min(cc, parseInt((window.screen.width * 1.5) / size));
         for (let i = 1; i < limit; ++i) {
           this._insertNewItem(this._resultNode.getChild(i), fragment);
         }
         this._rootElt.appendChild(fragment);
+
+        this.updateNodesVisibility();
       });
     }
 
     if (this._chevronPopup.hasAttribute("type")) {
       // Chevron has already been initialized, but since we are forcing
       // a rebuild of the toolbar, it has to be rebuilt.
       // Otherwise, it will be initialized when the toolbar overflows.
       this._chevronPopup.place = this.place;
@@ -1153,32 +1155,32 @@ PlacesToolbar.prototype = {
       case "unload":
         this.uninit();
         break;
       case "resize":
         // This handler updates nodes visibility in both the toolbar
         // and the chevron popup when a window resize does not change
         // the overflow status of the toolbar.
         if (aEvent.target == aEvent.currentTarget) {
-          this.updateChevron();
+          this.updateNodesVisibility();
         }
         break;
       case "overflow":
         if (!this._isOverflowStateEventRelevant(aEvent))
           return;
         this._onOverflow();
         break;
       case "underflow":
         if (!this._isOverflowStateEventRelevant(aEvent))
           return;
         this._onUnderflow();
         break;
       case "TabOpen":
       case "TabClose":
-        this.updateChevron();
+        this.updateNodesVisibility();
         break;
       case "dragstart":
         this._onDragStart(aEvent);
         break;
       case "dragover":
         this._onDragOver(aEvent);
         break;
       case "dragexit":
@@ -1205,59 +1207,47 @@ PlacesToolbar.prototype = {
       case "popuphidden":
         this._onPopupHidden(aEvent);
         break;
       default:
         throw "Trying to handle unexpected event.";
     }
   },
 
-  updateOverflowStatus() {
-    if (this._rootElt.scrollLeftMin != this._rootElt.scrollLeftMax) {
-      this._onOverflow();
-    } else {
-      this._onUnderflow();
-    }
-  },
-
   _isOverflowStateEventRelevant: function PT_isOverflowStateEventRelevant(aEvent) {
     // Ignore events not aimed at ourselves, as well as purely vertical ones:
     return aEvent.target == aEvent.currentTarget && aEvent.detail > 0;
   },
 
   _onOverflow: function PT_onOverflow() {
     // Attach the popup binding to the chevron popup if it has not yet
     // been initialized.
     if (!this._chevronPopup.hasAttribute("type")) {
       this._chevronPopup.setAttribute("place", this.place);
       this._chevronPopup.setAttribute("type", "places");
     }
     this._chevron.collapsed = false;
-    this.updateChevron();
+    this.updateNodesVisibility();
   },
 
   _onUnderflow: function PT_onUnderflow() {
-    this.updateChevron();
+    this.updateNodesVisibility();
     this._chevron.collapsed = true;
   },
 
-  updateChevron: function PT_updateChevron() {
-    // If the chevron is collapsed there's nothing to update.
-    if (this._chevron.collapsed)
-      return;
-
+  updateNodesVisibility: function PT_updateNodesVisibility() {
     // Update the chevron on a timer.  This will avoid repeated work when
     // lot of changes happen in a small timeframe.
-    if (this._updateChevronTimer)
-      this._updateChevronTimer.cancel();
+    if (this._updateNodesVisibilityTimer)
+      this._updateNodesVisibilityTimer.cancel();
 
-    this._updateChevronTimer = this._setTimer(100);
+    this._updateNodesVisibilityTimer = this._setTimer(100);
   },
 
-  _updateChevronTimerCallback: function PT__updateChevronTimerCallback() {
+  _updateNodesVisibilityTimerCallback: function PT__updateNodesVisibilityTimerCallback() {
     let scrollRect = this._rootElt.getBoundingClientRect();
     let childOverflowed = false;
     for (let child of this._rootElt.childNodes) {
       // Once a child overflows, all the next ones will.
       if (!childOverflowed) {
         let childRect = child.getBoundingClientRect();
         childOverflowed = this.isRTL ? (childRect.left < scrollRect.left)
                                      : (childRect.right > scrollRect.right);
@@ -1271,17 +1261,17 @@ PlacesToolbar.prototype = {
         if (icon)
           child.setAttribute("image", icon);
         child.style.visibility = "visible";
       }
     }
 
     // We rebuild the chevron on popupShowing, so if it is open
     // we must update it.
-    if (this._chevron.open)
+    if (!this._chevron.collapsed && this._chevron.open)
       this._updateChevronPopupNodesVisibility();
     let event = new CustomEvent("BookmarksToolbarVisibilityUpdated", {bubbles: true});
     this._viewElt.dispatchEvent(event);
   },
 
   nodeInserted:
   function PT_nodeInserted(aParentPlacesNode, aPlacesNode, aIndex) {
     let parentElt = this._getDOMNodeForPlacesNode(aParentPlacesNode);
@@ -1309,17 +1299,17 @@ PlacesToolbar.prototype = {
       let prevSiblingOverflowed = aIndex > 0 && aIndex <= children.length &&
                                   children[aIndex - 1].style.visibility == "hidden";
       if (prevSiblingOverflowed) {
         button.style.visibility = "hidden";
       } else {
         let icon = aPlacesNode.icon;
         if (icon)
           button.setAttribute("image", icon);
-        this.updateChevron();
+        this.updateNodesVisibility();
       }
       return;
     }
 
     PlacesViewBase.prototype.nodeInserted.apply(this, arguments);
   },
 
   nodeRemoved:
@@ -1338,17 +1328,17 @@ PlacesToolbar.prototype = {
       let overflowed = elt.style.visibility == "hidden";
       this._removeChild(elt);
       if (this._resultNode.childCount > this._rootElt.childNodes.length) {
         // A new node should be built to keep a coherent number of children.
         this._insertNewItem(this._resultNode.getChild(this._rootElt.childNodes.length),
                             this._rootElt);
       }
       if (!overflowed)
-        this.updateChevron();
+        this.updateNodesVisibility();
       return;
     }
 
     PlacesViewBase.prototype.nodeRemoved.apply(this, arguments);
   },
 
   nodeMoved:
   function PT_nodeMoved(aPlacesNode,
@@ -1386,22 +1376,22 @@ PlacesToolbar.prototype = {
         if (icon)
           elt.setAttribute("image", icon);
       } else {
         this._rootElt.insertBefore(elt, this._rootElt.childNodes[aNewIndex]);
       }
 
       // The chevron view may get nodeMoved after the toolbar.  In such a case,
       // we should ensure (by manually swapping menuitems) that the actual nodes
-      // are in the final position before updateChevron tries to updates their
-      // visibility, or the chevron may go out of sync.
-      // Luckily updateChevron runs on a timer, so, by the time it updates
+      // are in the final position before updateNodesVisibility tries to update
+      // their visibility, or the chevron may go out of sync.
+      // Luckily updateNodesVisibility runs on a timer, so, by the time it updates
       // nodes, the menu has already handled the notification.
 
-      this.updateChevron();
+      this.updateNodesVisibility();
       return;
     }
 
     PlacesViewBase.prototype.nodeMoved.apply(this, arguments);
   },
 
   nodeAnnotationChanged:
   function PT_nodeAnnotationChanged(aPlacesNode, aAnno) {
@@ -1441,17 +1431,17 @@ PlacesToolbar.prototype = {
     PlacesViewBase.prototype.nodeTitleChanged.apply(this, arguments);
 
     // Here we need the <menu>.
     if (elt.localName == "menupopup")
       elt = elt.parentNode;
 
     if (elt.parentNode == this._rootElt) { // Node is on the toolbar.
       if (elt.style.visibility != "hidden")
-        this.updateChevron();
+        this.updateNodesVisibility();
     }
   },
 
   invalidateContainer: function PT_invalidateContainer(aPlacesNode) {
     let elt = this._getDOMNodeForPlacesNode(aPlacesNode, true);
     // Nothing to do if it's a never-visible node.
     if (!elt)
       return;
@@ -1600,23 +1590,23 @@ PlacesToolbar.prototype = {
 
   _setTimer: function PT_setTimer(aTime) {
     let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
     timer.initWithCallback(this, aTime, timer.TYPE_ONE_SHOT);
     return timer;
   },
 
   notify: function PT_notify(aTimer) {
-    if (aTimer == this._updateChevronTimer) {
-      this._updateChevronTimer = null;
+    if (aTimer == this._updateNodesVisibilityTimer) {
+      this._updateNodesVisibilityTimer = null;
       // TODO: This should use promiseLayoutFlushed("layout"), so that
-      // _updateChevronTimerCallback can use getBoundsWithoutFlush. But for
-      // yet unknown reasons doing that causes intermittent failures, apparently
-      // due the flush not happening in a meaningful time.
-      window.requestAnimationFrame(this._updateChevronTimerCallback.bind(this));
+      // _updateNodesVisibilityTimerCallback can use getBoundsWithoutFlush. But
+      // for yet unknown reasons doing that causes intermittent failures,
+      // apparently due the flush not happening in a meaningful time.
+      window.requestAnimationFrame(this._updateNodesVisibilityTimerCallback.bind(this));
     } else if (aTimer == this._ibTimer) {
       // * Timer to turn off indicator bar.
       this._dropIndicator.collapsed = true;
       this._ibTimer = null;
     } else if (aTimer == this._overFolder.openTimer) {
       // * Timer to open a menubutton that's being dragged over.
       // Set the autoopen attribute on the folder's menupopup so that
       // the menu will automatically close when the mouse drags off of it.
--- a/browser/components/places/tests/browser/browser_toolbar_overflow.js
+++ b/browser/components/places/tests/browser/browser_toolbar_overflow.js
@@ -30,16 +30,17 @@ add_task(async function setup() {
     await promiseSetToolbarVisibility(gToolbar, true);
     await promiseReady;
   }
   registerCleanupFunction(async () => {
     if (wasCollapsed) {
       await promiseSetToolbarVisibility(gToolbar, false);
     }
     await PlacesUtils.bookmarks.eraseEverything();
+    await PlacesUtils.history.clear();
   });
 });
 
 add_task(async function() {
   // Check that the overflow chevron is visible.
   Assert.ok(!gChevron.collapsed, "The overflow chevron should be visible");
   Assert.ok(gToolbarContent.childNodes.length < BOOKMARKS_COUNT,
             "Not all the nodes should be built by default");
@@ -62,16 +63,46 @@ add_task(async function() {
   await test_move_index("Move node from fist visible to last built",
                         0, gToolbarContent.childNodes.length - 1,
                         "visible", "hidden");
   await test_move_index("Move node from fist visible to first non built",
                         0, gToolbarContent.childNodes.length,
                         "visible", undefined);
 });
 
+add_task(async function test_newWindow_noOverflow() {
+  info("Check toolbar in a new widow when it was already visible and not overflowed");
+  await PlacesUtils.bookmarks.eraseEverything();
+  // Add a single bookmark.
+  await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+    url: "http://toolbar.overflow/",
+    title: "Example"
+  });
+  // Add a favicon for the bookmark.
+  let favicon =  "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAA" +
+                 "AAAA6fptVAAAACklEQVQI12NgAAAAAgAB4iG8MwAAAABJRU5ErkJggg==";
+  await PlacesTestUtils.addFavicons(new Map([["http://toolbar.overflow/", favicon]]));
+
+  let win = await BrowserTestUtils.openNewBrowserWindow();
+  try {
+    let toolbar = win.document.getElementById("PersonalToolbar");
+    Assert.ok(!toolbar.collapsed, "The toolbar is not collapsed");
+    let content = win.document.getElementById("PlacesToolbarItems");
+    await BrowserTestUtils.waitForCondition(() => {
+      return content.childNodes.length == 1 &&
+             content.childNodes[0].hasAttribute("image");
+    });
+    let chevron = win.document.getElementById("PlacesChevron");
+    Assert.ok(chevron.collapsed, "The chevron should be collapsed");
+  } finally {
+    await BrowserTestUtils.closeWindow(win);
+  }
+});
+
 async function test_index(desc, index, expected) {
   info(desc);
   let children = gToolbarContent.childNodes;
   let originalLen = children.length;
   let nodeExisted = children.length > index;
   let previousNodeIsVisible = nodeExisted &&
                               children[index - 1].style.visibility == "visible";
   let promiseUpdateVisibility = expected == "visible" || previousNodeIsVisible