Bug 1398019 - Favicons missing in Bookmarks toolbar on startup. r=past
authorMarco Bonardo <mbonardo@mozilla.com>
Fri, 08 Sep 2017 11:43:08 +0200
changeset 429220 0a49941e45a5d58b01051b4283d4bd1fff7aadd1
parent 429219 6f6a270a020f26f88c6ee099de86073f00ac84c1
child 429221 39dc0da77e09f839fc395bdeb019536c7dcf244d
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspast
bugs1398019
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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