Bug 1356440 - Favicons of bookmarks views don't update on visit. r=mrbkap,past,enndeakin+6102 draft
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 19 Apr 2017 11:41:49 +0200
changeset 566577 0da3b4bf0fca0462e22f76c1392f1d9e69f0e71c
parent 566156 8b854986038cf3f3f240697e27ef48ea65914c13
child 625355 4a0a7fd17030ea9054a184e1151306fc8e1110d8
push id55261
push userbmo:mak77@bonardo.net
push dateFri, 21 Apr 2017 20:04:51 +0000
reviewersmrbkap, past, enndeakin
bugs1356440
milestone55.0a1
Bug 1356440 - Favicons of bookmarks views don't update on visit. r=mrbkap,past,enndeakin+6102 MozReview-Commit-ID: 8j5yLqr7MTc
browser/components/places/content/browserPlacesViews.js
browser/components/places/content/treeView.js
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_views_iconsupdate.js
browser/components/places/tests/browser/favicon-normal16.png
browser/components/places/tests/browser/head.js
dom/webidl/TreeBoxObject.webidl
layout/xul/tree/TreeBoxObject.cpp
layout/xul/tree/TreeBoxObject.h
layout/xul/tree/nsITreeBoxObject.idl
layout/xul/tree/nsTreeBodyFrame.cpp
layout/xul/tree/nsTreeBodyFrame.h
testing/modules/TestUtils.jsm
--- a/browser/components/places/content/browserPlacesViews.js
+++ b/browser/components/places/content/browserPlacesViews.js
@@ -506,16 +506,18 @@ PlacesViewBase.prototype = {
     // be done when the icon changes.
     if (elt == this._rootElt)
       return;
 
     // Here we need the <menu>.
     if (elt.localName == "menupopup") {
       elt = elt.parentNode;
     }
+    // We must remove and reset the attribute to force an update.
+    elt.removeAttribute("image");
     elt.setAttribute("image", aPlacesNode.icon);
   },
 
   nodeAnnotationChanged:
   function PVB_nodeAnnotationChanged(aPlacesNode, aAnno) {
     let elt = this._getDOMNodeForPlacesNode(aPlacesNode);
 
     // All livemarks have a feedURI, so use it as our indicator of a livemark
--- a/browser/components/places/content/treeView.js
+++ b/browser/components/places/content/treeView.js
@@ -776,18 +776,21 @@ PlacesTreeView.prototype = {
     if (aNode == this._rootNode)
       return;
 
     let row = this._getRowForNode(aNode);
     if (row == -1)
       return;
 
     let column = this._findColumnByType(aColumnType);
-    if (column && !column.element.hidden)
+    if (column && !column.element.hidden) {
+      if (aColumnType == this.COLUMN_TYPE_TITLE)
+        this._tree.removeImageCacheEntry(row, column);
       this._tree.invalidateCell(row, column);
+    }
 
     // Last modified time is altered for almost all node changes.
     if (aColumnType != this.COLUMN_TYPE_LASTMODIFIED) {
       let lastModifiedColumn =
         this._findColumnByType(this.COLUMN_TYPE_LASTMODIFIED);
       if (lastModifiedColumn && !lastModifiedColumn.hidden)
         this._tree.invalidateCell(row, lastModifiedColumn);
     }
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -48,13 +48,16 @@ support-files =
 [browser_library_panel_leak.js]
 [browser_library_search.js]
 [browser_library_views_liveupdate.js]
 [browser_markPageAsFollowedLink.js]
 [browser_sidebarpanels_click.js]
 skip-if = true # temporarily disabled for breaking the treeview - bug 658744
 [browser_sort_in_library.js]
 [browser_toolbarbutton_menu_context.js]
+[browser_views_iconsupdate.js]
+support-files =
+  favicon-normal16.png
 [browser_views_liveupdate.js]
 [browser_bookmark_all_tabs.js]
 support-files =
   bookmark_dummy_1.html
   bookmark_dummy_2.html
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_views_iconsupdate.js
@@ -0,0 +1,119 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Tests Places views (toolbar, tree) for icons update.
+ * The menu is not tested since it uses the same code as the toolbar.
+ */
+
+add_task(function* () {
+  const PAGE_URI = NetUtil.newURI("http://places.test/");
+  const ICON_URI = NetUtil.newURI("http://mochi.test:8888/browser/browser/components/places/tests/browser/favicon-normal16.png");
+
+  info("Uncollapse the personal toolbar if needed");
+  let toolbar = document.getElementById("PersonalToolbar");
+  let wasCollapsed = toolbar.collapsed;
+  if (wasCollapsed) {
+    yield promiseSetToolbarVisibility(toolbar, true);
+    registerCleanupFunction(function* () {
+      yield promiseSetToolbarVisibility(toolbar, false);
+    });
+  }
+
+  info("Open the bookmarks sidebar");
+  let sidebar = document.getElementById("sidebar");
+  let promiseSidebarLoaded = new Promise(resolve => {
+    sidebar.addEventListener("load", resolve, {capture: true, once: true});
+  });
+  SidebarUI.show("viewBookmarksSidebar");
+  registerCleanupFunction(() => {
+    SidebarUI.hide();
+  });
+  yield promiseSidebarLoaded;
+
+  // Add a bookmark to the bookmarks toolbar.
+  let bm = yield PlacesUtils.bookmarks.insert({
+    url: PAGE_URI,
+    title: "test icon",
+    parentGuid: PlacesUtils.bookmarks.toolbarGuid
+  });
+  registerCleanupFunction(function* () {
+    yield PlacesUtils.bookmarks.remove(bm);
+  });
+
+  // The icon is read asynchronously from the network, we don't have an easy way
+  // to wait for that.
+  yield new Promise(resolve => {
+    setTimeout(resolve, 3000);
+  });
+
+  let toolbarElt = getNodeForToolbarItem(bm.guid);
+  let toolbarShot1 = TestUtils.screenshotArea(toolbarElt, window);
+  let sidebarRect = yield getRectForSidebarItem(bm.guid);
+  let sidebarShot1 = TestUtils.screenshotArea(sidebarRect, window);
+
+  yield new Promise(resolve => {
+    PlacesUtils.favicons.setAndFetchFaviconForPage(
+      PAGE_URI, ICON_URI, true,
+      PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE,
+      resolve,
+      Services.scriptSecurityManager.getSystemPrincipal()
+    );
+  });
+
+  // The icon is read asynchronously from the network, we don't have an easy way
+  // to wait for that.
+  yield new Promise(resolve => {
+    setTimeout(resolve, 3000);
+  });
+
+  // Assert.notEqual truncates the strings, so it is unusable here for failure
+  // debugging purposes.
+  let toolbarShot2 = TestUtils.screenshotArea(toolbarElt, window);
+  if (toolbarShot1 != toolbarShot2) {
+    info("Before toolbar: " + toolbarShot1);
+    info("After toolbar: " + toolbarShot2);
+  }
+  Assert.notEqual(toolbarShot1, toolbarShot2, "The UI should have updated");
+
+  let sidebarShot2 = TestUtils.screenshotArea(sidebarRect, window);
+  if (sidebarShot1 != sidebarShot2) {
+    info("Before sidebar: " + sidebarShot1);
+    info("After sidebar: " + sidebarShot2);
+  }
+  Assert.notEqual(sidebarShot1, sidebarShot2, "The UI should have updated");
+});
+
+/**
+ * Get Element for a bookmark in the bookmarks toolbar.
+ *
+ * @param guid
+ *        GUID of the item to search.
+ * @returns DOM Node of the element.
+ */
+function getNodeForToolbarItem(guid) {
+  return Array.from(document.getElementById("PlacesToolbarItems").childNodes)
+              .find(child => child._placesNode && child._placesNode.bookmarkGuid == guid);
+}
+
+/**
+ * Get a rect for a bookmark in the bookmarks sidebar
+ *
+ * @param guid
+ *        GUID of the item to search.
+ * @returns DOM Node of the element.
+ */
+function* getRectForSidebarItem(guid) {
+  let itemId = yield PlacesUtils.promiseItemId(guid);
+  let sidebar = document.getElementById("sidebar");
+  let tree = sidebar.contentDocument.getElementById("bookmarks-view");
+  tree.selectItems([itemId]);
+  let rect = {};
+  [rect.left, rect.top, rect.width, rect.height] = tree.treeBoxObject
+                                                       .selectionRegion
+                                                       .getRects();
+  // Adjust the position for the sidebar.
+  rect.left += sidebar.getBoundingClientRect().left;
+  rect.top += sidebar.getBoundingClientRect().top;
+  return rect;
+}
new file mode 100644
index 0000000000000000000000000000000000000000..62b69a3d031f616ad622eb67aa0e57bd400be50d
GIT binary patch
literal 286
zc$@(q0pb3MP)<h;3K|Lk000e1NJLTq000mG000mO1^@s6AM^iV0002xNkl<ZILpP;
zy-Gtt5C!0GC17mCpIS&M*edt{c6kYVE1${Q!Xljn?8VkJK7xr>7RDS4jk(vv8$t>X
z4D2j3XLrublq9hRHmr%(+)YM!AOj51rgU(O14^rbAW!6q^zn!hHc5waLOCa=ly)U&
zipenfxJ<@7$=DxumQ3!G$@@=QMT0wXjhBXsZ^;c_7l@eNcg=HU*8TMGyog|cx3A;)
zm##H`7PqvGrM1c&ltXMaygWrcu0qho-f|5}7yHZG$jqABIO8t6xWG2)%eXQ~Dud*x
k_N6knOe>CZ`S)%71OS<-E}v@0ssI2007*qoM6N<$g2c>!G5`Po
--- a/browser/components/places/tests/browser/head.js
+++ b/browser/components/places/tests/browser/head.js
@@ -1,14 +1,16 @@
 XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
   "resource://gre/modules/NetUtil.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Promise",
   "resource://gre/modules/Promise.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesTestUtils",
   "resource://testing-common/PlacesTestUtils.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "TestUtils",
+  "resource://testing-common/TestUtils.jsm");
 
 // Imported via PlacesOverlay.xul
 /* global doGetPlacesControllerForCommand:false, PlacesControllerDragHelper:false,
           PlacesUIUtils:false
 */
 
 // We need to cache this before test runs...
 var cachedLeftPaneFolderIdGetter;
--- a/dom/webidl/TreeBoxObject.webidl
+++ b/dom/webidl/TreeBoxObject.webidl
@@ -199,9 +199,15 @@ interface TreeBoxObject : BoxObject {
    * Notify the tree that the view has completed a batch update.
    */
   void endUpdateBatch();
 
   /**
    * Called on a theme switch to flush out the tree's style and image caches.
    */
   void clearStyleAndImageCaches();
+
+  /**
+   * Remove an image source from the image cache to allow its invalidation.
+   */
+  [Throws]
+  void removeImageCacheEntry(long row, TreeColumn col);
 };
--- a/layout/xul/tree/TreeBoxObject.cpp
+++ b/layout/xul/tree/TreeBoxObject.cpp
@@ -662,16 +662,34 @@ NS_IMETHODIMP
 TreeBoxObject::ClearStyleAndImageCaches()
 {
   nsTreeBodyFrame* body = GetTreeBodyFrame();
   if (body)
     return body->ClearStyleAndImageCaches();
   return NS_OK;
 }
 
+NS_IMETHODIMP
+TreeBoxObject::RemoveImageCacheEntry(int32_t aRowIndex, nsITreeColumn* aCol)
+{
+  NS_ENSURE_ARG(aCol);
+  NS_ENSURE_TRUE(aRowIndex >= 0, NS_ERROR_INVALID_ARG);
+  nsTreeBodyFrame* body = GetTreeBodyFrame();
+  if (body) {
+    return body->RemoveImageCacheEntry(aRowIndex, aCol);
+  }
+  return NS_OK;
+}
+
+void
+TreeBoxObject::RemoveImageCacheEntry(int32_t row, nsITreeColumn& col, ErrorResult& aRv)
+{
+  aRv = RemoveImageCacheEntry(row, &col);
+}
+
 void
 TreeBoxObject::ClearCachedValues()
 {
   mTreeBody = nullptr;
 }
 
 JSObject*
 TreeBoxObject::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
--- a/layout/xul/tree/TreeBoxObject.h
+++ b/layout/xul/tree/TreeBoxObject.h
@@ -70,16 +70,18 @@ public:
 
   already_AddRefed<DOMRect> GetCoordsForCellItem(int32_t row,
                                                  nsTreeColumn& col,
                                                  const nsAString& element,
                                                  ErrorResult& aRv);
 
   bool IsCellCropped(int32_t row, nsITreeColumn* col, ErrorResult& aRv);
 
+  void RemoveImageCacheEntry(int32_t row, nsITreeColumn& col, ErrorResult& aRv);
+
   // Deprecated APIs from old IDL
   void GetCellAt(JSContext* cx,
                  int32_t x, int32_t y,
                  JS::Handle<JSObject*> rowOut,
                  JS::Handle<JSObject*> colOut,
                  JS::Handle<JSObject*> childEltOut,
                  ErrorResult& aRv);
 
--- a/layout/xul/tree/nsITreeBoxObject.idl
+++ b/layout/xul/tree/nsITreeBoxObject.idl
@@ -43,17 +43,17 @@ interface nsITreeBoxObject : nsISupports
   readonly attribute long rowHeight;
 
   /**
    * Obtain the width of a row.
    */
   readonly attribute long rowWidth;
 
   /**
-   * Get the pixel position of the horizontal scrollbar. 
+   * Get the pixel position of the horizontal scrollbar.
    */
   readonly attribute long horizontalPosition;
 
   /**
    * Return the region for the visible parts of the selection, in device pixels.
    */
   readonly attribute nsIScriptableRegion selectionRegion;
 
@@ -181,9 +181,17 @@ interface nsITreeBoxObject : nsISupports
    * Notify the tree that the view has completed a batch update.
    */
   void endUpdateBatch();
 
   /**
    * Called on a theme switch to flush out the tree's style and image caches.
    */
   void clearStyleAndImageCaches();
+
+  /**
+   * Remove an image source from the image cache to allow its invalidation.
+   *
+   * @note This only affects images supplied by the view, not the ones supplied
+   *       through the styling context, like twisties or checkboxes.
+   */
+  void removeImageCacheEntry(in long row, in nsITreeColumn col);
 };
--- a/layout/xul/tree/nsTreeBodyFrame.cpp
+++ b/layout/xul/tree/nsTreeBodyFrame.cpp
@@ -1104,17 +1104,17 @@ nsTreeBodyFrame::GetCoordsForCellItem(in
   *aX = 0;
   *aY = 0;
   *aWidth = 0;
   *aHeight = 0;
 
   bool isRTL = StyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL;
   nscoord currX = mInnerBox.x - mHorzPosition;
 
-  // The Rect for the requested item. 
+  // The Rect for the requested item.
   nsRect theRect;
 
   nsPresContext* presContext = PresContext();
 
   for (nsTreeColumn* currCol = mColumns->GetFirstColumn(); currCol; currCol = currCol->GetNext()) {
 
     // The Rect for the current cell.
     nscoord colWidth;
@@ -4524,16 +4524,32 @@ nsresult
 nsTreeBodyFrame::ClearStyleAndImageCaches()
 {
   mStyleCache.Clear();
   CancelImageRequests();
   mImageCache.Clear();
   return NS_OK;
 }
 
+nsresult
+nsTreeBodyFrame::RemoveImageCacheEntry(int32_t aRowIndex, nsITreeColumn* aCol)
+{
+  nsAutoString imageSrc;
+  if (NS_SUCCEEDED(mView->GetImageSrc(aRowIndex, aCol, imageSrc))) {
+    nsTreeImageCacheEntry entry;
+    if (mImageCache.Get(imageSrc, &entry)) {
+      nsLayoutUtils::DeregisterImageRequest(PresContext(), entry.request,
+                                            nullptr);
+      entry.request->CancelAndForgetObserver(NS_BINDING_ABORTED);
+      mImageCache.Remove(imageSrc);
+    }
+  }
+  return NS_OK;
+}
+
 /* virtual */ void
 nsTreeBodyFrame::DidSetStyleContext(nsStyleContext* aOldStyleContext)
 {
   nsLeafBoxFrame::DidSetStyleContext(aOldStyleContext);
 
   // Clear the style cache; the pointers are no longer even valid
   mStyleCache.Clear();
   // XXX The following is hacky, but it's not incorrect,
--- a/layout/xul/tree/nsTreeBodyFrame.h
+++ b/layout/xul/tree/nsTreeBodyFrame.h
@@ -113,16 +113,17 @@ public:
                                 const nsACString &aElt,
                                 int32_t *aX, int32_t *aY,
                                 int32_t *aWidth, int32_t *aHeight);
   nsresult IsCellCropped(int32_t aRow, nsITreeColumn *aCol, bool *aResult);
   nsresult RowCountChanged(int32_t aIndex, int32_t aCount);
   nsresult BeginUpdateBatch();
   nsresult EndUpdateBatch();
   nsresult ClearStyleAndImageCaches();
+  nsresult RemoveImageCacheEntry(int32_t aRowIndex, nsITreeColumn* aCol);
 
   void CancelImageRequests();
 
   void ManageReflowCallback(const nsRect& aRect, nscoord aHorzWidth);
 
   virtual nsSize GetXULMinSize(nsBoxLayoutState& aBoxLayoutState) override;
   virtual void SetXULBounds(nsBoxLayoutState& aBoxLayoutState, const nsRect& aRect,
                             bool aRemoveOverflowArea = false) override;
--- a/testing/modules/TestUtils.jsm
+++ b/testing/modules/TestUtils.jsm
@@ -56,9 +56,32 @@ this.TestUtils = {
           resolve([subject, data]);
         } catch (ex) {
           Services.obs.removeObserver(observer, topic);
           reject(ex);
         }
       }, topic);
     });
   },
+
+  /**
+   * Takes a screenshot of an area and returns it as a data URL.
+   *
+   * @param eltOrRect
+   *        The DOM node or rect ({left, top, width, height}) to screenshot.
+   * @param win
+   *        The current window.
+   */
+  screenshotArea(eltOrRect, win) {
+    if (eltOrRect instanceof Ci.nsIDOMElement) {
+      eltOrRect = eltOrRect.getBoundingClientRect();
+    }
+    let { left, top, width, height } = eltOrRect;
+    let canvas = win.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
+    let ctx = canvas.getContext("2d");
+    let ratio = win.devicePixelRatio;
+    canvas.width = width * ratio;
+    canvas.height = height * ratio;
+    ctx.scale(ratio, ratio);
+    ctx.drawWindow(win, left, top, width, height, "#fff");
+    return canvas.toDataURL();
+  }
 };