536893 - Asynchronous API for opening nsNavHistoryFolderResultNodes

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Drew Willcoxon :adw]

Attachment: patch 1

I'd like to forget about async batching for now and just get async done, especially since off-main-thread I/O is a Q1 goal.  Batching is the reason for bug 530236 and SQL sorting, which has been harder than I thought.  If we forget about it, async is easier.  (This patch is not so big.)  It's the next logical step, and we can come back to it later using what we've built for async.

Marco, let me know if Mano would be better to review this.  It gives folder nodes async powers.  I started there since it's easier than query nodes, but query nodes will be the next bug.  It relies on the Storage patch in bug 536798.

Comment 1

[Marco Bonardo [:mak]]

Attachment: first-pass review

Comment 2

[Drew Willcoxon :adw]

Thanks Marco.  I just skimmed over it so far, but first, what does "ucfirst" mean?  And when did we decide on this coding style?  It's not a big deal (but I do disagree a little FWIW), you just mention it a lot.  Seems out of the blue, and at least a couple of points are contrary to the way we've been doing things.

Comment 3

[Marco Bonardo [:mak]]

ucfirst just uppercase first, i mean, comments should be readable phrases.
We decided to follow platform conventions for pointers and other decorations, that is to have it near the type, we were doing the opposite before (storage style), but we thought is better to follow the largest part of code.
See https://wiki.mozilla.org/Places/Coding_Style i think i sent a mail about this some days ago

Comment 4

[Drew Willcoxon :adw]

Attachment: patch 2

The Storage patch/bug that I relied on turned out to be invalid, so I added nsNavHistoryContainerResultNode::mAsyncCanceledState.

>after "setting containerOpen" i think a comma is enough

I'm not sure what you mean, so I cut everything after "setting containerOpen".

>>+  void containerOpening(in nsINavHistoryContainerResultNode aContainerNode);
>
>is this always called even if asyncMode == false?

No, comment updated.

>please check History() return value, let's try to avoid possible crashes hooks

Crash hooks, hehe.  Sounds like a feature!

>is it possible that a sync and an async stmt using mDBGetChildren
>runs at the same time?

Yes.

>if so, could happen that one of the two resets the statement while
>the other one is running? These are problems that could probably
>happen during the transition sync->async if the same stmt is used in
>both flavors...

I asked Shawn about this, it's safe.  There's actually one sqlite3_stmt used in the sync case and another for async.  Of course these two separate sqlite3_stmts will race each other if they're running at the same time, but that's not a problem here.

>maybe we want just #include "mozilla/storage.h", but looks like you
>can just include it in the .h

Cool, I didn't know about that.  I added it to the header and removed the existing mozStorageHelper.h include in the cpp.

>so, since you removed the assertion, looks like could happen that you
>call closeContainer before mExpanded is set, this could happen in the
>async case, right?  But in such a case, what's the point of notifying
>the view? if the node is not expanded the view should not have been
>changed and should probably not need to be notified. Unless when we
>open the container async we notify the view and decorate the
>container as open before we have all the results.
>
>ok, looks like we notify ContainerOpening, is this intended to handle
>something like a throbber on the folder icon?

Yes.  Maybe I should leave in the assertion but change it to mAsyncEnabled || !mExpanded?  Or factor out the portion that is useful for me into a separate function.

(With async the meaning of expanded is murky.  We could set mExpanded to true as soon as the container is told to open, or we could wait until all children load.  I think it's less invasive in our code to wait until all children load.  So now we have an "opening" state (nsINavHistoryResultViewer.containerOpening) during which time the container is not expanded but can be closed.)

>also check && mResult, cycle collector could have disconnected it.

Updated, but there are other places I use mResult without checking.  (And other places I didn't write.)  Should I check everywhere?  When can mResult become null?

>please warn out the error message, we should have some code doing
>that around...  i think Shawn added something like that to Helpers.h,
>a class that extends pendingStatement providing a common handleError

Cool, updated.  (toolkit/components/places/src/Helpers.h)

>comments inside if/elseif/else statements, not outside

OK, but... I disagree.  Comments describe what comes after, not before.  Comments after conditions are harder to read.  I have to parse the condition before I get to the comment, when not parsing code is the whole point of comments in the first place.  If I write a comment directly above a conditional, it's obvious what it applies to and when.

>would it be crazy to use openContainer and just check mAsyncMode inside it?

It's good to separate out the two cases into smaller methods rather than having a big one.  Except for notifying the view, they're different.  I want to make the separate async and sync paths explicit if possible.

>>+  void CancelAsync(PRBool aRestart);
>
>this method's name sounds too generic, CancelPendingStatement maybe?
>or CancelAsyncOpen?

Yeah, I like CancelAsyncOpen, since there's OpenContainerAsync.

>>+  PRBool mAsyncMode;
>
>this is a bool so the name should be something like mUseAsyncMode
>otherwise looks like you can have different async modes

I assume this comment applies to the idl also.  I went with {mA,a}syncEnabled.

>what about doing this in doNextTest like test.containerOpening =
>false... so that you have an initialized test env?

I made some changes, let's see what you think.  I used to like factoring out every common bit of code in tests, but I've come to the conclusion that it often makes them harder to follow and debug.

Comment 5

[Drew Willcoxon :adw]

Attachment: patch 2.1

Applies cleanly.

Comment 6

[Drew Willcoxon :adw]

Attachment: patch 2.2

A few small changes:

Retain assertion in CloseContainer, extend it to the async case.  It currently ensures that CloseContainer isn't called twice, and now it correctly takes the async case into account.

Pass folder ID to QueryFolderChildrenAsync rather than getting it from the folder node.  Maybe someday we'll have proper information hiding, use of protected/private data.

Removes some work in HandleCompletion that was copied from ClearChildren, calls ClearChildren instead.

NS_ENSURE_ARG_POINTER checks.

Larger changes to the test to make it even more structured.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 419704 [details] [diff] [review]
patch 2.2

i don't see anything blocking this from going, but would be nice to get a second opinion (either mano or dietrich), i prefer this to start in the right direction.
did you get a tryserver run?
also, you still need an SR for idl changes

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

>+nsresult
>+nsNavBookmarks::ProcessFolderNodeRow(
>+  mozIStorageValueArray* aRow,
>+  nsNavHistoryQueryOptions* aOptions,
>+  nsCOMArray<nsNavHistoryResultNode>* aChildren,
>+  PRInt32& aCurrentIndex)
>+{
>+  nsRefPtr<nsNavHistoryResultNode> node;
>+  if (itemType == TYPE_BOOKMARK) {
>+    nsNavHistory* history = nsNavHistory::GetHistoryService();
>+    NS_ENSURE_TRUE(history, NS_ERROR_UNEXPECTED);

hm, i guess how much this could slowdown reading a large folder
we could pass it in for QFC, but in the async case, we would be called by handleResult, and that won't help :(

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

>+/**
>+ * attribute boolean asyncEnabled;
>+ */

are these comments really useful?

>+    nsresult rv;
>+    if (IsDynamicContainer()) {
>+      // Notify dynamic containers that we are closing.
>+      nsCOMPtr<nsIDynamicContainer> svc =
>+        do_GetService(mDynamicContainerType.get(), &rv);
>+      if (NS_SUCCEEDED(rv))

just check svc, so you can avoid rv

>-  if (unregister && mContentsValid) {
>-    if (mResult) {
>-      mResult->RemoveBookmarkFolderObserver(this, mItemId);
>-      mIsRegisteredFolderObserver = PR_FALSE;
>-    }
>+  if (unregister &&
>+      (mContentsValid || mAsyncPendingStmt) &&
>+      mResult &&
>+      mIsRegisteredFolderObserver) {

cache
bool needsUnregistering = unregister && (mContentsValid || mAsyncPendingStmt)
so the if is more readable

>
>+  RESTART_AND_RETURN_IF_ASYNC_PENDING;

i'd prefer these to appear like no-params functions, i think like they are they break a bit readability.
RESTART_AND_RETURN_IF_ASYNC_PENDING();
You can ignore me though if you don't agree

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

>+
>+protected:
>+
>+  enum AsyncCanceledState { NOT_CANCELED, CANCELED, CANCELED_RESTART_NEEDED };

new lines should be better then additional spaces around braces


> class nsNavHistoryFolderResultNode : public nsNavHistoryContainerResultNode,
>-                                     public nsINavHistoryQueryResultNode
>+                                     public nsINavHistoryQueryResultNode,
>+                                     public mozilla::places::AsyncStatementCallback

i guess we can "using mozilla::places;" at file start

>diff --git a/toolkit/components/places/tests/queries/test_async.js b/toolkit/components/places/tests/queries/test_async.js

>+Test.prototype = {
>+  /**
>+   * Call this when a viewer callback is entered.  It registers the callback's
>+   * invocation and ensures that it has been invoked the given number of times.
>+   * See checkCallback for parameter explanations.
>+   *
>+   * @return The number of times the callback with the given name has been
>+   *         invoked.
>+   */
>+  checkCallbackEntered: function (aCallbackName, aExpectedMin, aExpectedMax) {
>+    print(aCallbackName + " entered");
>+    this.callbackCounts[aCallbackName] =
>+      this.callbackCounts[aCallbackName] || 0;

i guess this.callbackCounts[aCallbackName] could fire a strict warning

>+  setup: function () {
>+    // Set up a simple query.
>+    bmsvc.insertBookmark(bmsvc.unfiledBookmarksFolder, uri(BOOKMARK_URL),
>+                         bmsvc.DEFAULT_INDEX, "");

can we add a folder and a separator as well? just in case.

Comment 8

[Drew Willcoxon :adw]

Attachment: patch 2.3

Mano, requesting a second review as suggested by Marco.  Please let me know if you won't be able to get to it for awhile and I'll ask Dietrich instead.

(In reply to comment #7)
> did you get a tryserver run?

Pushed now.

> >+/**
> >+ * attribute boolean asyncEnabled;
> >+ */
> 
> are these comments really useful?

Removed.

> >+  RESTART_AND_RETURN_IF_ASYNC_PENDING;
> 
> i'd prefer these to appear like no-params functions, i think like they are they
> break a bit readability.
> RESTART_AND_RETURN_IF_ASYNC_PENDING();

Sounds fine, changed.

> > class nsNavHistoryFolderResultNode : public nsNavHistoryContainerResultNode,
> >-                                     public nsINavHistoryQueryResultNode
> >+                                     public nsINavHistoryQueryResultNode,
> >+                                     public mozilla::places::AsyncStatementCallback
> 
> i guess we can "using mozilla::places;" at file start

I'm not a big fan of using... so I left it as is.  Let me know if you feel strongly about it.

> >+  setup: function () {
> >+    // Set up a simple query.
> >+    bmsvc.insertBookmark(bmsvc.unfiledBookmarksFolder, uri(BOOKMARK_URL),
> >+                         bmsvc.DEFAULT_INDEX, "");
> 
> can we add a folder and a separator as well? just in case.

I changed the test to better use head_queries.js also.

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 420213 [details] [diff] [review]
patch 2.3

Arg, I goofed when requesting review...

> Mano, requesting a second review as suggested by Marco.  Please let me know if
> you won't be able to get to it for awhile and I'll ask Dietrich instead.

Comment 10

[Asaf Romano (gone)]

Expect review (or at least a first-pass) during the weekend.

Comment 11

[Marco Bonardo [:mak]]

(In reply to comment #8)
> > i guess we can "using mozilla::places;" at file start
> 
> I'm not a big fan of using... so I left it as is.  Let me know if you feel
> strongly about it.

while i don't have feelings for cpp constructs, i think soon or later we will be adding that, so no reasons to delay that, imo.

Comment 12

[Drew Willcoxon :adw]

Thanks Mano.

(In reply to comment #11)
> while i don't have feelings for cpp constructs, i think soon or later we will
> be adding that, so no reasons to delay that, imo.

The C++ FAQ explains it better than I could.  Does this argument give you feelings? :)
http://www.parashift.com/c++-faq-lite/coding-standards.html#faq-27.5

Comment 13

[Marco Bonardo [:mak]]

(In reply to comment #12)
> The C++ FAQ explains it better than I could.  Does this argument give you
> feelings? :)
> http://www.parashift.com/c++-faq-lite/coding-standards.html#faq-27.5

hm, no, std is a pretty big namespace while our own contains a really small amount of entried, btw, i won't care.

Comment 14

[Shawn Wilsher :sdwilsh]

Note: no using directives in a .h file per our own coding standards.

Comment 15

[Asaf Romano (gone)]

(Tomorrow! sorry).

Comment 16

[Asaf Romano (gone)]

Comment on attachment 420213 [details] [diff] [review]
patch 2.3

Sorry for the massive delay.  Code looks mostly OK, but the API has some issues.

>   /**
>+   * Set to true to turn on asynchronous mode.  While async mode is activated,
>+   * setting containerOpen to true causes the container to be opened
>+   * asynchronously.  To be notified when the container has opened, register a
>+   * viewer before setting containerOpen.
>+   *
>+   * @note Currently this is supported only for bookmark folder containers.
>+   */
>+  attribute boolean asyncEnabled;
>+

IMO, this should go to the query interface, and we can have a readonly attribute in the result interface. Otherwise, you'll have to deal with: 1) sync->async->sync hierarchies 2) asyncEnabled changes after the container was opened synchronously and vice-versa. You might be able to handle both, but I don't think that's necessary at all.
 

>-   * Called after a container node went from closed to opened.
>+   * Called after a container node went from closed to opened.  If the container
>+   * has asyncEnabled set, this is called after containerOpening is called, when
>+   * all children have loaded.
>    *
>    * @param aContainerNode
>    *        the container node which was opened
>    */
>   void containerOpened(in nsINavHistoryContainerResultNode aContainerNode);
> 
>   /**
>+   * If a container has asyncEnabled set and its children must be reloaded from
>+   * the database, this is called just after the container is opened and before
>+   * containerOpened is called.  If the container's children are up to date or
>+   * if the container does not have asyncEnabled set, this method is not called.
>+   */
>+  void containerOpening(in nsINavHistoryContainerResultNode aContainerNode);
>+

I prefer:

1. nodesInserted, replacing nodeInserted.
2. When opening a container asynchronously, let the caller pass a callback object to be notified when the container finished loading
3. a state attribute in the container interface. The possible states three possible states: closed, loading, opened.

This has a couple of advantages:
1. Views won't need to care as much about async mode
2. The new API will be useful for view-less use-cases.

>+nsresult
>+nsNavBookmarks::ProcessFolderNodeRow(
>+  mozIStorageValueArray* aRow,
>+  nsNavHistoryQueryOptions* aOptions,
>+  nsCOMArray<nsNavHistoryResultNode>* aChildren,
>+  PRInt32& aCurrentIndex)
>+{

>+  }
>+  else if (itemType == TYPE_FOLDER || itemType == TYPE_DYNAMIC_CONTAINER) {
>+    if (aOptions->ExcludeReadOnlyFolders()) {
>+      // If the folder is read-only, skip it.
>+      PRBool readOnly = PR_FALSE;
>+      GetFolderReadonly(id, &readOnly);

Can/Should GetFolderReadonly handle dynamic containers?

Comment 17

[Drew Willcoxon :adw]

> >+  attribute boolean asyncEnabled;
> >+
> 
> IMO, this should go to the query interface, and we can have a readonly
> attribute in the result interface. Otherwise, you'll have to deal with: 1)
> sync->async->sync hierarchies 2) asyncEnabled changes after the container was
> opened synchronously and vice-versa. You might be able to handle both, but I
> don't think that's necessary at all.

I hear what you're saying.  My concern is that async mode affects opening containers.  Its only connection to queries is by way of results by way of containers.  That's two levels removed.  Putting the toggle in a different iface obscures its purpose and is not a good design IMO.  Besides, both points 1 and 2 ought to be available to consumers should they want to do them, neither are dangers, and no extra code in the impl is necessary to support them.

> I prefer:
> 
> 1. nodesInserted, replacing nodeInserted.
> 2. When opening a container asynchronously, let the caller pass a callback
> object to be notified when the container finished loading
> 3. a state attribute in the container interface. The possible states three
> possible states: closed, loading, opened.
>
> This has a couple of advantages:
> 1. Views won't need to care as much about async mode
> 2. The new API will be useful for view-less use-cases.

This was my initial approach many months ago.  My prototype patch in bug 490714 is written this way.  Let me explain why I chose another path to see if you agree.

Say we pass a callback object to be notified when the container has finished loading.  So we need a new iface for this callback.  Or we can use something like nsIObserver, but that's too generic.  The new iface has a single method, let's call it containerLoaded.  But wait -- now in my impl, in HandleCompletion, I'm calling both containerLoaded on the new iface and containerOpened on the viewer.  Why is that?  containerLoaded and containerOpened have the same semantics.  They're called when all the children are available.  The new iface is a subset of nsINavHistoryResultViewer.  Why not just use nsINavHistoryResultViewer?  Indeed, the new iface is just a view.  We already have a view.

Great, if containerOpened's semantics are preserved under async execution, then none of our existing view implementations need to be modified to support async, more or less.  They just work, to your first point about advantages.

However, regardless of the design we need to provide visual feedback in the UI that a container is loading.  And UI considerations aside, consumers might like to know when a container starts loading.  A new viewer method is a natural, simple solution.  The view sees containerOpening and renders a throbber or sets whatever internal state it needs.  Later it sees containerOpened and renders the children just as it always did.

As for viewless usecases -- in this context, any callback that's notified of changes in state is a view.  Whether it's hooked up to UI is an orthogonal issue.  The core of your second point about advantages is really, how do you hook up the view to the container?  We've already decided that:  You attach it to its result.  We could have decided that containerOpen is a method, not a boolean, and that the viewer is passed to it.  But that's not the case, and how the view is hooked up to the container/result is independent of sync vs. async anyway.

Look at the API if we added an async open method.  We'd have containerOpen, a boolean, in addition to a new openAsync(callback).  How do the two interact?  Do we then need closeAsync()?  That doesn't make sense.  Just close()?  But we already set containerOpen = false to close.  So do you openAsync(callback) and then containerOpen = false?  That's inconsistent.  How is the callback/view passed to openAsync() related to the view attached to the result?  A simple async toggle while reusing containerOpen and the existing view mechanism is the better choice.

> >+nsresult
> >+nsNavBookmarks::ProcessFolderNodeRow(
> >+  mozIStorageValueArray* aRow,
> >+  nsNavHistoryQueryOptions* aOptions,
> >+  nsCOMArray<nsNavHistoryResultNode>* aChildren,
> >+  PRInt32& aCurrentIndex)
> >+{
> 
> >+  }
> >+  else if (itemType == TYPE_FOLDER || itemType == TYPE_DYNAMIC_CONTAINER) {
> >+    if (aOptions->ExcludeReadOnlyFolders()) {
> >+      // If the folder is read-only, skip it.
> >+      PRBool readOnly = PR_FALSE;
> >+      GetFolderReadonly(id, &readOnly);
> 
> Can/Should GetFolderReadonly handle dynamic containers?

Marco, do you know?  That snippet is from QueryFolderChildren, which I carried over into ProcessFolderNodeRow.

Comment 18

[Marco Bonardo [:mak]]

(In reply to comment #17)
> > Can/Should GetFolderReadonly handle dynamic containers?
> 
> Marco, do you know?  That snippet is from QueryFolderChildren, which I carried
> over into ProcessFolderNodeRow.

ha, no getFolderReadOnly does not handle dynamic containers, but they should be always considered readOnly. Please fix it, even if actually i don't really bother much, dynamic containers are broken still, and i still don't know which direction they'll take.

Comment 19

[Asaf Romano (gone)]

Still not convinced :) Let me explain:

1. No, what's actually async is the query which generated the container. It would be the container's own query, if it had one, or the closest ancestor's query.  Since the new API only supports folder-containers, that's not an issue (because each folder container has its own query).  Whenever this API is implemented for complex queries, container.asyncEnabled will be meaningless for most containers.  For example, on "group by tag" queries:
 * Not setting it on the "top" container means that opening any tag container wouldn't be async.
 * Setting it on the "container" but unsetting it on the children won't actually work.

2. However, I do agree now that the query interface isn't the right place.  The new query options should go to the options(!) interface. Note that this will work nicely because we already have a well done implementation for options inheritance in place.

3. All that said, a new container.state attribute will be needed either way.

4. The view vs. callback issue is an expected mess.... (see bug 497608).  Add-ons used to rely on the bookmarks and history observers for watching results, because we had our own view set. That won't be possible anymore (in the async case, that is).   However, you're right that the callback interface I suggested just duplicates the view interface.  We need a new solution for these add-ons before breaking the current API. I think you could simply replace the current single "view" mechanism with a multiple-views one.

Comment 20

[Marco Bonardo [:mak]]

So, about the interface where asyncMode should go, i don't see clear advantages both sides, from what i got talking from Mano the query option would read something as OpenContainersAsyncWhenPossible. But that also means the "open async" operation is directly related to a container that is an entity not directly related to the query (we have flat queries with no containers in them), for those queries it would be a meaningless option, but we also have excludeReadOnlyFolders that is meaningless as well.
Both positions have same value for me, but i see that having to read the parent container async mode before opening a child container is worst than just knowing root was async, and if i'll open a container it could be async. Query Options is 0.1 times more preferrable for me than container property.

About the rest, i think we should move on with this implementation on the viewer side but at the same time provide a multi-viewer implementation for results, so observing a result that already has a view won't force extenders to do crazy things. So the best solution is that while Drew implements this first layer of async on viewer interface, Mano works on multi-viewer thing, that afaict is needed to allow extensions to observe view changes in async mode. This is something that should still be done regardless direction of this bug.

I also like the idea of the .state readonly attribute.
But then actually we could also have an onContainerStateChanged going to replace containerOpening, containerOpen, containerClosed, containerClosing and all of this fancy stuff... it's crazy having so many methods to just check 1 state. and since we are going to change the API... would that be crazy?
It would just be for old implementations, we could add it, declare other calls deprecated, and remove later.

Comment 21

[Drew Willcoxon :adw]

(In reply to comment #19)
> 1. No, what's actually async is the query which generated the container. It
> would be the container's own query, if it had one, or the closest ancestor's
> query.  Since the new API only supports folder-containers, that's not an issue
> (because each folder container has its own query).  Whenever this API is
> implemented for complex queries, container.asyncEnabled will be meaningless for
> most containers.  For example, on "group by tag" queries:
>  * Not setting it on the "top" container means that opening any tag container
> wouldn't be async.
>  * Setting it on the "container" but unsetting it on the children won't
> actually work.

I think I understand, let me make sure.  Are you saying that for some types of containers, such as group by tag, by the time the container is opened it already contains its children?  In other words, opening the container and filling the children are independent acts?  (Not doubting you, but in the interest of learning where in the code does this happen?  I looked but didn't find.)

> 3. All that said, a new container.state attribute will be needed either way.

I see how container.state is a (read-only) convenience, but I don't see how it's necessary.  If the view needs to know whether the container is closed, opening, or opened, it can listen for each event and set internal state accordingly.

> 4. The view vs. callback issue is an expected mess.... (see bug 497608). 
> ...
> suggested just duplicates the view interface.  We need a new solution for these
> add-ons before breaking the current API. I think you could simply replace the
> current single "view" mechanism with a multiple-views one.

As we discussed on IRC, you'll be working on this and it doesn't block this bug, correct?

(In reply to comment #20)
> I also like the idea of the .state readonly attribute.
> But then actually we could also have an onContainerStateChanged going to
> replace containerOpening, containerOpen, containerClosed, containerClosing and
> all of this fancy stuff... it's crazy having so many methods to just check 1
> state. and since we are going to change the API... would that be crazy?

Not crazy, good to discuss.  (BTW, there are only three methods, no containerClosing. :\)  That would mean we eventually have to convert all of our view impls in tests, etc. to the new method.  As I noted in comment 17, blending in to the current view iface was a goal of this design.  Should it not be?  And, if there is a single method the first thing every impl will do is switch on the state.  Breaking out the switch into separate methods is better modularity IMO.

Comment 22

[Marco Bonardo [:mak]]

(In reply to comment #21)
> (In reply to comment #19)
> Are you saying that for some types of
> containers, such as group by tag, by the time the container is opened it
> already contains its children?

i'll answer for him, yes.

> I see how container.state is a (read-only) convenience, but I don't see how
> it's necessary.  If the view needs to know whether the container is closed,
> opening, or opened, it can listen for each event and set internal state
> accordingly.

that's the point, we can track it for them at really low cost.

> > 4. The view vs. callback issue is an expected mess.... (see bug 497608). 
>
> As we discussed on IRC, you'll be working on this and it doesn't block this
> bug, correct?

that's what i got as well

> (In reply to comment #20)
> > I also like the idea of the .state readonly attribute.
> > But then actually we could also have an onContainerStateChanged going to
> > replace containerOpening, containerOpen, containerClosed, containerClosing and
> > all of this fancy stuff... it's crazy having so many methods to just check 1
> > state. and since we are going to change the API... would that be crazy?
> 
> Not crazy, good to discuss.  (BTW, there are only three methods, no
> containerClosing. :\)  That would mean we eventually have to convert all of our
> view impls in tests, etc. to the new method. As I noted in comment 17,
> blending in to the current view iface was a goal of this design.  Should it not
> be?  And, if there is a single method the first thing every impl will do is
> switch on the state.  Breaking out the switch into separate methods is better
> modularity IMO.

I'm worried about interface explosion, we are adding onContainerOpening now, we could add other states in future (onContainerClosing was just an example), and adding a method for each state looks redundant when state is clearly a single property that can just have one value at a time.
Personally, i'd not add onContainerOpening, instead i'd add .state and .onContainerStateChanged, i'd add a deprecate comment to onContainerOpen and onContainerClosed saying they'll go away in future versions.
Sure, we should call both for some time, but that leaves space for eventual future additions without having to rev the API version.

Comment 23

[Asaf Romano (gone)]

About container.state: a view (that is, an observer) could be attached at any point - when any container in the tree could be either closed or opened or opening.  Thus we *should* expose data, not just because we can.

Comment 24

[Asaf Romano (gone)]

Also, I agree with Marco about onContainerStateChanged.

Comment 25

[Drew Willcoxon :adw]

Comments 23 and 24 make sense, OK.

Asking this again...

(In reply to comment #21)
> In other words, opening the container and
> filling the children are independent acts?  (Not doubting you, but in the
> interest of learning where in the code does this happen?  I looked but didn't
> find.)

Where does this happen?  FillChildren is called only from OpenContainer, Refresh, and GetHasChildren.  Drawing twisties calls GetHasChildren and ruins async, so I'd like to change that, but I don't think that's what you guys are referring to.

Comment 26

[Asaf Romano (gone)]

We're referring to non-folder containers, which don't use FillChildren.

Comment 27

[Drew Willcoxon :adw]

Sorry to be obtuse, but could you point out the code path that doesn't use FillChildren?  I see nsNavHistoryQueryResultNode::FillChildren, which is certainly for non-folder containers.

Comment 28

[Asaf Romano (gone)]

Ask Marco, he implemented the tags queries :)

Comment 29

[Marco Bonardo [:mak]]

tags query just returns queries as if they would be nodes, i mean it's just an SQL trick. they are queries, so i expect them to pass through FillChildren, as well. Otherwise, if i said unrelated things, then i didn't understand the question.

Comment 30

[Drew Willcoxon :adw]

OK, here's my question from comment 21 again:

> Are you saying that for some types of
> containers, such as group by tag, by the time the container is opened it
> already contains its children?  In other words, opening the container and
> filling the children are independent acts?

If yes, where does it happen?  Please point to a code path.  I have looked but didn't find.

But it's now sounding like the answer is no.  The bottom line is, if OpenContainer is the only path that fills children (aside from GetHasChildren, which is a side problem I will fix at some point), then again I claim that this patch's design is OK.

Comment 31

[Marco Bonardo [:mak]]

(In reply to comment #30)
> But it's now sounding like the answer is no.  The bottom line is, if
> OpenContainer is the only path that fills children (aside from GetHasChildren,
> which is a side problem I will fix at some point), then again I claim that this
> patch's design is OK.

queries containing queries are just sql queries returning data generated on the fly, and they look like any other query code-side, just that other queries return only flat uris, these ones return place: uris. Tags are not even folderNodes.
By design i assume you talking about "asyncMode" position, remaining points look clear. Still i think that won't make a difference, but assuming i'm a user of our code, having to set asyncMode on tags container (for example) and then, once i'm inside, having to set it again on each tag container before opening them looks boring, as if i have a folder with folders inside, do i have to set it on each container? would be nicer to set it once in root query options and then make it inherit by children options. And i think Mano was talking about this kind of inheritence.

Comment 32

[Drew Willcoxon :adw]

(In reply to comment #31)
> By design i assume you talking about "asyncMode" position, remaining points
> look clear.

Yeah, sorry.  It's the "are queries async or are containers" question I was talking about.

> Still i think that won't make a difference, but assuming i'm a user
> of our code, having to set asyncMode on tags container (for example) and then,
> once i'm inside, having to set it again on each tag container before opening
> them looks boring, as if i have a folder with folders inside, do i have to set
> it on each container? would be nicer to set it once in root query options and
> then make it inherit by children options. And i think Mano was talking about
> this kind of inheritence.

OK, that argument makes sense to me.

If it were the case that opening and filling were independent, then I would agree that there's no choice but to make queries async and not containers.  From comment 21 up until now I thought that this was the case.

But since that's not true, it's a question of taste.  I think removing asyncEnabled from the thing it's acting on for the sake of slightly more convenience is bad design.  But it looks like I'm outnumbered. :)

Comment 33

[Asaf Romano (gone)]

FWIW, in the past we used to have GroupByFoo code that did group queries into "static" containers (and still the container interface is independent from the query interface, by the way).  It was removed as part of the history-tree rewrite, but I'm not sure at all if we're not going to bring it back at some point (say, can a dynamic container fill itself before opening it? I think it could).

Furthermore, note that queries containing queries sometimes refresh the entire structure for every little change, at which point you'll need to carry over asyncMode state somehow.

Another problematic case: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistoryResult.cpp#4544

Comment 34

[Marco Bonardo [:mak]]

(In reply to comment #33)
> Furthermore, note that queries containing queries sometimes refresh the entire
> structure for every little change, at which point you'll need to carry over
> asyncMode state somehow.

sometimes is probably always, since those queries are complex, to simplify the thing, if we think we need to refresh, we refresh the external query and not the internal queries (they are still rebuilt by the refresh of the external query).

Comment 35

[Asaf Romano (gone)]

Right.

Comment 36

[Drew Willcoxon :adw]

OK.  I don't know anything about dynamic containers, and I don't understand comment 34.  Why don't they fill themselves when they're opened?  Wouldn't it be consistent if for all containers the initial opening means filling?  Why should a container fill itself before opening if you must open it to see its children?

Related, what's the UI for dynamic containers?  How are they drawn when they're filling but not opening?  What happens when you click one that is filling but closed?  (I realize they're not exposed yet.)  Since they're inconsistent with other types of containers, their UI and interactions will be inconsistent.

Refresh is a separate problem for a follow-up bug.  When a container is refreshed, it's open, its children have previously loaded.  It's already gone through the initial open => filling path at that point.

Comment 37

[Drew Willcoxon :adw]

For "static" and dynamic containers, this algo should work, no?

If children are in other containers:
  When the container is opened:
    If those other containers are already open:
      Their children are in memory, great.  asyncEnabled is irrelevant.
    Else:
      Open those other containers according to asyncEnabled.
Else:
  If children are in memory:
    Add them before opening, sure.  asyncEnabled is irrelevant.
  Else:
    When the container is opened, execute the (custom) SQL synchronously or
    async, according to asyncEnabled.

Comment 38

[Asaf Romano (gone)]

The children of a dynamic container may be ready at any point (for example, a dynamic container of the opened tabs).

Comment 39

[Asaf Romano (gone)]

(Yet again, this is all fine if asyncEnabled goes to the options interface.  If we've agreed on that, just ignore my last few comments).

Comment 40

[Drew Willcoxon :adw]

asyncEnabled in the options iface is fine.

I think we're missing each others' points, at least I was.  The important part I left out is, *from the point of view of the container's consumer* opening triggers the initial load.  If the children are ready before the container is opened, asyncEnabled is irrelevant.  Otherwise, we're in the loading state until they're ready.  Either way, the consumer thinks it triggered the load.

But your point is don't lie to consumers, especially since refreshes occur.  OK, that makes sense, but then aren't there four states, not three?  Either two booleans containerOpen and isLoading, or the loading state is broken into loading_closed and loading_opened.

Comment 41

[Asaf Romano (gone)]

When would loading_closed happen? IMO "loading" means "expect nodes to come".  It sure cannot happen when the container is closed.

By the way, a better naming for the attribute would be "allowAsync" (or maybe "allowAsyncLoading").  That would reflect that sometimes, we might just show the contents synchronously, even if async has been allowed.  Then you don't rneed to mention that the API is implemented just for folders - we can implement it for various container types without modifying the API even in point releases.

Comment 42

[Drew Willcoxon :adw]

I am absolutely confused.  I don't understand what's wrong with the patch.

Comment 43

[Drew Willcoxon :adw]

Attachment: patch 3

Makes these changes:

* Moves async enabled to the options iface.
* Adds state (OPENED_STATE, LOADING_STATE, CLOSED_STATE) to the container iface.
* Adds containerStateChanged(container, newState, oldState) to the view.

Tests still pass.  When bug 543444 lands I'll update the patch accordingly.

Comment 44

[Asaf Romano (gone)]

* Yes, the places APIs usually pass the new values in notifications like this
* I didn't read the actual code. Is async state inherited now (from a folder-container to its subfolders-containers)?
* nit: afaict the convention is to name state constants in this form: STATE_* (as opposed to *_STATE).

Comment 45

[Marco Bonardo [:mak]]

(In reply to comment #44)
> * nit: afaict the convention is to name state constants in this form: STATE_*
> (as opposed to *_STATE).

agree, i think makes things more visually grouping and meaningful. for example around we have things like NS_SOMETHING_THAT_YOU_DONT_KNOW_WHAT_IT_IS_TOPIC_ID, then you have to read all the name to know it's a topic id, while NS_TOPIC_ID_SOMETHING_ETC. would give you the meaningful info immediately.

Comment 46

[Drew Willcoxon :adw]

(In reply to comment #44)
> * I didn't read the actual code. Is async state inherited now (from a
> folder-container to its subfolders-containers)?

Containers have an mOptions member that's set on construction.  It's difficult to tell, but it appears that query and folder containers assume it's non-null.  This patch simply does: on container open, if mOptions->AsyncEnabled, then open async, else sync.

There's nsNavHistoryResultNode::GetGeneratingOptions also.  TBH I can't follow the maze of when a node's options are valid or whether we should look up the parent chain or whether it depends on the type of node or what, so I appreciate any help here.

Comment 47

[Marco Bonardo [:mak]]

Drew, as discussed during the work week, please file a followup to replace opening, closing apis with the stateChanged api (that should be added here).
Dependancy is pushed and sticking from what i see, so this patch should be able to evolve now. Sorry if it took some time and for the bitrot, but code should be a bit better atm.

Comment 48

[Drew Willcoxon :adw]

Attachment: patch 4

Thanks guys.  All Places toolkit xpcshell tests pass locally, will push to tryserver.

(In reply to comment #47)
> Drew, as discussed during the work week, please file a followup to replace
> opening, closing apis with the stateChanged api (that should be added here).

Filed bug 552025.

Notes:

* We now have nsINavHistoryContainerResultNode.state and
  containerOpen.  Making state read-write is not optimal, since
  state = STATE_LOADING doesn't make sense.  At some point we
  should maybe make open() and close() methods.

* Added nsNavHistoryContainerResultNode::NotifyOnStateChange and
  funneled all observer notifications though it.  It notifies
  through both the new containerStateChanged and the old methods.

Questions:

* An xpconnect error is dumped to text console if a JS view
  doesn't define containerStateChanged.  Any way around that?
  When you use the bookmarks sidebar, e.g.

* What's the correct way to get the options for a container node?
  Both result and folder nodes seem to assume mOptions is
  non-null, but there's
  nsNavHistoryResultNode::GetGeneratingOptions also.  (See
  comment 46.)

* Similar question for mResult and GetResult.  In this case,
  GetResult actually raises an assertion if the node is a
  container and has a null mResult, so what's the point of ever
  calling GetResult in container impl code?

Comment 49

[Drew Willcoxon :adw]

Only one orange on tryserver, and it's known.

Comment 50

[Marco Bonardo [:mak]]

(In reply to comment #48)
> * We now have nsINavHistoryContainerResultNode.state and
>   containerOpen.  Making state read-write is not optimal, since
>   state = STATE_LOADING doesn't make sense.  At some point we
>   should maybe make open() and close() methods.

could evaluate that in bug 552025. If we are going to break compatibility with observers at that point we could thinking about making the API better (open() is clearly better than containerOpen = true). Add a note in that bug please.

> * Added nsNavHistoryContainerResultNode::NotifyOnStateChange and
>   funneled all observer notifications though it.  It notifies
>   through both the new containerStateChanged and the old methods.

this is fine till we implement bug 552025.

> * An xpconnect error is dumped to text console if a JS view
>   doesn't define containerStateChanged.  Any way around that?
>   When you use the bookmarks sidebar, e.g.

i doubt there is a way around that, and is fine that it notifies about missing methods. Mano could know some hack since he has far more experience than me, but i'd not hold my breath.

Btw the idea is that we immediately stop using containerOpened and containerClosed (and so on) and you replace them with stateChanged in all of our views and tests. Clearly for now you'll have to leave in stub methods for the old notifications. I don't recall if you did in this patch, if not it should be done. What bug 552025 should do is just remove the methods from the idl and all empty stubs.

> * What's the correct way to get the options for a container node?
>   Both result and folder nodes seem to assume mOptions is
>   non-null, but there's
>   nsNavHistoryResultNode::GetGeneratingOptions also.  (See
>   comment 46.)

this is an interesting question, GetGeneratingOptions skips the current container and goes looking up the parent, but actually mOptions is injected as you said. I think the point of it is when you have an option like mExcludeItems, you could be a container that has not ExcludeItems defined inside a query that has it defined. But actually it stops at the first parent so it does not look really useful as it could (indeed i added a direct check with the root node's options).
Have you tried looking at the CVS blame for it?
Mano could have some insight about it that i'm missing.

> * Similar question for mResult and GetResult.  In this case,
>   GetResult actually raises an assertion if the node is a
>   container and has a null mResult, so what's the point of ever
>   calling GetResult in container impl code?

i think it is just extra protection against cycle collector impl errors.

Comment 51

[Marco Bonardo [:mak]]

Comment on attachment 432226 [details] [diff] [review]
patch 4

some random comment, not a review

>diff --git a/toolkit/components/places/public/nsINavHistoryService.idl b/toolkit/components/places/public/nsINavHistoryService.idl
>   /**
>+   * When this is true, the root container node generated by these options and
>+   * all its descendant containers will be opened asynchronously when their
>+   * containerOpen members are set to true.
>+   *
>+   * @note Currently this is supported only for bookmark folder containers.
>+   */
>+  attribute boolean asyncEnabled;

So i think the comment should not say that they "will be opened asynchronously" but that they "could be" opened asynchronously if they support that opening method. And just say "when they are opened" rather than pointing to containerOpen set to true (since that could change, see your commenting above about open() method).
 
>diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp

>+nsresult
>+nsNavBookmarks::ProcessFolderNodeRow(
>+  mozIStorageValueArray* aRow,
>+  nsNavHistoryQueryOptions* aOptions,
>+  nsCOMArray<nsNavHistoryResultNode>* aChildren,
>+  PRInt32& aCurrentIndex)
>+{
>+  NS_ENSURE_ARG_POINTER(aRow);
>+  NS_ENSURE_ARG_POINTER(aOptions);
>+  NS_ENSURE_ARG_POINTER(aChildren);
>+
>+  nsresult rv;

define at first use please.

>+  if (itemType == TYPE_BOOKMARK) {
>+    nsNavHistory* history = nsNavHistory::GetHistoryService();
>+    NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);

i still find a bit unfortunate that we have to call GetHistoryService for each bookmark, it's a lot of work for 1 thousands bookmarks in a folder, can't we get it in the caller and pass it to the method?
I see it could be hard for the async calls. And we also getBookmarksService at each handleResult... ok let's hope it won't be too bad and that compiler loves us.

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

>+void
>+nsNavHistoryFolderResultNode::EnsureRegisteredAsFolderObserver()
>+{
>+  if (!mIsRegisteredFolderObserver && mResult) {
>+    mResult->AddBookmarkFolderObserver(this, mItemId);
>+    mIsRegisteredFolderObserver = PR_TRUE;
>+  }
>+}

this looks like convertable to a macro

So, i think you really want (i know that) to update our views to use the new stateChanged notification instead of open/close/whatever.
And... do you think we can use this somewhere already? bookmarks toolbar anyone? Follow-up a bug to actually use this, please.

Comment 52

[Marco Bonardo [:mak]]

(In reply to comment #51)
> So, i think you really want (i know that) to update our views to use the new
> stateChanged notification instead of open/close/whatever.

PS: this should still be done in a separate patch. ideally this bug is splitted into 3 parts (bugs):
1. backend impl
2. replace all of our views
3. remove old methods

Now 3 had its own bug, 1 has this bug. 2 can either have its own bug or be a new attachment part of this bug.

Comment 53

[Drew Willcoxon :adw]

(In reply to comment #50)
> could evaluate that in bug 552025. If we are going to break compatibility with
> observers at that point we could thinking about making the API better (open()
> is clearly better than containerOpen = true). Add a note in that bug please.

I think that discussion definitely deserves its own bug.  I'll file it once this bug lands.  Bug 552025 should be kept focused on the grunt work of replacing old API calls with the new.

> > * Added nsNavHistoryContainerResultNode::NotifyOnStateChange and
> >   funneled all observer notifications though it.  It notifies
> >   through both the new containerStateChanged and the old methods.
> 
> this is fine till we implement bug 552025.

Yes, it will either go away or be simplified with bug 552025's fix.

> Btw the idea is that we immediately stop using containerOpened and
> containerClosed (and so on) and you replace them with stateChanged in all of
> our views and tests. Clearly for now you'll have to leave in stub methods for
> the old notifications. I don't recall if you did in this patch, if not it
> should be done. What bug 552025 should do is just remove the methods from the
> idl and all empty stubs.

The old notifications are still supported with this patch, so they aren't stubs at all, because all our tests and browser consumers still depend on their working.  Bug 552025 will change everything over to the new API.  There will be a gap between when this patch lands and when bug 552025's fix lands, which is why I raised the concern about the JS warnings.

> > * Similar question for mResult and GetResult.  In this case,
> >   GetResult actually raises an assertion if the node is a
> >   container and has a null mResult, so what's the point of ever
> >   calling GetResult in container impl code?
> 
> i think it is just extra protection against cycle collector impl errors.

Well, this patch either calls GetResult or uses mResult after making sure it's non-null.  Seems OK, but it would be nice to know if/when we should always use one or the other.

(In reply to comment #51)
> >+  if (itemType == TYPE_BOOKMARK) {
> >+    nsNavHistory* history = nsNavHistory::GetHistoryService();
> >+    NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
> 
> i still find a bit unfortunate that we have to call GetHistoryService for each
> bookmark, it's a lot of work for 1 thousands bookmarks in a folder, can't we
> get it in the caller and pass it to the method?
> I see it could be hard for the async calls. And we also getBookmarksService at
> each handleResult... ok let's hope it won't be too bad and that compiler loves
> us.

Is it so bad?  nsNavHistory::GetHistoryService caches the service, and it's not a virtual function.

> >+void
> >+nsNavHistoryFolderResultNode::EnsureRegisteredAsFolderObserver()
> >+{
> >+  if (!mIsRegisteredFolderObserver && mResult) {
> >+    mResult->AddBookmarkFolderObserver(this, mItemId);
> >+    mIsRegisteredFolderObserver = PR_TRUE;
> >+  }
> >+}
> 
> this looks like convertable to a macro

Hmm, it could be, but should it?

> So, i think you really want (i know that) to update our views to use the new
> stateChanged notification instead of open/close/whatever.
> And... do you think we can use this somewhere already? bookmarks toolbar
> anyone? Follow-up a bug to actually use this, please.

(In reply to comment #52)
> 1. backend impl
> 2. replace all of our views
> 3. remove old methods
> 
> Now 3 had its own bug, 1 has this bug. 2 can either have its own bug or be a
> new attachment part of this bug.

Yeah, I agree, but 2 should be its own bug.  That's a big and distinct task that we can break into smaller pieces, at least per view.  I was thinking of tackling the tree view first, but I dunno.  There's also the work of making query nodes async.

Comment 54

[Marco Bonardo [:mak]]

(In reply to comment #53)
>  Bug 552025 will change everything over to the new API.  There will be
> a gap between when this patch lands and when bug 552025's fix lands, which is
> why I raised the concern about the JS warnings.

i think you should then at least add a stub onContainerStateChanged method everywhere we have an observer. This will shut up the warnings and also mark points for future work in bug 552025.

> Is it so bad?  nsNavHistory::GetHistoryService caches the service, and it's not
> a virtual function.

dunno but not-bad*1000 == maybe-bad ;) i'd guess we don't care for now.

> > this looks like convertable to a macro
> 
> Hmm, it could be, but should it?

i think there is no added benefit in having a method rather then a macro here.

> Yeah, I agree, but 2 should be its own bug.  That's a big and distinct task
> that we can break into smaller pieces, at least per view.  I was thinking of
> tackling the tree view first, but I dunno.  There's also the work of making
> query nodes async.

i'm unsure there is the need to a separate bug for each view if you just want to replace open/close notifications, work should mostly be a small code move. If instead you are thinking to larger changes or adding async support, then it would be better to split. Personally i'd prefer a single bug to convert to containerStateChanged, a bug to deprecate old notifications, and a bug to implement async support in views.

Comment 55

[Asaf Romano (gone)]

ETA: Early Sunday.

Comment 56

[Drew Willcoxon :adw]

Thanks Mano.  As Marco says I should add a stub containerStateChanged to every observer we already have to shutup the warnings.  I'll work on a "part 2" patch so it can be reviewed separately and attach it to this bug.

(In reply to comment #54)
> i'm unsure there is the need to a separate bug for each view if you just want
> to replace open/close notifications, work should mostly be a small code move.

You might be right (I hope!).  But yes, we should add async support, so it's not just a matter of replacing open/close but supporting the UI for loading as well.  We might want to wait to support loading until all container types are async, though, so there aren't UI inconsistencies...

> would be better to split. Personally i'd prefer a single bug to convert to
> containerStateChanged, a bug to deprecate old notifications, and a bug to
> implement async support in views.

We agree, except that I think we should split that last one into multiple bugs.  No biggie.

Comment 57

[Drew Willcoxon :adw]

Attachment: part 2 - containerStateChanged stubs to shutup warnings

This patch is much smaller than I was expecting, only 4 lines changed.  (We really have only one test that uses result observers?)  Mano and Marco, if you want it folded into the bigger patch, that would be fine with me.

Comment 58

[Asaf Romano (gone)]

Comment on attachment 432226 [details] [diff] [review]
patch 4

Just some nits:

+   * Called after a container changes state.
+   *
+   * @param aContainerNode
+   *        The container that has changed state.
+   * @param aOldState
+   *        The state that aContainerNode has transitioned out of.
+   * @param aNewState
+   *        The state that aContainerNode has transitioned into.
+   */
+  void containerStateChanged(in nsINavHistoryContainerResultNode aContainerNode,
+                             in unsigned long aOldState,
+                             in unsigned long aNewState);
+

Above containerOpened/Closed, please note that they're deprecated and suggest using this method instead.
 
   /**
+   * When this is true, the root container node generated by these options and
+   * all its descendant containers will be opened asynchronously when their
+   * containerOpen members are set to true.

not "all", see your note ;) I'm pretty we'll always have some containers that open synchronously.

You should also note wether or not it's turned on by default.

diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp
--- a/toolkit/components/places/src/nsNavBookmarks.cpp
+++ b/toolkit/components/places/src/nsNavBookmarks.cpp

+  nsRefPtr<nsNavHistoryResultNode> node;
+  if (itemType == TYPE_BOOKMARK) {
+    nsNavHistory* history = nsNavHistory::GetHistoryService();
+    NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);

Lately we started using NS_ENSURE_STATE for cases like this.

+  nsCOMPtr<mozIStoragePendingStatement> pendingStmt;
+  rv = mDBGetChildren->ExecuteAsync(aNode, getter_AddRefs(pendingStmt));
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  NS_ADDREF(*_pendingStmt = pendingStmt);

NS_IF_ADDREF, just in case.

 NS_IMETHODIMP
 nsNavHistoryContainerResultNode::SetContainerOpen(PRBool aContainerOpen)
 {
-  if (mExpanded && !aContainerOpen)
-    CloseContainer(PR_FALSE);
-  else if (!mExpanded && aContainerOpen)
-    OpenContainer();
+  if (aContainerOpen) {
+    if (!mExpanded) {
+      if (mOptions->AsyncEnabled())

Use GetGeneratingOptions here, you cannot assume that mOptions is set within "plain" containers.


+NS_IMETHODIMP
+nsNavHistoryContainerResultNode::GetState(PRUint16* _state)
+{

NS_ENSURE_ARG_POINTER here.

+  if (mExpanded)
+    *_state = STATE_OPENED;
+  else if (mAsyncPendingStmt)
+    *_state = STATE_LOADING;
+  else
+    *_state = STATE_CLOSED;
+

I prefer |: ?| syntax here.

+ * The async version of OpenContainer.
+ */
+nsresult
+nsNavHistoryContainerResultNode::OpenContainerAsync()
+{
+  return NS_OK;

NS_ERROR_NOT_IMPLEMENTED.

--- /dev/null
+++ b/toolkit/components/places/tests/queries/test_async.js
@@ -0,0 +1,395 @@

+ * Portions created by the Initial Developer are Copyright (C) 2009

2010 :)

r=mano. Awesome work!

Comment 59

[Marco Bonardo [:mak]]

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

> +    nsNavHistory* history = nsNavHistory::GetHistoryService();
> +    NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
> 
> Lately we started using NS_ENSURE_STATE for cases like this.

This is instead correct, NS_ENSURE_STATE is fine for generic null checks, but if the error can most likely come from a memory condition, we use OUT_OF_MEMORY... i'd say for constructors, memory allocations and get services.

Comment 60

[Marco Bonardo [:mak]]

yes, the edge is subtle.

Comment 61

[Marco Bonardo [:mak]]

(In reply to comment #57)
> Created an attachment (id=434439) [details]
> part 2 - containerStateChanged stubs to shutup warnings
> 
> This patch is much smaller than I was expecting, only 4 lines changed.  (We
> really have only one test that uses result observers?)  Mano and Marco, if you
> want it folded into the bigger patch, that would be fine with me.

I'm not surprised, when we add tests to views we usually add b-c tests, and this interface is in a sub hierarchy for them (they won't act on it). still we will most likely catch regressions at an higher level. Tests are never enough, though.

Comment 62

[Drew Willcoxon :adw]

Thanks Mano!  I've got a new patch that incorporates comment 51 (except the macro part), comment 58, and comment 59, but bug 507414 landed and broke it.  I commented there (comment 48), and depending on what kind of response it gets I'll attach the new patch and ask for super-review.

(In reply to comment #58)
> --- /dev/null
> +++ b/toolkit/components/places/tests/queries/test_async.js
> @@ -0,0 +1,395 @@
> 
> + * Portions created by the Initial Developer are Copyright (C) 2009
> 
> 2010 :)

Actually the first several versions of this patch were done over Christmas, and that file hasn't changed substantially since then.  I'm kind of sentimental about these things. :)

Comment 63

[Drew Willcoxon :adw]

unit/test_dynamic_containers.js fails with the new patch here, when |folder| is opened:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_dynamic_containers.js#85

Because the final NOTREACHED in GetGeneratingOptions is reached:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistoryResult.cpp#403

Because when a container is opened, I call GetGeneratingOptions as suggested in comment 58 and check if async is enabled.  But |folder|'s parent is a dynamic container, and GetGeneratingOptions doesn't handle that case.

What should I do?  I still don't get GetGeneratingOptions.  Why does it start at the node's parent when looking up the tree instead of the node itself?  Why does it only check for folders and queries?  Why doesn't it look like this:

nsNavHistoryQueryOptions*
nsNavHistoryResultNode::GetGeneratingOptions()
{
  nsNavHistoryResultNode* node = this;
  while (node) {
    if (node->IsContainer())
      return node->GetAsContainer()->mOptions;
    node = node->mParent;
  }

  NS_NOTREACHED(...);
  return nsnull;
}

Comment 64

[Drew Willcoxon :adw]

Tests pass and the browser works OK with the GetGeneratingOptions in comment 63 FWIW...

Comment 65

[Drew Willcoxon :adw]

Attachment: part 1 - async API

Had to make some changes from the r+'ed patch:

* The Storage changes noted in comment 62 required some extra
  QI's of Storage statements to mozIStorageValueArrays, and not
  just in the code I added.

* Marco explained GetGeneratingOptions to me, thanks Marco.  The
  dynamic container test failure noted in comment 63 required a
  change to it.  Instead of checking IsFolder and IsQuery in the
  mParent != null case, check IsContainer, the same way IsContainer
  is used in the mParent == null case.

Since I'm off today and this weekend, I'll ping someone for sr next week.

Comment 66

[Drew Willcoxon :adw]

Attachment: part 1 - async API

Noticed a small optimization: in nsNavBookmarks::QueryFolderChildren QI the statement to a mozIStorageValueArray outside of the loop instead of inside.  I'll ask for sr now.

Comment 67

[Drew Willcoxon :adw]

Comment on attachment 435927 [details] [diff] [review]
part 1 - async API

Vlad, requesting super-review for API changes to nsINavHistoryService.idl.  It's the first diff in the patch.

The main change is the addition of |asyncEnabled| on nsINavHistoryQueryOptions, which instructs containers generated by the options to open asynchronously, if they support being opened asynchronously.  (This patch implements async support for bookmark folders, the first containers to support async.)  Since containers can now be "loading" and not just opened or closed, we introduce the read-only, three-way nsINavHistoryContainerResultNode.state, which leads to the new nsINavHistoryResultObserver.containerStateChanged, which leads to deprecating container{Opened,Closed}.

Comment 68

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 435927 [details] [diff] [review]
part 1 - async API

Looks fine!

Comment 69

[Drew Willcoxon :adw]

Attachment: part 1 - async API (with commit message)

Just adding a commit message to make it easier for whoever checks this in.

Comment 70

[Drew Willcoxon :adw]

Attachment: part 2 - containerStateChanged stubs to shutup warnings (with commit message)

Just adding a commit message to make it easier for whoever checks this in.

Tryserver run looks OK.

Comment 71

[Bryan]

(In reply to comment #68)
> (From update of attachment 435927 [details] [diff] [review])
> Looks fine!

Does this have review+?

Comment 72

[Drew Willcoxon :adw]

Yes, both patches, parts 1 and 2, are reviewed and ready to land.  They're waiting on some kind soul to come by and check them in.

Comment 73

[Drew Willcoxon :adw]

Removing checkin-needed, bug 556376 landed and bitrotted the test.  I'll land tomorrow.

Comment 74

[Drew Willcoxon :adw]

Attachment: part 1 - async API (unbitrotted)

This fixes the test, which was bitrotted by bug 556376.

Comment 75

[Drew Willcoxon :adw]

Part 1: http://hg.mozilla.org/mozilla-central/rev/b1db3f3e3ac1
Part 2: http://hg.mozilla.org/mozilla-central/rev/c87274edac65

Comment 76

[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