Last Comment Bug 409279 - FUEL: provide a way to perform bookmark queries
: FUEL: provide a way to perform bookmark queries
Status: RESOLVED WONTFIX
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-20 13:27 PST by Adam Kowalczyk
Modified: 2016-01-28 08:02 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fuelIApplication.idl (4.46 KB, patch)
2008-08-03 12:42 PDT, Samer Ziadeh
mark.finkle: review-
Details | Diff | Review
fuelApplication.js (4.03 KB, patch)
2008-08-11 13:47 PDT, Samer Ziadeh
mark.finkle: review-
Details | Diff | Review
fuelIApplication.idl (4.46 KB, patch)
2008-08-11 13:48 PDT, Samer Ziadeh
mark.finkle: review-
Details | Diff | Review
fuelApplication.js (7.30 KB, patch)
2008-08-11 14:15 PDT, Samer Ziadeh
mark.finkle: review-
Details | Diff | Review
fuelIApplication.idl (5.31 KB, patch)
2008-08-11 14:16 PDT, Samer Ziadeh
mark.finkle: review+
Details | Diff | Review
fuelApplication.js (3.75 KB, patch)
2008-08-13 19:46 PDT, Samer Ziadeh
mark.finkle: review-
Details | Diff | Review
fuelIApplication.idl (2.49 KB, patch)
2008-08-13 19:46 PDT, Samer Ziadeh
mark.finkle: review-
Details | Diff | Review
fuelIApplication.idl (2.49 KB, patch)
2008-08-17 13:50 PDT, Samer Ziadeh
mark.finkle: review-
Details | Diff | Review
fuelIApplication.idl (2.22 KB, patch)
2008-08-17 18:41 PDT, Samer Ziadeh
mark.finkle: review+
Details | Diff | Review
fuelApplication.js (3.02 KB, patch)
2008-08-17 18:42 PDT, Samer Ziadeh
mark.finkle: review-
Details | Diff | Review
fuelApplication.js (3.30 KB, patch)
2008-08-18 13:55 PDT, Samer Ziadeh
mark.finkle: review-
Details | Diff | Review
fuelApplication.js (3.21 KB, patch)
2008-08-18 16:49 PDT, Samer Ziadeh
no flags Details | Diff | Review
fuelIApplication.idl (2.22 KB, patch)
2008-08-18 16:49 PDT, Samer Ziadeh
mark.finkle: review-
Details | Diff | Review
fuelApplication.js (3.32 KB, patch)
2008-09-08 08:48 PDT, Samer Ziadeh
no flags Details | Diff | Review
fuelApplication.js (3.32 KB, patch)
2008-09-16 12:01 PDT, Samer Ziadeh
mark.finkle: review+
Details | Diff | Review
fuelIApplication.idl (2.34 KB, patch)
2008-09-24 13:39 PDT, Samer Ziadeh
mark.finkle: review+
Details | Diff | Review
browser_BookmarkQuery.js (1.21 KB, text/plain)
2008-09-29 13:46 PDT, Samer Ziadeh
no flags Details

Description Adam Kowalczyk 2007-12-20 13:27:29 PST
Right now the only way of finding bookmarks in FUEL is to recursively iterate folders beginning with the root. Because the assumption is that in Firefox 3 users are going to accumulate a lot of bookmarks, this could be not efficient enough (not to mention inconvenient). 

I talked about it a little with Mark Finkle and we agreed that it would be useful if FUEL provided an interface for querying bookmarks, as a wrapper for nsINavHistoryQuery and friends. An example of how it could look like in action:

var query = bookmarks.newQuery(); 
query.type = "folder"; 
query.searchTerms = "goats"; 
var results = query.execute();
Comment 1 Samer Ziadeh 2008-04-14 21:40:57 PDT
why not use hash?

var folder = bookmsars['folder'];
Comment 2 Samer Ziadeh 2008-04-23 16:24:43 PDT
Hello, my name is Samer (irc: samer) I will be working on this feature for my GSoC project
Comment 3 Samer Ziadeh 2008-08-02 18:02:26 PDT
can I use nsINavHistoryService
http://developer.mozilla.org/en/docs/nsINavHistoryService
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2008-08-02 18:21:34 PDT
(In reply to comment #3)
> can I use nsINavHistoryService
> http://developer.mozilla.org/en/docs/nsINavHistoryService
> 

Yes, with the help of some other stuff:
http://developer.mozilla.org/en/docs/Retrieving_part_of_the_bookmarks_tree
Comment 5 Samer Ziadeh 2008-08-02 18:41:51 PDT
> 
> var query = bookmarks.newQuery(); 
> query.type = "folder"; 
> query.searchTerms = "goats"; 
> var results = query.execute();
> 
is that a good syntax for it? or should it be used the same way as nsINavHistoryService
Comment 6 Samer Ziadeh 2008-08-03 12:42:40 PDT
Created attachment 332130 [details] [diff] [review]
fuelIApplication.idl

I added 4 things to fuelIBookmark
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2008-08-03 14:57:44 PDT
I think we would need a new interface, pseudo code below:

interface fuelIBookmarkQuery {
string type;
string searchTerms;
nsIVariant execute();
};

and add a method to fuelIBookmarkFolder:

fuelIBookmarkQuery newQuery();

We won't add anything to fuelIBookmark since it can't contain any children.

Let's see how this goes.
Comment 8 Samer Ziadeh 2008-08-03 15:02:41 PDT
ah I had that initially and moved everything to fuelIBookmark I thought it was wrong :/
I'll change it back.
Comment 9 Samer Ziadeh 2008-08-11 13:47:45 PDT
Created attachment 333277 [details] [diff] [review]
fuelApplication.js
Comment 10 Samer Ziadeh 2008-08-11 13:48:17 PDT
Created attachment 333278 [details] [diff] [review]
fuelIApplication.idl
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2008-08-11 13:59:47 PDT
Comment on attachment 333278 [details] [diff] [review]
fuelIApplication.idl

Is this the same as the last patch? 

Let's discuss this on IRC sometime.
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2008-08-11 14:00:17 PDT
Comment on attachment 333277 [details] [diff] [review]
fuelApplication.js

Anything new here?
Comment 13 Samer Ziadeh 2008-08-11 14:15:40 PDT
Created attachment 333280 [details] [diff] [review]
fuelApplication.js
Comment 14 Samer Ziadeh 2008-08-11 14:16:05 PDT
Created attachment 333281 [details] [diff] [review]
fuelIApplication.idl
Comment 15 Mark Finkle (:mfinkle) (use needinfo?) 2008-08-13 11:39:34 PDT
Comment on attachment 333280 [details] [diff] [review]
fuelApplication.js

> // BookmarkFolder implementation
> function BookmarkFolder(aId, aParent) {
>   this._id = aId;
>   this._parent = aParent;
>   this._annotations = new Annotations(this._id);
>   this._events = new Events();
>+  this._query = null;

You don't seem to use this member

> BookmarkFolder.prototype = {
>   _shutdown : function bmf_shutdown() {
>     this._annotations = null;
>     this._events = null;
>+    this._query = null;

Remove if unused

>+//=================================================
>+// BookmarkQuery implementation
>+function BookmarkQuery() {
>+   this._options = Utilities.history.getNewQueryOptions();
>+   this._query = Utilities.history.getNewQuery();

These are both unused during the lifetime of the BookmarkQuery, so remove them. You only need them in the "execute" method.

>+   this._type = null;
>+   this._searchTerms = null;
>+}
>+
>+BookmarkQuery.prototype = {
>+  _shutdown: function bmq_shutdown() {
>+     this._options = null;
>+     this._query = null;

Remove _query and _options

>+   
>+  execute: function bmq_execute() {
>+    var rv = [];

You need to make the query and options locally now. AND you need to use the _type and _searchTerms here too. Currently, you are not searching for anything.

>+    var result = Utilities.history.executeQuery(this._query, this._options);
>+    var root = result.root;
>+    root.containerOpen = true;
>+    
>+    for (var i = 0; i < root.childCount; i++) {
>+        rv.push(root.getChild(i));

You may need to create a BookmarkFolder or Bookmark here and NOT use whatever raw object comes back from the query.
Comment 16 Mark Finkle (:mfinkle) (use needinfo?) 2008-08-13 11:40:47 PDT
Comment on attachment 333281 [details] [diff] [review]
fuelIApplication.idl

I would like a patch that did not have the other bug patch in it as well.
Comment 17 Samer Ziadeh 2008-08-13 19:46:23 PDT
Created attachment 333676 [details] [diff] [review]
fuelApplication.js

how will searchTerm and type be used i'm a little confused about those
Comment 18 Samer Ziadeh 2008-08-13 19:46:54 PDT
Created attachment 333677 [details] [diff] [review]
fuelIApplication.idl
Comment 19 Mark Finkle (:mfinkle) (use needinfo?) 2008-08-17 13:41:56 PDT
Comment on attachment 333677 [details] [diff] [review]
fuelIApplication.idl

This patch is broken. It appears to have somehow landed in the middle of fuelIBookmarkRoots.
Comment 20 Samer Ziadeh 2008-08-17 13:50:19 PDT
Created attachment 334186 [details] [diff] [review]
fuelIApplication.idl
Comment 21 Mark Finkle (:mfinkle) (use needinfo?) 2008-08-17 13:53:35 PDT
Comment on attachment 333676 [details] [diff] [review]
fuelApplication.js

>Index: browser/fuel/src/fuelApplication.js

>+  newQuery: function bmf_newQuery() {
>+    return new BookmarkQuery(this._id);
>+  },
>+  
>+  QueryInterface : XPCOMUtils.generateQI([Ci.fuelIBookmarkFolder, Ci.fuelIBookmarkQuery, Ci.nsINavBookmarkObserver])
> };

Why is fuelIBookmarkQuery here? Shouldn't be
> 
> //=================================================
> // BookmarkRoots implementation
> function BookmarkRoots() {
>   var self = this;
>   gShutdown.push(function() { self._shutdown(); });
> }
>@@ -644,16 +647,70 @@ BookmarkRoots.prototype = {
>       this._unfiled = new BookmarkFolder(Utilities.bookmarks.unfiledBookmarksFolder, null);
> 
>     return this._unfiled;
>   },
> 
>   QueryInterface : XPCOMUtils.generateQI([Ci.fuelIBookmarkRoots])
> };
> 
>+//=================================================
>+// BookmarkQuery implementation
>+function BookmarkQuery(aBookmarkFolder) {
>+   this._type = null;
>+   this._searchTerms = null;
>+   this._folder = aBookmarkFolder;
>+}
>+
>+BookmarkQuery.prototype = {
>+  _shutdown: function bmq_shutdown() {
>+     this._type = null;
>+     this._searchTerms = null;
>+  },

No need for _shutdown anymore. This is only used to release class instances, not primitives.

>+  set type(aType) {
>+     if (aType)

Since you initialize to null, then you'll have to check for null in "execute" and therefore you don't need to check here

>+
>+  set searchTerms(aSearchTerm) {
>+     if (aSearchTerm)

Same as above

>+  execute: function bmq_execute() {
>+    var rv = [];
>+    
>+    var options = Utilities.history.getNewQueryOptions();
>+    options.excludeItems = true;
>+    
>+    var query = Utilities.history.getNewQuery([this._searchTerms], 1);

This is wrong.

Use the example here to help you figure out how to use searchterm:
http://developer.mozilla.org/en/docs/Places_migration_guide#Searching_2
Notice the search term "firefox" is used in the example.

>+    
>+    var result = Utilities.history.executeQuery(query, options);
>+    var root = result.root;
>+    root.containerOpen = true;
>+    
>+    for (var i = 0; i < root.childCount; i++) {
>+        rv.push(root.getChild(i));

Keep in mind, you will probably need to wrap the results in BookmarFolder or Bookmark instances.
Comment 22 Mark Finkle (:mfinkle) (use needinfo?) 2008-08-17 13:59:49 PDT
Comment on attachment 334186 [details] [diff] [review]
fuelIApplication.idl

>Index: browser/fuel/public/fuelIApplication.idl
>+
>+  /**
>+   * The folder for the 'unfiled bookmarks' root.
>+   */
>+  readonly attribute fuelIBookmarkFolder unfiled;
>+  
>+  /**
>+   * This returns a new query object that you can pass to execute.
>+   * It will be initialized to all empty (so using it will give you all history).
>+   */  
>+  fuelIBookmarkQuery newQuery();
> };

Why has "unfiled" been added to BookmarkFolder?

> 
> /**
>  * Interface representing a container for bookmark roots. Roots
>  * are the top level parents for the various types of bookmarks in the system.
>  */
> [scriptable, uuid(c9a80870-eb3c-11dc-95ff-0800200c9a66)]
> interface fuelIBookmarkRoots : nsISupports
>@@ -251,21 +263,35 @@ interface fuelIBookmarkRoots : nsISuppor
>    * The folder for the 'personal toolbar' root.
>    */
>   readonly attribute fuelIBookmarkFolder toolbar;
> 
>   /**
>    * The folder for the 'tags' root.
>    */
>   readonly attribute fuelIBookmarkFolder tags;
>+};
> 
>+
>+[scriptable, uuid(6de026ca-548f-42f5-8cf1-c3c835ba0e6a)]
>+interface fuelIBookmarkQuery : nsISupports {
>   /**
>-   * The folder for the 'unfiled bookmarks' root.
>+   * The type of the bookmark item
>    */
>-  readonly attribute fuelIBookmarkFolder unfiled;
>+  attribute AString type;
>+  
>+  /**
>+   * The item to search for
>+   */
>+  attribute AString searchTerm;
>+  
>+  /**
>+   * Executes a single query.
>+   */  
>+  nsIVariant execute();
> };
> 

Why was "unfiled" removed from BookmarkQuery, especially since it was never there.

You might need to do a fresh source pull and of this file from CVS and edit it again.
Comment 23 Samer Ziadeh 2008-08-17 18:41:40 PDT
Created attachment 334207 [details] [diff] [review]
fuelIApplication.idl

I hope this one works
Comment 24 Samer Ziadeh 2008-08-17 18:42:49 PDT
Created attachment 334208 [details] [diff] [review]
fuelApplication.js
Comment 25 Mark Finkle (:mfinkle) (use needinfo?) 2008-08-17 20:19:18 PDT
Comment on attachment 334207 [details] [diff] [review]
fuelIApplication.idl

Good
Comment 26 Mark Finkle (:mfinkle) (use needinfo?) 2008-08-17 20:38:14 PDT
Comment on attachment 334208 [details] [diff] [review]
fuelApplication.js

>Index: browser/fuel/src/fuelApplication.js
> 
>-  QueryInterface : XPCOMUtils.generateQI([Ci.fuelIBookmarkFolder, Ci.nsINavBookmarkObserver])
>+  QueryInterface : XPCOMUtils.generateQI([Ci.fuelIBookmarkFolder, Ci.fuelIBookmarkQuery, Ci.nsINavBookmarkObserver])

Remove Ci.fuelIBookmarkQuery from this list. fuelIBookmarkFolder is not a fuelIBookmarkQuery.

>+BookmarkQuery.prototype = {
>+  _shutdown: function bmq_shutdown() {
>+     this._type = null;
>+     this._searchTerms = null;
>+  },

Remove _shutdown function, you are not using it anywhere and it is not needed anyway. We only need _shutdown functions to release objects held by a class, not for primitive types (strings in this case)

>+  set type(aType) {
>+     if (aType)
>+        this._type = aType;
>+  },

Remove the check for a null aType. You initialize this._type to null, so you need to do the check in "execute()", not here.

>+  set searchTerms(aSearchTerm) {
>+     if (aSearchTerm)
>+        this._searchTerms = aSearchTerm
>+  },

Remove the check for a null aSearchTerm. You initialize this._searchTerms to null, so you need to do the check in "execute()", not here.

>+   
>+  execute: function bmq_execute() {
>+    var rv = [];
>+    
>+    var options = Utilities.history.getNewQueryOptions();
>+    options.excludeItems = true;
>+    
>+    var query = Utilities.history.getNewQuery([this._searchTerms], 1);
>+    
>+    var result = Utilities.history.executeQuery(query, options);
>+    var root = result.root;
>+    root.containerOpen = true;

This is wrong. Use http://developer.mozilla.org/en/docs/Places:Migration_Guide#Searching_2 to help you figure it out.

>+    
>+    for (var i = 0; i < root.childCount; i++) {
>+        rv.push(root.getChild(i));

Need to convert to Bookmark or BookmarkFolder, just like we do in BookmarkFolder.children

I posted the same comments to an earlier review!
Comment 27 Samer Ziadeh 2008-08-18 13:55:00 PDT
Created attachment 334336 [details] [diff] [review]
fuelApplication.js
Comment 28 Mark Finkle (:mfinkle) (use needinfo?) 2008-08-18 16:35:18 PDT
Comment on attachment 334336 [details] [diff] [review]
fuelApplication.js

OK. one small change. Let's use "this._type == null" to mean "any type" instead of "*".

Ok?

The changes below take that into account.

>Index: browser/fuel/src/fuelApplication.js

>+  execute: function bmq_execute() {
>+    if(!this._searchTerms || !this._type)

Add a space bewteen the "if("

Remove the "|| !this._type", we want to allow a null type

>+      return false;
>+    
>+    var folders = [ bookmarks.toolbarFolder, bookmarks.bookmarksMenuFolder, bookmarks.unfiledBookmarsFolder ];

I think we should use "this._folder" here. We want to search the given folder, not all of those top level folders.

>+      
>+      var item = null;
>+      

Remove this blank line

>+      if (node.type == this._type || this._type == '*') {

Change to:
        if (node.type == this._type || this._type == null) {

So close. Add a new patch with these changes for a win! Also, work on some tests!
Comment 29 Samer Ziadeh 2008-08-18 16:49:04 PDT
Created attachment 334365 [details] [diff] [review]
fuelApplication.js
Comment 30 Samer Ziadeh 2008-08-18 16:49:37 PDT
Created attachment 334366 [details] [diff] [review]
fuelIApplication.idl

I had a typo in there and forgot a semicolon
Comment 31 Samer Ziadeh 2008-09-08 08:48:04 PDT
Created attachment 337463 [details] [diff] [review]
fuelApplication.js

modified the execute function.
Comment 32 Samer Ziadeh 2008-09-16 12:01:28 PDT
Created attachment 338907 [details] [diff] [review]
fuelApplication.js

fixed typo in execute function from node.itemID to node.itemId
Comment 33 Mark Finkle (:mfinkle) (use needinfo?) 2008-09-18 09:34:05 PDT
Comment on attachment 334366 [details] [diff] [review]
fuelIApplication.idl

>Index: browser/fuel/public/fuelIApplication.idl

> /**
>+ * Interface representing a bookmark query
>+ */
>+[scriptable, uuid(6de026ca-548f-42f5-8cf1-c3c835ba0e6a)]
>+interface fuelIBookmarkQuery : nsISupports

>+  /**
>+   * Executes a single query.
>+   */  
>+  nsIVariant execute();
>+};

Please add a comment about the return type for "execute". Look at other examples in the IDL file for how to format it. Basicially, we need to tell programmers what the nsIVariant is holding.

Looks great
Comment 34 Samer Ziadeh 2008-09-24 13:39:49 PDT
Created attachment 340197 [details] [diff] [review]
fuelIApplication.idl
Comment 35 Simon Bünzli 2008-09-24 14:15:19 PDT
(In reply to comment #0)
> var query = bookmarks.newQuery(); 
> query.type = "folder"; 
> query.searchTerms = "goats"; 
> var results = query.execute();

What's wrong with |bookmarks.query("goats", "folder")| ?

AFAICT from looking at the code of attachment #338907 [details] [diff] [review], the BookmarkQuery object isn't reusable in any way, the |type| and |searchTerms| getters aren't needed (unless you pass the object around before executing, which in my book isn't more efficient than passing the values around without that wrapper) and newQuery could just as well take accept default initial values since at least the search terms are required for actually executing the query.

So unless you plan on expanding the BookmarkQuery significantly in a later step, I (as an extension author) would prefer the one-line function to the four-line one (and even more so the functional programmer in me).

Should you be dead set on allowing to reuse/repeat a BookmarkQuery, what about at least allowing |bookmarks.newQuery("goats", "folder").execute()| ?
Comment 36 Simon Bünzli 2008-09-24 14:31:24 PDT
And while I'm at it: What about having an empty query return all bookmarks instead of none, so that I could use |bookmarks.query("", null).filter(...)| for matching against a RegExp or any other more elaborate filter? Getting |false| back doesn't strike me as particularly useful, either... ;-)
Comment 37 Samer Ziadeh 2008-09-24 14:52:43 PDT
I'll have to talk it over with Mark since he's the one deciding. But I like your ideas
Comment 38 Adam Kowalczyk 2008-09-24 15:19:36 PDT
(In reply to comment #35)
> So unless you plan on expanding the BookmarkQuery significantly in a later
> step, I (as an extension author) would prefer the one-line function to the
> four-line one (and even more so the functional programmer in me).

My proposal was just a quick example and I am not attached to it at all. However, there are many other query parameters that could be added: tags, visit count, visit date, date added, annotations etc.
Comment 39 Mark Finkle (:mfinkle) (use needinfo?) 2008-09-25 10:22:56 PDT
(In reply to comment #35)
> (In reply to comment #0)
> > var query = bookmarks.newQuery(); 
> > query.type = "folder"; 
> > query.searchTerms = "goats"; 
> > var results = query.execute();
> 
> What's wrong with |bookmarks.query("goats", "folder")| ?
> 
> AFAICT from looking at the code of attachment #338907 [details] [diff] [review], the BookmarkQuery object
> isn't reusable in any way, the |type| and |searchTerms| getters aren't needed
> (unless you pass the object around before executing, which in my book isn't
> more efficient than passing the values around without that wrapper) and
> newQuery could just as well take accept default initial values since at least
> the search terms are required for actually executing the query.
> 
> So unless you plan on expanding the BookmarkQuery significantly in a later
> step, I (as an extension author) would prefer the one-line function to the
> four-line one (and even more so the functional programmer in me).
> 
> Should you be dead set on allowing to reuse/repeat a BookmarkQuery, what about
> at least allowing |bookmarks.newQuery("goats", "folder").execute()| ?

I'd actually like:

bookmarks.query({terms:"goats", type:"folder"});

I don't like creating an ever growing list of function args to extend the feature. If we could pass a literal JS object, I'd say we'd use it to set the defaults and then your one-liner would work.

BookmarkQuery is an IDL stand-in for a JS literal object.

(In reply to comment #36)
> And while I'm at it: What about having an empty query return all bookmarks
> instead of none, so that I could use |bookmarks.query("", null).filter(...)|
> for matching against a RegExp or any other more elaborate filter? Getting
> |false| back doesn't strike me as particularly useful, either... ;-)

Hmm, returning |false| is not right - (Samer: we should at least return an empty array |return [];| )

As to returning everything instead of nothing, we could look into that, but it could be a bad performance problem.
Comment 40 Samer Ziadeh 2008-09-25 12:03:42 PDT
The only time I'm returning false is if searchTerms is null.
Should I make it to search for '*' if searchTerm is null. execute returns an empty array if nothing was found.

Another thing while testing the script, whenever I set query.type nothing gets found, but when I leave query.type blank I get results.
Comment 41 Samer Ziadeh 2008-09-29 13:46:15 PDT
Created attachment 340984 [details]
browser_BookmarkQuery.js

Testing for BookmarkQuery.type isn't working, or I'm probably putting in the wrong values. I tried 'folder', 'uri', 'URI', 'url', and 'URL'
Comment 42 Dietrich Ayala (:dietrich) 2008-12-18 12:49:03 PST
i second mfinkle's suggestion for simplifying the query semantics. this removes the need for an exposed query object altogether.

also, callers will likely want to search all bookmarks sometimes. with the proposed api they'd need to query against each root folder. a convenience api like so would be nice:

// searches all roots
var fullSearchResults = Application.bookmarks.query({...});
Comment 43 Marco Bonardo [::mak] 2016-01-28 08:02:37 PST
FUEL is being removed in bug 1090880, so this is now wontfix.

Note You need to log in before you can comment on or make changes to this bug.