543444 - Replace single-view API with multiple observers

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Asaf Romano (gone)]

Attachment: v1.

This is a requirement for bug 536893.

I still need to update the tests

Comment 1

[Marco Bonardo [:mak]]

hm, i am a bit worried about compatibility, i thought about retaining the nsINavHistoryResultViewer interface name, use addResultViewer and removeResultViewer, but also leave in the old SetViewer and GetViewer that will act on the first viewer in the viewers array. This way we would preserve compatibility with add-ons and current views... But since this piece of code won't make before Firefox 4.0 that is a major release known to break things, i should probably ignore the problem. And we are probably also already breaking treeviews in the lazy treeview patch.
Thoughts about this?

Comment 2

[Asaf Romano (gone)]

With async turned on by default and paged-treeview, I think we're breaking enough for not duplicating APIs.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 424579 [details] [diff] [review]
v1.

Not ready for final review (tests missing and some stuff to change), so skipping flag set

>diff --git a/browser/base/content/sanitizeDialog.js b/browser/base/content/sanitizeDialog.js

>-    view.QueryInterface(Ci.nsINavHistoryResultViewer).result.viewer = null;
>+    view.QueryInterface(Ci.nsINavHistoryResultObserver)
>+        .result.removeObserver(view);

this piece of recursive code is awesome.

>diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js

>   setDataTransfer: function PC_setDataTransfer(aEvent) {

>+    result.notificationsEnabled = false;
>     try {

>     }
>     finally {
>-      if (oldViewer)
>-        result.viewer = oldViewer;
>+      result.notificationsEnabled = true;
>     }

here and later, can we be sure every time that notificationsEnabled was true before starting? shouldn't we cache it and restore old value just in case? Or the usage is so "special-cased" that is up to the implementer being sure to not call methods that could set it?
In this case will at least need a comment in the idl.

>diff --git a/browser/components/places/content/menu.xml b/browser/components/places/content/menu.xml

>-          if (this._self._result != val) {

why removing this check?

>diff --git a/browser/components/places/content/toolbar.xml b/browser/components/places/content/toolbar.xml

>-          if (this._self._result != val) {

why removing this check?

>diff --git a/browser/components/places/content/tree.xml b/browser/components/places/content/tree.xml

>+          try {
>+          result.addObserver(treeView, false);
>           this.view = treeView;
>           if (!this._controller) {
>             this._controller = new PlacesController(this);
>             this.controllers.appendController(this._controller);
>           }
>           this._cachedInsertionPoint = undefined;
>+          }
>+          catch(ex) {alert(ex); }

debug stuff

>diff --git a/browser/components/places/content/treeView.js b/browser/components/places/content/treeView.js

>   set result(val) {
>-    if (this._result != val) {
>-      if (this._result)
>-        this._rootNode.containerOpen = false;
>+    if (this._result)
>+      this._rootNode.containerOpen = false;

why did you remove the this._result != val check?

>diff --git a/toolkit/components/places/public/nsINavHistoryService.idl b/toolkit/components/places/public/nsINavHistoryService.idl

> [scriptable, uuid(af4ac418-a687-4775-8ffa-97c160196432)]
>-interface nsINavHistoryResultViewer : nsISupports
>+interface nsINavHistoryResultObserver : nsISupports

i think changing the interface name is asking for a uuid rev

> [scriptable, uuid(fa77e4e9-9fc8-45d2-9507-0fe4f0602505)]
>-interface nsINavHistoryResultTreeViewer : nsINavHistoryResultViewer
>+interface nsINavHistoryResultTreeViewer : nsINavHistoryResultObserver

ditto, since this inherits from a new interface

>@@ -673,17 +669,17 @@ interface nsINavHistoryResultTreeViewer 
>  *             |
>  *             +-- nsINavHistoryResultViewer
>  *
>  * The result indicates to the view when something changes through the
>  * nsINavHistoryResultViewer interface. The viewer is set through
>  * the nsINavHistoryResult.viewer property.

all of these comments are clearly wrong now since talking about "viewer".
Double check comments in the idl and Places codebase for "viewer" please.

>+   * Weather or not observers are notified on result changes, initially set to

typo: "Weather" -> "whether"

>+   * true.
>+   *
>+   * Some front-end code use this to improve perfomance when temporary changes
>+   * are done.

typo: "use" -> "uses"
Btw, this comment should instead tell that whoever has a view and needs to to temporary changes to
the result that does not want to see in the view (like recursive searches in closed nodes) should
set it to false. Not just for performance but also for "flickering"

>-  attribute nsINavHistoryResultViewer viewer;
>+  attribute boolean notificationsEnabled;

hm, what about just notifyObservers?

>+  /**
>+   * Observe changes in the result.

Adds a result observer that will observer changes...

>+   *
>+   * @param aObserver
>+   *        a result observer.  @see nsINavHistoryResultObserver

move the @see after @params

>+   * @param aOwnsWeak
>+   *        If false, the result will keep an owning reference to the observer,
>+   *        which must be removed using removeObserver.
>+   *        If true, the result will keep a weak reference to the observer, which
>+   *        must implement nsISupportsWeakReference.
>+   */
>+  void addObserver(in nsINavHistoryResultObserver aObserver, in boolean aOwnsWeak);
>+
>+  /**
>+   * Remove an observer that was added by addObserver.

Removes a result observer that was added...

>diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp

>   nsNavHistoryResult* result = GetResult();
>   NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
>-  if (result->GetView())
>-    result->GetView()->ContainerOpened(this);
>+  ENUMERATE_WEAKARRAY(result->mObservers, nsINavHistoryResultObserver,
>+                      ContainerOpened(
>+                       static_cast<nsNavHistoryContainerResultNode*>(this)))

shouldn't you check if notifications are enabled? We use this feature to avoid flickering open/close status of nodes when recursive searching inside them iirc.

please add a #define NOTIFY_RESULT_OBSERVERS(_notification) ENUMERATE_WEAKARRAY(result->mObservers, nsINavHistoryResultObserver, _notification) on top and use the former.

also would be cool to have something like a CAST_NODE_TO_CONTAINER(_node) static_cast<nsNavHistoryContainerResultNode*>(_node)
and other macros to avoid these long static_cast constructs.

so this would end up being:
NOTIFY_RESULT_OBSERVERS(ContainerOpened(CAST_NODE_TO_CONTAINER(this));

ditto for all other instances.

>+    ENUMERATE_WEAKARRAY(result->mObservers,
>+                        nsINavHistoryResultObserver,
>+                        NodeMoved(node, this, aIndex, this, newIndex))

always end macros funcs with a seimicolon like other funcs

>   if (! aIsTemporary) {

nit: while here, fix this space

>@@ -1745,28 +1767,31 @@ nsNavHistoryContainerResultNode::UpdateU

>-    PRBool childrenVisible = result->GetView() != nsnull && parent->AreChildrenVisible();
>+    PRBool childrenVisible = parent->AreChildrenVisible();
> 
>     if (oldAccessCount != node->mAccessCount || oldTime != node->mTime) {
>       // need to update/redraw the parent
>       parent->mAccessCount += node->mAccessCount - oldAccessCount;
>       if (node->mTime > parent->mTime)
>         parent->mTime = node->mTime;
>-      if (childrenVisible)
>-        result->GetView()->NodeHistoryDetailsChanged(
>-            static_cast<nsINavHistoryContainerResultNode*>(parent),
>-            parent->mTime,
>-            parent->mAccessCount);
>+      if (result->mNotificationsEnabled && childrenVisible) {

at this point the initial childrenVisible assignment looks useless, just put parent->AreChildrenVisible here, unless it's used later

>@@ -3499,17 +3536,17 @@ nsNavHistoryFolderResultNode::StartIncre

>     // when a tree is attached also do incremental updates if our parent is
>     // visible so that twisties are drawn correctly.
>-    if (mParent && result->GetView())
>+    if (mParent)
>       return PR_TRUE;

ehr, this looks different, should we check if we have any observer?

> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsNavHistoryResult)
>   tmp->StopObserving();
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mRootNode)
>-  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mView)
>   tmp->mBookmarkFolderObservers.Enumerate(&RemoveBookmarkFolderObserversCallback, nsnull);
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSTARRAY(mAllBookmarksObservers)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSTARRAY(mHistoryObservers)

hm, i guess if we should unlink our new result observers, at first glance i think not since i can't think of possible cycles we don't already break. but we were unlinking the view, that's not much different from an owning pointer to an observer.
This change increases our dependancy on users having to recall to remove their observers.

>+NS_IMETHODIMP
>+nsNavHistoryResult::AddObserver(nsINavHistoryResultObserver* aObserver,
>+                                PRBool aOwnsWeak)
>+{
>+  NS_ENSURE_ARG(aObserver);
>+  printf("foo");

recall to remove fancy dumps :)

>diff --git a/toolkit/components/places/src/nsNavHistoryResult.h b/toolkit/components/places/src/nsNavHistoryResult.h
>--- a/toolkit/components/places/src/nsNavHistoryResult.h
>+++ b/toolkit/components/places/src/nsNavHistoryResult.h
>@@ -45,16 +45,17 @@
> #ifndef nsNavHistoryResult_h_
> #define nsNavHistoryResult_h_
> 
> #include "nsTArray.h"
> #include "nsInterfaceHashtable.h"
> #include "nsDataHashtable.h"
> #include "nsCycleCollectionParticipant.h"
> 
>+

useless change

> 
>-  nsCOMPtr<nsINavHistoryResultViewer> mView;
>+  nsMaybeWeakPtrArray<nsINavHistoryResultObserver> mObservers;
>+  PRBool mNotificationsEnabled;

The order matters here, you are putting mNotificationsEnabled before mBatchInProgress, but the constructor is initing them in reverse order, GCC will WARN (on linux at least).

Comment 4

[Asaf Romano (gone)]

>>diff --git a/browser/components/places/content/menu.xml b/browser/components/places/content/menu.xml
>>-          if (this._self._result != val) {
>why removing this check?
>>diff --git a/browser/components/places/content/toolbar.xml b/browser/components/places/content/toolbar.xml
>>-          if (this._self._result != val) {
>why removing this check?

Because this was an optimization for a use-case that shouldn't happen anymore (unsetting the view in order to disable notifications, then setting it back).

Comment 5

[Asaf Romano (gone)]

> hm, i guess if we should unlink our new result observers, at first glance i
> think not since i can't think of possible cycles we don't already break. but we
> were unlinking the view, that's not much different from an owning pointer to an
> observer.

I *think* we did that back when treeview had been implemented in c++. I cannot see a possible cycle here, and thus not adding the unlinkage code back.

Comment 6

[Asaf Romano (gone)]

Attachment: v1.5

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 428205 [details] [diff] [review]
v1.5

>diff -r c99b9a4940e4 browser/components/places/content/controller.js

>   setDataTransfer: function PC_setDataTransfer(aEvent) {
>+    let doDisableNotifications = result.notifyObservers;

hm, can we call this suppressNotifications?

>diff -r c99b9a4940e4 browser/components/places/content/toolbar.xml

>+          if (val) {
>+            this._self._resultNode = val.root;
>+            this._self._resultNode._DOMElement = this._self;
>+            // This calls _rebuild through invalidateContainer.
>+            this._self._resultNode.containerOpen = true;
>           }
>+          else
>+            this._self._resultNode = null;

per code style, if one part of an if/elseif/else gets braces, all parts will get braces (https://wiki.mozilla.org/Places/Coding_Style suggestions are welcome)

>diff -r c99b9a4940e4 browser/components/places/content/tree.xml

>-          // Null the viewer while looking for nodes

nit: period.

>+          // Disable notifications while looking for nodes

nit: period.

>+          let result = this.getResult();
>+          let doDisableNotifications = result.notifyObservers;

suppressNotifications please

>diff -r c99b9a4940e4 toolkit/components/places/public/nsINavHistoryService.idl

>+   * Adds an observer for changes done in the result.
>+   *
>+   * @param aObserver
>+   *        a result observer.
>+   * @param aOwnsWeak
>+   *        If false, the result will keep an owning reference to the observer,
>+   *        which must be removed using removeObserver.
>+   *        If true, the result will keep a weak reference to the observer, which
>+   *        must implement nsISupportsWeakReference.
>+   *
>+   * @see nsINavHistoryResultObserver
>+   */
>+  void addObserver(in nsINavHistoryResultObserver aObserver, in boolean aOwnsWeak);

each param on its own line please

>diff -r c99b9a4940e4 toolkit/components/places/src/nsNavHistoryResult.cpp
>+#define TO_ICONTAINER(_node)                                              \
>+    static_cast<nsINavHistoryContainerResultNode*>(_node)                      
>+
>+#define TO_CONTAINER(_node)                                              \
>+    static_cast<nsNavHistoryContainerResultNode*>(_node)
>+#define NOTIFY_RESULT_OBSERVERS(_result, _method)                             \

newline between defines

>+  PR_BEGIN_MACRO                                                              \
>+  if (_result->mNotifyObservers)                                               \
>+    ENUMERATE_WEAKARRAY(_result->mObservers, nsINavHistoryResultObserver,     \
>+                        _method)                                              \

since ideally we don't know what the inside macros expand to, it's always better to use braces in macros ifs.

>+  PR_END_MACRO;

no semicolon after PR_END_MACRO

the "\"es of the macros are in random positions, either put all of them at the 80 char limit, or just 1 space after the end of the text

>-  if (! mChildren.InsertObjectAt(aNode, aIndex))
>-    return NS_ERROR_OUT_OF_MEMORY;
>-
>-  // update our (the container) stats and refresh our row on the screen
>-  if (! aIsTemporary) {
>+  if (mChildren.InsertObjectAt(aNode, aIndex))
>+    return NS_ERROR_OUT_OF_MEMORY;

the check has been inverted, since the NOT disappeared

>+    if (!mParent || mParent->AreChildrenVisible()) {
>+      NOTIFY_RESULT_OBSERVERS(result,
>+                              NodeHistoryDetailsChanged(
>+                                TO_ICONTAINER(this), mTime, mAccessCount));

80 chars limit is strongly suggested but not mandatory, i think in this case is better having

      NOTIFY_RESULT_OBSERVERS(result,
                              NodeHistoryDetailsChanged(TO_ICONTAINER(this),
                                                        mTime,
                                                        mAccessCount));
regardless the limit.

> //  This checks if the item at aIndex is located correctly given the sorting
>-//  move. If it's not, the item is moved, and the result view are notified.
>+//  move. If it's not, the item is moved, and the result's observers are
>+//  notified.

nit: 2 spaces after period
> //
> //  Returns true if the item position has been changed, false otherwise.

all of this sounds like a bad javadoc, what about converting it?

> // nsNavHistoryContainerResultNode::ReplaceChildURIAt
> //
> //    This is called to replace a leaf node. It will update tree stats
>-//    and redraw the view if any. You can not use this to replace a container.
>+//    and notify the result's observers. You can not use this to replace a
>+//    container.
> //
> //    This assumes that the node is being replaced with a newer version of
> //    itself and so its visit count will not go down. Also, this means that the
> //    collapsing of duplicates will not change.

ditto


>+  if (!mChildren.ReplaceObjectAt(aNode, aIndex))
>+    return NS_ERROR_FAILURE;
>+
>+  // Notify the result's observers.
>+  nsNavHistoryResult* result = GetResult();
>+  NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);

this sounds like NS_ENSURE_STATE(result);
this probably can be done in the above changes too, so check full patch.


>-  // stats
>+  // Stats

since changing, what about "Update stats."?


>+      if (parent->AreChildrenVisible()) {
>+          NOTIFY_RESULT_OBSERVERS(result,
>+                                  NodeHistoryDetailsChanged(
>+                                    static_cast<nsINavHistoryContainerResultNode*>(parent),

TO_ICONTAINER?
and see above about 80 chars limit

>+  if (aResult &&  (!aNode->mParent || aNode->mParent->AreChildrenVisible()))
>+    NOTIFY_RESULT_OBSERVERS(aResult, NodeIconChanged(aNode));
>+}

there is a double space after &&

>-    // when a tree is attached also do incremental updates if our parent is
>-    // visible so that twisties are drawn correctly.
>-    if (mParent && result->GetView())
>-      return PR_TRUE;
>+    // When any observers are attached also do incremental updates if our
>+    // parent is visible so that twisties are drawn correctly.

comma after visible?


>+// nsNavHistoryResult::AddBookmarkFolderObserver
>+//
>+//    Here, we also attach as a bookmark service observer if necessary

what a useless comment :(


>+NS_IMETHODIMP
>+nsNavHistoryResult::AddObserver(nsINavHistoryResultObserver* aObserver,
>+                                PRBool aOwnsWeak)
>+{
>+  NS_ENSURE_ARG(aObserver);
>+  nsresult rv = mObservers.AppendWeakElement(aObserver, aOwnsWeak);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  rv = aObserver->SetResult(this);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsNavHistoryResult::RemoveObserver(nsINavHistoryResultObserver* aObserver)
>+{
>+  NS_ENSURE_ARG(aObserver);
>+  return mObservers.RemoveWeakElement(aObserver);
>+}
>+
>+NS_IMETHODIMP
>+nsNavHistoryResult::GetNotifyObservers(PRBool* _retval)
>+{
>+  *_retval = mNotifyObservers;
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsNavHistoryResult::SetNotifyObservers(PRBool aNotifyObservers)
>+{
>+  mNotifyObservers = aNotifyObservers;
>   return NS_OK;
> }

please, double newline between methods

>diff -r c99b9a4940e4 toolkit/components/places/src/utils.js

>+    let root = this.getContainerNodeWithOptions(aNode, false, true);
>+    let result = root.parentResult;
>+    let doDisableNotifications = false;
>+    let wasOpen = root.containerOpen;
>     if (!wasOpen) {
>-      root.parentResult.viewer = null;
>+      if (doDisableNotifications = result.notifyObservers)
>+        result.notifyObservers = false;
>+

this code looks wrong and scary, like the assignment in the if...
Why is it different from other uses where we just assign notifyObservers to suppressNotifications?
Could maybe be a bad merge

>     if (!wasOpen) {
>-      root.parentResult.viewer = null;
>+      root.parentResult.notificationsEnabled = false;

i guess being open is not == to being notifying. maybe the result is not notifying, but this is closed, and later we make it notifying...

>diff -r c99b9a4940e4 toolkit/components/places/tests/unit/test_451499.js

>+  // Observe our result
>+  let resultObserver = {

is this comment adding anything to the name of the object?


>+    itemChanged: function(item) {
>+      // The favicon should not be set on the containing query.
>+      if (item.uri.substr(0,5) == "place")
>+        dump("\nTesting itemChanged on: \n " + item.uri + "\n\n");

why so many \n? also you could use print() instead

as usual, get a tryserver green run on next iteration please.

Comment 8

[Asaf Romano (gone)]

Attachment: Code Style Update

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 429779 [details] [diff] [review]
Code Style Update

>diff --git a/browser/components/places/content/menu.xml b/browser/components/places/content/menu.xml
>+          if (val) {
>+            this._self._resultNode = val.root;
>+            this._self._resultNode._DOMElement = this._self.parentNode;
>           }
>+          else
>+            this._self._resultNode = null;

braces on this else please

>diff --git a/browser/components/places/content/tree.xml b/browser/components/places/content/tree.xml

>+          
>+          // Disable notifications while looking for nodes.

trailing spaces in the newline above the comment

>diff --git a/toolkit/components/places/public/nsINavHistoryService.idl b/toolkit/components/places/public/nsINavHistoryService.idl

>+   * Whether or not notifications on result changes are suppressed, initially
>+   * set to false.

replace comma with period, move "Initially set to false." in a new line, so it's more visible

>diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp

>-// nsNavHistoryResultNode ******************************************************
>-
>+// nsNavHistoryResultNode
> NS_IMPL_CYCLE_COLLECTION_CLASS(nsNavHistoryResultNode)

i'd say to just kill the comment, the name of the class is just one row below in the macro

>+
> NS_IMETHODIMP
> nsNavHistoryResultNode::GetParentResult(nsINavHistoryResult** aResult)
> {
>   *aResult = nsnull;
>-  if (IsContainer() && GetAsContainer()->mResult) {
>+  if (IsContainer() && GetAsContainer()->mResult)
>     NS_ADDREF(*aResult = GetAsContainer()->mResult);
>-  } else if (mParent && mParent->mResult) {
>+  else if (mParent && mParent->mResult)
>     NS_ADDREF(*aResult = mParent->mResult);

what about removing the second condition of the if/else if that is just null checking, and using NS_IF_ADDREF instead

>-  } else {
>+  else
>    return NS_ERROR_UNEXPECTED;

bad indent on this last line

>-// nsNavHistoryResultNode::GetResult
>-//
>-//    This will find the result for this node. We can ask the nearest container
>-//    for this value (either ourselves or our parents should be a container,
>-//    and all containers have result pointers).
>-
>+/**
>+ * nsNavHistoryResultNode::GetResult
>+ *
>+ * This will find the result for this node.  We can ask the nearest container
>+ * for this value (either ourselves or our parents should be a container,
>+ * and all containers have result pointers).
>+ */

remove method name from the comment, it's redundant

>+/**
>+ * nsNavHistoryResultNode::GetGeneratingOptions
>+ *

ditto

>+ * Searches up the tree for the closest node that has an options structure.
>+ * This will tell us the options that were used to generate this node.

closest is a bit generic, looks like it is just looking at the parent... so either parent node or ancestor nodes. 

>+ *
>+ * Be careful, this function walks up the tree, so it can not be used when
>+ * result nodes are created because they have no parent.  Only call this
>+ * function after the tree has been built.
>+ */

hm, really? does not look like it is walking the tree, am i missing something?

>-// nsNavHistoryVisitResultNode *************************************************
>-
>+// nsNavHistoryVisitResultNode

kill them!
 
>-// nsNavHistoryFullVisitResultNode *********************************************
>-
>+// nsNavHistoryFullVisitResultNode

kill...

>-// nsNavHistoryContainerResultNode *********************************************
>-
>+
>+// nsNavHistoryContainerResultNode

them!

> nsNavHistoryContainerResultNode::~nsNavHistoryContainerResultNode()
> {
>   // Explicitly clean up array of children of this container.  We must ensure
>   // all references are gone and all of their destructors are called.
>   mChildren.Clear();
> }
> 
>-// nsNavHistoryContainerResultNode::OnRemoving
>-//
>-//    Containers should notify their children that they are being removed
>-//    when the container is being removed.
>-
>+/**
>+ * nsNavHistoryContainerResultNode::OnRemoving
>+ *
>+ * Containers should notify their children that they are being removed when the
>+ * container is being removed.
>+ */

double newline before method, remove useless method name repetition

>+/**
>+ * nsNavHistoryContainerResultNode::OpenContainer
>+ *
>+ * This handles the generic container case.  Other container types should
>+ * override this to do their own handling.
>+ */

ditto

>+  NS_ENSURE_STATE(result);
>+
>+  NOTIFY_RESULT_OBSERVERS(result, ContainerOpened(TO_CONTAINER(this)));

what about including "NS_ENSURE_STATE(result);" in the notification macro? looks like worth it since it's a pattern repeated in various points.

>+/**
>+ * nsNavHistoryContainerResultNode::CloseContainer
>+ *
>+ * Unset aSuppressNotifications to notify observers on this change.  That is
>+ * the normal operation.  This is set to false for the recursive calls since the
>+ * root container that is being closed will handle recomputation of the visible
>+ * elements for its entire subtree.
>+ */

remove method name repetition


>+  if (!aSuppressNotifications) {
>+    NS_ENSURE_STATE(result);
>+    NOTIFY_RESULT_OBSERVERS(result, ContainerClosed(TO_CONTAINER(this)));
>   }

ditto for the inclusion of the check in the macro

>+/**
>+ * nsNavHistoryContainerResultNode::FillStats
>+ *

ditto for the name of the method, there are others below, won't comment further on this. i think they should be removed unless you think they improve readability

>+/**
>+ * nsNavHistoryContainerResultNode::SortComparison_Title*
>+ 
>+ * These are a little more complicated because they do a localization

there is an empty line without * in this jdoc

> nsresult
> nsNavHistoryContainerResultNode::ReplaceChildURIAt(PRUint32 aIndex,

>+  // Notify the result's observers.
>+  nsNavHistoryResult* result = GetResult();
>+  NS_ENSURE_STATE(result);
>+  if (AreChildrenVisible()) {
>+    NOTIFY_RESULT_OBSERVERS(result,
>+                            NodeReplaced(this, oldItem, aNode, aIndex));
>+  }

thanks to the macro name the comment "Notify the result's observers" is not really useful, if you integrate the NS_ENSURE_STATE in the macro it cal also go away.

>+  NS_ENSURE_TRUE(!mURI.IsEmpty(), NS_ERROR_FAILURE);

NS_ENSURE_STATE?

> NS_IMETHODIMP
> nsNavHistoryQueryResultNode::OnVisit(nsIURI* aURI, PRInt64 aVisitId,
>                                      PRTime aTime, PRInt64 aSessionId,
>                                      PRInt64 aReferringId,
>                                      PRUint32 aTransitionType,
>                                      PRUint32* aAdded)
> {
>-  // ignore everything during batches
>+  // Ignore everything during batches

missing period

>-  // register with the result for updates
>-  nsNavHistoryResult* result = GetResult();
>-  NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
>+  // Register with the result for updates
>+  nsNavHistoryResult* result = GetResult();
>+  NS_ENSURE_STATE(result);

missing period

>+  if (!mExpanded) {
>+    // When we are not expanded, we don't update, just invalidate and unhook
>     return NS_OK; // no updates in tree state

missing period on first comment.
second comment is a bit cryptic (or redundant).

>-// nsNavHistoryResult **********************************************************
>+// nsNavHistoryResult

as said above, i'd remove these

>+  if (!mRootNode)
>+    return NS_ERROR_FAILURE;

NS_ENSURE_STATE?

>diff --git a/toolkit/components/places/src/nsNavHistoryResult.h b/toolkit/components/places/src/nsNavHistoryResult.h

>-  // sorting
>+  // Sorting methods

period

i have not found any particular code problem, just some needed cleanup, so r=me

You still need:
- SR from mconnor or any other superreviewer (i think we already talked with mconnor about this change some week ago)
- tryserver run

Comment 10

[Asaf Romano (gone)]

Comment on attachment 429779 [details] [diff] [review]
Code Style Update

Thanks for the review Marco.  Mike says he cannot review this anytime soon.
 
Rob, can you look at the API change?

Comment 11

[Asaf Romano (gone)]

Attachment: address comments

Comment 12

[Asaf Romano (gone)]

Robert, the part of this patch that needs sr is the changes to nsINavHistoryService.idl.  Basically, we're replacing a single-observer api with the general add/removeObserver mechanism.  I'm adding the relevant parts here:

Comment 13

[Asaf Romano (gone)]

Ignore that last sentence.  The "relevant part(s)" /is/ the changes to nsINavHistoryService.idl

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 429850 [details] [diff] [review]
address comments

r=me for the api changes

Comment 15

[Asaf Romano (gone)]

And the try server builds are green.

Comment 16

[Asaf Romano (gone)]

http://hg.mozilla.org/mozilla-central/rev/a9aebf429a6b

Comment 17

[Marco Bonardo [:mak]]

additional changeset for typo
http://hg.mozilla.org/mozilla-central/rev/34b4f900a45f

Comment 18

[Robert Kaiser]

If I see that correctly, http://hg.mozilla.org/mozilla-central/rev/656ecf1ba58a backed out this bug so it should be reopened, right?

Comment 19

[Marco Bonardo [:mak]]

yep, backout: http://hg.mozilla.org/mozilla-central/rev/656ecf1ba58a

Comment 20

[Asaf Romano (gone)]

Attachment: Fix oranges

Comment 21

[Asaf Romano (gone)]

Relanded.

http://hg.mozilla.org/mozilla-central/rev/79f299fc10d6

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I backed this out to try to fix places-related test failures, e.g.
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1267770417.1267772116.8361.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1267770417.1267771118.5568.gz

Comment 23

[Asaf Romano (gone)]

Thanks Robert....

Comment 24

[Marco Bonardo [:mak]]

So far i've found an error in utils.js, where result is not defined in getURLsForContainerNode.
There could be more so i'll move with the tryserver.  Since i'm used to these nice oranges, i'll take care of driving out the final bits of this bug, so Asaf can move to the next enqueued stuff.

Comment 25

[Marco Bonardo [:mak]]

Attachment: orange fix patch

tryserver reported all relevant tests passing.  A leak was reported but the same leak is on the push before mine and the one after mine, so it is unrelated afaict.  Also a known random orange was reported.
Apart the above reported error in utils.js, one test had a var scope problem.

Comment 26

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/2df45af84fd4

Comment 27

[[e-mail]]

Comment on attachment 431346 [details] [diff] [review]
orange fix patch

>+    let suppressNotificationsOld = result.suppressNotifications;
Any chance we can rename this to didSuppressNotifications?

Comment 28

[Marco Bonardo [:mak]]

(In reply to comment #27)
> (From update of attachment 431346 [details] [diff] [review])
> >+    let suppressNotificationsOld = result.suppressNotifications;
> Any chance we can rename this to didSuppressNotifications?

feel free to file a bug and patch it, i'm available for review.

Comment 29

[Marco Bonardo [:mak]]

sigh, the leaks on tryserver were half real, this leaks, but since something in central was leaking already, they got hidden :(

backout: http://hg.mozilla.org/mozilla-central/rev/432649ee585e

since needs a new iteration, will also include Neil's comment 27.

Comment 30

[Asaf Romano (gone)]

Do we know what object leaked?

Comment 31

[Marco Bonardo [:mak]]

not that easy, everything was leaked out, i suppose some cycle was intact.
also, i see a bunch of assertions while b-c tests run due to NS_NOTREACHED("Invalid index for item adding: greater than count"); and i see some exceptions NS_NO_INTERFACE returned by tree.xml::get_view

Comment 32

[Marco Bonardo [:mak]]

Attachment: orange fix v1.1

this adds some bits (error checking, observers removal at result change), and based on Mano's suggestions i added back some cycle collector bits too.
I was able to reproduce the first leak running Places b-c tests, and another set of leaks running browser/base b-c tests.
This fixes both locally, i will immediately push to tryserver to get an idea if it is working as intended.

Comment 33

[Marco Bonardo [:mak]]

Comment on attachment 431752 [details] [diff] [review]
orange fix v1.1

Yep, cycle collector changes did the trick.

Mano, could you please review the interdiff between this patch and "fix oranges" patch?

Comment 34

[Asaf Romano (gone)]

Comment on attachment 431752 [details] [diff] [review]
orange fix v1.1

r=mano

Comment 35

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/95804c2b56e4
if this fails i'll personally go to kick the tryserver.

Comment 36

[Marco Bonardo [:mak]]

we should update docs to remove the old viewer implementation and insert the new multiple observers implementation. iirc we have some example code of how to handle places trees on mdc.

Comment 37

[Eric Shepherd [:sheppy]]

Created:

https://developer.mozilla.org/en/nsINavHistoryResultObserver

Mostly based on the old nsINavHistoryResultViewer, but updated.

Updated:

https://developer.mozilla.org/en/nsINavHistoryResult
https://developer.mozilla.org/en/nsINavHistoryContainerResultNode
https://developer.mozilla.org/en/nsINavHistoryQueryOptions

Comment 38

[Eric Shepherd [:sheppy]]

Actually, this needs to be updated too:

https://developer.mozilla.org/en/Displaying_Places_information_using_views#Creating_custom_views

Does anyone feel up to tweaking that? I didn't write that originally, so it would be very helpful if someone else could fix it.

Comment 39

[Andreas Wagner [:TheOne] [use NI]]

I tried my best to update 

https://developer.mozilla.org/en/Displaying_Places_information_using_views

but someone with more insight should definitely read that page and check the examples.
Please remove my note at the end of https://developer.mozilla.org/en/Displaying_Places_information_using_views#Creating_custom_views after checking that I didn't write nonsense, or correcting my nonsense :).

Please also remove the tag needsTechnicalReview afterwards.

Thank you mak :P