457708 - Make dynamic containers serializable as queries (nsNavHistoryDynamicContainerResultNode)

Status: RESOLVED WONTFIX

(Firefox :: Bookmarks & History, defect, P3)

Description

[Marco Bonardo [:mak]]

Attachment: patch

Dynamic containers are actually simple nsNavHistoryContainerResultNode, the main problems about that are:
- we can't persist them in localstore since they don't have an uri
- we can't show their contents in the library right pane, indeed the right pane uses the serialized query obtained through asQuery(node), all other resultnode types supports asQuery being inherited from nsNavHistoryQueryResultNode, this is not true for dynamic containers.

So a new resultNode type should make easier having common queries code and dynamic containers code separated, making all of this working.

the attached patch is all needed to make dynamic containers working correctly in our UI (splitted from patch in bug 453530), will try to add some cleanup and some test.

Comment 1

[Marco Bonardo [:mak]]

or easier, make them nsNavHistoryFolderResultNode with a contractId, should involve less code changes

Comment 2

[Marco Bonardo [:mak]]

Attachment: wip 0.1

this passes our current unit tests, but needs still some test and fix to ensure dyn containers are working as intended.

Comment 3

[Marco Bonardo [:mak]]

Attachment: wip 0.2

enabled most of the dyn containers apis (with modified test) and fixed another couple glitches. Still need to enable nested dynamic containers, and we should be mostly done.
Sortcuts to dyn containers are actually supported for completeness and working, but their "design" is only half-done, they have only the concreteId actually, i'm not even sure we need them for any purpose, so could be a followup.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.0

if i continue looking for perfection this won't ever land, current patch passes all tests, has additional tests, and should be good enough to moving on with livemarks. PAtch is big but final result should be a cleaner file since there is a more radical separation between containers, folders and dynamic containers.

Eventual issues or additions to the interface can be made while working on livemarks conversion and in follow-ups.
For the most part this patch enables unused features, so the risk is low atm.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 387194 [details] [diff] [review]
patch v1.0

>+  /**
>+   * Appends a full visit node to this container and returns it.
>+   *
>+   * @see nsINavHistoryFullVisitResultNode for parameters.
>+   */

should note that full visit nodes aren't currently supported?

>+  /**
>+   * Appends a bookmark container node to this container and returns it.
>+   *
>+   * All container attributes will come from the boomkarks service for this
>+   * container.
>+   */
>+  nsINavHistoryContainerResultNode appendBookmarksContainerNode(
>+    in PRInt64 aFolderId);

why not folder node?

> nsresult
>-nsNavBookmarks::GetFolderType(PRInt64 aFolder, nsACString &aType)
>+nsNavBookmarks::GetDynamicContainerServiceContractID(PRInt64 aItemId,
>+                                                     nsACString &aServiceContractID)

i'm beginning to think you could remove "service" from everywhere. and maybe leave out "contract" as well. don't know, but this seems overly long.

> nsNavHistoryContainerResultNode::AreChildrenVisible()
> {
>-  // can't see children when we're invisible
>+  // Can't see children when we are not open.

could be both?

>@@ -561,17 +536,19 @@ nsNavHistoryContainerResultNode::FillSta
>     accessCount += node->mAccessCount;
>     // this is how container nodes get sorted by date
>     // The container gets the most recent time of the child nodes.
>     if (node->mTime > newTime)
>       newTime = node->mTime;
>   }
> 
>   if (mExpanded) {
>+    // We use this property to sort containers by visit count.
>     mAccessCount = accessCount;
>+    // Some query provides its time based on options (eg. day queries).

s/query provides its time/queries provide their time/

>+  // recursively close all child containers
>+  for (PRInt32 i = 0; i < mChildren.Count(); i++) {
>+    if (mChildren[i]->IsContainer() &&
>+        mChildren[i]->GetAsContainer()->mExpanded)
>+      mChildren[i]->GetAsContainer()->CloseContainer(PR_FALSE);
>+  }

are we doing any recalculating when this occurs? would it be cheaper to walk it from the bottom up?

>+// nsNavHistoryDynamicContainerResultNode::GetQueries
>+//
>+//    This just returns the queries that give you this bookmarks folder

s/bookmarks//

>+NS_IMETHODIMP
>+nsNavHistoryDynamicContainerResultNode::GetQueries(PRUint32* queryCount,
>+                                         nsINavHistoryQuery*** queries)
>+{
>+  // Get the query object.
>+  nsCOMPtr<nsINavHistoryQuery> query;
>+  nsNavHistory* history = nsNavHistory::GetHistoryService();
>+  NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
>+  nsresult rv = history->GetNewQuery(getter_AddRefs(query));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // Guery just has the folder ID set and nothing else.

s/Guery/Query/

>+// Dynamic containers are populated on containerOpen.
>+// If the container is closed we should null the viewer and open the container
>+// before counting children, and finally restore the viewer.
>+// This would be slow though, and the associated service can probably give us
>+// an answer faster, so we will simply ask him.

s/should/could/
s/him/it/

>+
>+NS_IMETHODIMP
>+nsNavHistoryDynamicContainerResultNode::ClearContents()
>+{
>+  // we know if CanRemoteContainersChange() then we are a regular container

nit: capitalize beginning of sentence

>diff --git a/toolkit/components/places/tests/unit/nsDynamicContainerAsyncServiceSample.js b/toolkit/components/places/tests/unit/nsDynamicContainerAsyncServiceSample.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/places/tests/unit/nsDynamicContainerAsyncServiceSample.js

>+ * The Initial Developer of the Original Code is Mozilla Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2007
>+ * the Initial Developer. All Rights Reserved.

2007?!

>+RemoteContainerAsyncSampleService.prototype = {
>+
>+  get bms() {
>+    if (!this._bms) {
>+      this._bms = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+                  getService(Ci.nsINavBookmarksService);
>+    }
>+    return this._bms;
>+  },
>+
>+  get history() {
>+    if (!this._history) {
>+      this._history = Cc["@mozilla.org/browser/nav-history-service;1"].
>+                      getService(Ci.nsINavHistoryService);
>+    }
>+    return this._history;
>+  },
>+
>+  get ios() {
>+    if (!this._ios) {
>+      this._ios = Cc["@mozilla.org/network/io-service;1"].
>+                  getService(Ci.nsIIOService);
>+    }
>+    return this._ios;
>+  },
>+
>+  get db() {
>+    if (!this._db)
>+      this._db = this.history.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection;
>+    return this._db;
>+  },

i know it's just a test, but irks me that these aren't memoized.

>+
>+  classDescription: "Remote Container Async Sample Service",

s/Remote/Dynamic/ here and elsewhere in the tests

Comment 6

[Marco Bonardo [:mak]]

(In reply to comment #5)
> (From update of attachment 387194 [details] [diff] [review])
> >+  /**
> >+   * Appends a full visit node to this container and returns it.
> >+   *
> >+   * @see nsINavHistoryFullVisitResultNode for parameters.
> >+   */
> 
> should note that full visit nodes aren't currently supported?

i added note to all references to full visit nodes... we were not specifying that anywhere. Btw, probably full visit nodes should just be deprecated, and missing infos added to visit nodes. This is OT though.

> >+  nsINavHistoryContainerResultNode appendBookmarksContainerNode(
> >+    in PRInt64 aFolderId);
> 
> why not folder node?

because you can also append queries/dynamic containers/other existing containers. aFolderId was a poorly choosen name for the param, changed to aContainerId.

> 
> > nsresult
> >-nsNavBookmarks::GetFolderType(PRInt64 aFolder, nsACString &aType)
> >+nsNavBookmarks::GetDynamicContainerServiceContractID(PRInt64 aItemId,
> >+                                                     nsACString &aServiceContractID)
> 
> i'm beginning to think you could remove "service" from everywhere. and maybe
> leave out "contract" as well. don't know, but this seems overly long.

then it would become GetDynamicContainerID, that would be wrong.
i changed ServiceContractID to ContractID everywhere to reduce verbosity.

> > nsNavHistoryContainerResultNode::AreChildrenVisible()
> > {
> >-  // can't see children when we're invisible
> >+  // Can't see children when we are not open.
> 
> could be both?

yes but the code this comment refers to is only checking open status, visibility is checked later, so the comment was just wrong.


> >+  // recursively close all child containers
> >+  for (PRInt32 i = 0; i < mChildren.Count(); i++) {
> >+    if (mChildren[i]->IsContainer() &&
> >+        mChildren[i]->GetAsContainer()->mExpanded)
> >+      mChildren[i]->GetAsContainer()->CloseContainer(PR_FALSE);
> >+  }
> 
> are we doing any recalculating when this occurs? would it be cheaper to walk it
> from the bottom up?

since we pass in PR_FALSE, no, it should not recalculate views, and i can't see a reason why doing backwards would be faster being recursive. but since i looked at it, i fixed a bug, closing a container should QI to dynamic container in case, to correctly call OnContainerNodeClosed. So i fixed that.

fixed the other comments, i'm actually investigating a test failure, then i'll attach a new patch to ask for SR, based on current rules, since patch involves api changes.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.1

i suspect when we will really use the containers we could find further changes to do, but for now this should be fine, and involves the biggest code moves. Since it's bitrotting fast, i prefer taking this checkpoint asap.

Asking SR since this involves changes to the APIs (mostly moves and removals of unused ones).

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 392258 [details] [diff] [review]
patch v1.1

clearing sr request, hasChildren should pass the node, so i have to fix the API.

I plan to do further changes in future to allow hierarchic contents, so this won't be much frozen. as soon as i start reworking livemarks some need could become clearer.

Comment 9

[Marco Bonardo [:mak]]

actually i did add back hierarchic support to dynamic containers, and after doing that i tried implementing one and i noticed how ugly current implementation is... bad for everyone.
So i spent some time discussing with Mano in places, evaluating what we need, which problems we have, and exchanging ideas.
We will start a new project to completely get rid of this mess and reimplement dynamic containers in a saner way.

We will soon fill project page for the change.

For this reason i'm unassigning bug from me. dynamic containers won't make 3.6, but we needed them for 3.7 livemarks changes, so that won't change our long term plan.

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.2

checkpoint for reference.

Comment 11

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 12

[Marco Bonardo [:mak]]

no more dynamic containers fwiw