Bug 1368754 - Change `NodeHistoryDetailsChanged` and `NodeURIChanged` to be called with old time,access count and uri, r?mak draft
authormilindl <i.milind.luthra@gmail.com>
Tue, 30 May 2017 23:46:59 +0530
changeset 589415 861e5061bff2d0ea9c0c0ffbd273d375b326abf2
parent 589150 cad53f061da634a16ea75887558301b77f65745d
child 589416 a96d0161202d0eaafe4e55329e047883a7fcb4c7
child 589426 f990ccda023108242504b93257a29a25fbc3f558
child 589444 f3f35459e24027807888f1b525c9832275c1cb15
push id62360
push userbmo:i.milind.luthra@gmail.com
push dateTue, 06 Jun 2017 06:16:01 +0000
reviewersmak
bugs1368754
milestone55.0a1
Bug 1368754 - Change `NodeHistoryDetailsChanged` and `NodeURIChanged` to be called with old time,access count and uri, r?mak Update interface and all instances where the method is called to be called with the old values, since the new values are already there as a part of the node, and thus redundant. MozReview-Commit-ID: 5pcfJbg9tej
toolkit/components/places/nsINavHistoryService.idl
toolkit/components/places/nsLivemarkService.js
toolkit/components/places/nsNavHistoryResult.cpp
--- a/toolkit/components/places/nsINavHistoryService.idl
+++ b/toolkit/components/places/nsINavHistoryService.idl
@@ -371,20 +371,20 @@ interface nsINavHistoryResultObserver : 
                         in AUTF8String aNewTitle);
 
   /**
    * Called right after aNode's uri property has changed.
    * 
    * @param aNode
    *        a result node
    * @param aNewURI
-   *        the new uri
+   *        the old uri
    */
   void nodeURIChanged(in nsINavHistoryResultNode aNode,
-                      in AUTF8String aNewURI);
+                      in AUTF8String aOldURI);
 
   /**
    * Called right after aNode's icon property has changed.
    *
    * @param aNode
    *        a result node
    *
    * @note: The new icon is accessible through aNode.icon.
@@ -392,24 +392,24 @@ interface nsINavHistoryResultObserver : 
   void nodeIconChanged(in nsINavHistoryResultNode aNode);
 
   /**
    * Called right after aNode's time property or accessCount property, or both,
    * have changed.
    *
    * @param aNode
    *        a uri result node
-   * @param aNewVisitDate
-   *        the new visit date
-   * @param aNewAccessCount
-   *        the new access-count
+   * @param aOldVisitDate
+   *        the old visit date
+   * @param aOldAccessCount
+   *        the old access-count
    */
   void nodeHistoryDetailsChanged(in nsINavHistoryResultNode aNode,
-                                 in PRTime aNewVisitDate,
-                                 in unsigned long aNewAccessCount);
+                                 in PRTime aOldVisitDate,
+                                 in unsigned long aOldAccessCount);
 
   /**
    * Called when the tags set on the uri represented by aNode have changed.
    *
    * @param aNode
    *        a uri result node
    *
    * @note: The new tags list is accessible through aNode.tags.
--- a/toolkit/components/places/nsLivemarkService.js
+++ b/toolkit/components/places/nsLivemarkService.js
@@ -658,29 +658,31 @@ Livemark.prototype = {
    *
    * @param aURI
    *        If provided will update nodes having the given uri,
    *        otherwise any node.
    * @param aVisitedStatus
    *        Whether the nodes should be set as visited.
    */
   updateURIVisitedStatus(aURI, aVisitedStatus) {
+    let wasVisited = false;
     for (let child of this.children) {
       if (!aURI || child.uri.equals(aURI)) {
+        wasVisited = child.visited;
         child.visited = aVisitedStatus;
       }
     }
 
     for (let [ container, observer ] of this._resultObservers) {
       if (this._nodes.has(container)) {
         let nodes = this._nodes.get(container);
         for (let node of nodes) {
           if (!aURI || node.uri == aURI.spec) {
             Services.tm.dispatchToMainThread(() => {
-              observer.nodeHistoryDetailsChanged(node, 0, aVisitedStatus);
+              observer.nodeHistoryDetailsChanged(node, node.time, wasVisited);
             });
           }
         }
       }
     }
   },
 
   /**
--- a/toolkit/components/places/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/nsNavHistoryResult.cpp
@@ -653,28 +653,31 @@ nsNavHistoryContainerResultNode::FillSta
 nsresult
 nsNavHistoryContainerResultNode::ReverseUpdateStats(int32_t aAccessCountChange)
 {
   if (mParent) {
     nsNavHistoryResult* result = GetResult();
     bool shouldNotify = result && mParent->mParent &&
                           mParent->mParent->AreChildrenVisible();
 
+    uint32_t oldAccessCount = mParent->mAccessCount;
+    PRTime oldTime = mParent->mTime;
+
     mParent->mAccessCount += aAccessCountChange;
     bool timeChanged = false;
     if (mTime > mParent->mTime) {
       timeChanged = true;
       mParent->mTime = mTime;
     }
 
     if (shouldNotify) {
       NOTIFY_RESULT_OBSERVERS(result,
                               NodeHistoryDetailsChanged(TO_ICONTAINER(mParent),
-                                                        mParent->mTime,
-                                                        mParent->mAccessCount));
+                                                        oldTime,
+                                                        oldAccessCount));
     }
 
     // check sorting, the stats may have caused this node to move if the
     // sorting depended on something we are changing.
     uint16_t sortMode = mParent->GetSortType();
     bool sortingByVisitCount =
       sortMode == nsINavHistoryQueryOptions::SORT_BY_VISITCOUNT_ASCENDING ||
       sortMode == nsINavHistoryQueryOptions::SORT_BY_VISITCOUNT_DESCENDING;
@@ -1336,24 +1339,27 @@ nsNavHistoryContainerResultNode::InsertC
     container->mResult = result;
     container->FillStats();
   }
 
   if (!mChildren.InsertObjectAt(aNode, aIndex))
     return NS_ERROR_OUT_OF_MEMORY;
 
   // Update our stats and notify the result's observers.
+  uint32_t oldAccessCount = mAccessCount;
+  PRTime oldTime = mTime;
+
   mAccessCount += aNode->mAccessCount;
   if (mTime < aNode->mTime)
     mTime = aNode->mTime;
   if (!mParent || mParent->AreChildrenVisible()) {
     NOTIFY_RESULT_OBSERVERS(result,
                             NodeHistoryDetailsChanged(TO_ICONTAINER(this),
-                                                      mTime,
-                                                      mAccessCount));
+                                                      oldTime,
+                                                      oldAccessCount));
   }
 
   nsresult rv = ReverseUpdateStats(aNode->mAccessCount);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Update tree if we are visible.  Note that we could be here and not
   // expanded, like when there is a bookmark folder being updated because its
   // parent is visible.
@@ -1562,28 +1568,31 @@ nsNavHistoryContainerResultNode::UpdateU
     nsNavHistoryContainerResultNode* parent = node->mParent;
     if (!parent) {
       MOZ_ASSERT(false, "All URI nodes being updated must have parents");
       continue;
     }
 
     uint32_t oldAccessCount = node->mAccessCount;
     PRTime oldTime = node->mTime;
+    uint32_t parentOldAccessCount = parent->mAccessCount;
+    PRTime parentOldTime = parent->mTime;
+
     aCallback(node, aClosure, result);
 
     if (oldAccessCount != node->mAccessCount || oldTime != node->mTime) {
       parent->mAccessCount += node->mAccessCount - oldAccessCount;
       if (node->mTime > parent->mTime)
         parent->mTime = node->mTime;
       if (parent->AreChildrenVisible()) {
         NOTIFY_RESULT_OBSERVERS_RET(result,
                                     NodeHistoryDetailsChanged(
                                       TO_ICONTAINER(parent),
-                                      parent->mTime,
-                                      parent->mAccessCount),
+                                      parentOldTime,
+                                      parentOldAccessCount),
                                     true);
       }
       DebugOnly<nsresult> rv = parent->ReverseUpdateStats(node->mAccessCount - oldAccessCount);
       MOZ_ASSERT(NS_SUCCEEDED(rv), "should be able to ReverseUpdateStats");
     }
 
     if (aUpdateSort) {
       int32_t childIndex = parent->FindChild(node);
@@ -3746,29 +3755,31 @@ nsNavHistoryResultNode::OnItemChanged(in
     // XXX: what should we do if the new title is void?
     mTitle = aNewValue;
     if (shouldNotify)
       NOTIFY_RESULT_OBSERVERS(result, NodeTitleChanged(this, mTitle));
   }
   else if (aProperty.EqualsLiteral("uri")) {
     // clear the tags string as well
     mTags.SetIsVoid(true);
+    nsCString oldURI(mURI);
     mURI = aNewValue;
     if (shouldNotify)
-      NOTIFY_RESULT_OBSERVERS(result, NodeURIChanged(this, mURI));
+      NOTIFY_RESULT_OBSERVERS(result, NodeURIChanged(this, oldURI));
   }
   else if (aProperty.EqualsLiteral("favicon")) {
     if (shouldNotify)
       NOTIFY_RESULT_OBSERVERS(result, NodeIconChanged(this));
   }
   else if (aProperty.EqualsLiteral("cleartime")) {
+    PRTime oldTime = mTime;
     mTime = 0;
     if (shouldNotify) {
       NOTIFY_RESULT_OBSERVERS(result,
-                              NodeHistoryDetailsChanged(this, 0, mAccessCount));
+                              NodeHistoryDetailsChanged(this, oldTime, mAccessCount));
     }
   }
   else if (aProperty.EqualsLiteral("tags")) {
     mTags.SetIsVoid(true);
     if (shouldNotify)
       NOTIFY_RESULT_OBSERVERS(result, NodeTagsChanged(this));
   }
   else if (aProperty.EqualsLiteral("dateAdded")) {
@@ -3853,16 +3864,18 @@ nsNavHistoryFolderResultNode::OnItemVisi
     return NS_OK;
 
   uint32_t nodeIndex;
   nsNavHistoryResultNode* node = FindChildById(aItemId, &nodeIndex);
   if (!node)
     return NS_ERROR_FAILURE;
 
   // Update node.
+  uint32_t nodeOldAccessCount = node->mAccessCount;
+  PRTime nodeOldTime = node->mTime;
   node->mTime = aTime;
   ++node->mAccessCount;
 
   // Update us.
   int32_t oldAccessCount = mAccessCount;
   ++mAccessCount;
   if (aTime > mTime)
     mTime = aTime;
@@ -3880,17 +3893,17 @@ nsNavHistoryFolderResultNode::OnItemVisi
   NS_ENSURE_SUCCESS(rv, rv);
   NS_ENSURE_STATE(visitNode);
   node->mFrecency = visitNode->mFrecency;
 
   if (AreChildrenVisible()) {
     // Sorting has not changed, just redraw the row if it's visible.
     nsNavHistoryResult* result = GetResult();
     NOTIFY_RESULT_OBSERVERS(result,
-                            NodeHistoryDetailsChanged(node, mTime, mAccessCount));
+                            NodeHistoryDetailsChanged(node, nodeOldTime, nodeOldAccessCount));
   }
 
   // Update sorting if necessary.
   uint32_t sortType = GetSortType();
   if (sortType == nsINavHistoryQueryOptions::SORT_BY_VISITCOUNT_ASCENDING ||
       sortType == nsINavHistoryQueryOptions::SORT_BY_VISITCOUNT_DESCENDING ||
       sortType == nsINavHistoryQueryOptions::SORT_BY_DATE_ASCENDING ||
       sortType == nsINavHistoryQueryOptions::SORT_BY_DATE_DESCENDING ||