See https://developer.chrome.com/extensions/bookmarks#method-search

Comment on attachment 8692368 [details] [diff] [review] Implement chrome.bookmarks.search This is a WIP to get feedback before doing most of validation, docs and testing. We're currently using the new Async Bookmarks.jsm API for all operations in chrome.bookmarks, which is a breeze to use and allows us to give our bookmarks the same object structure as they have in chrome. However, Bookmarks.jsm doesn't have any query/search functionality. Therefore, my first attempt was to use the nsINavHistoryService, as described in https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Querying. The Addon SDK uses that one. I think it's not good for our use case though, half of my code was comprised of ugly hacks to fit the structure of the returned nodes and there were some issues like not being able to trivially find the parent folder of result nodes that I was unable to solve. Also, it's sync. I ended up extending the async Bookmarks.jsm with a search function and would like to check with Mossop if there are any issues with doing that :)

Comment on attachment 8692368 [details] [diff] [review] Implement chrome.bookmarks.search Review of attachment 8692368 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense to me, I defer to the places folks to review it though.

Comment on attachment 8700371 [details] [diff] [review] Implement chrome.bookmarks.search Mossop, can you forward this to anyone you'd like for review in the places team? Thanks :)

(In reply to Johann Hofmann [:johannh] from comment #2) > Therefore, my first attempt was to use the nsINavHistoryService, as > described in > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Querying. The > Addon SDK uses that one. I think it's not good for our use case though, half > of my code was comprised of ugly hacks to fit the structure of the returned > nodes and there were some issues like not being able to trivially find the > parent folder of result nodes that I was unable to solve. Also, it's sync. Changing the data structure should not be a big deal, sounds like an helper function could easily walk the result and build an array with the proper structure. Did you hit other issues? The fact the API is synchronous is a detail, yes it's horrible, but we can wrap it in an async API and replace it in future when an async alternative will exist. From the outside it doesn't make a difference. Finding the parent is a bigger problem that comes from a long time ago, the current querying API is very old and we never had resources to rewrite it. We may add the parentGuid to each result node if needed, the patch will be very similar to the one that added bookmarkGuid. > I ended up extending the async Bookmarks.jsm with a search function and > would like to check with Mossop if there are any issues with doing that :) I'm not thrilled by this, mostly cause the original plan was to have a more general async querying API and this will be a temporary placeholder that will become hard to remove once add-ons start using it. But if the alternatives are too expensive or worse we will take it (eventually in future it may just wrap the general API). Now, the problem with the current approach is that LIKE is case insensitive only for ASCII characters. Anything out of ASCII would fail to match and that may be a problem for some of our users. You could use the autocomplete SQL function (AUTOCOMPLETE_MATCH) that is not exactly trivial to use, but you could just copy what nsINavHistoryQuery is doing (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#3327). The problem is that it will match a single phrase, while the Crome API supports matching different words on URLs and Titles at the same time... We don't have such kind of ability. Could you please have a look at which kind of query Chrome is running when you search one word on the URI and one on the Title? Do they use LIKE or a fulltext index? In the worst case, we may need to run one of the 2 searches using AUTOCOMPLETE_MATCH (will match both uri and title), and then apply a further filter in javascript for the other search, throwing away some results.

(In reply to Marco Bonardo [::mak] from comment #6) Thanks for the comment :) I'm obviously not as experienced with the Places API as you are, so please correct me if my answers are based on false assumptions. > (In reply to Johann Hofmann [:johannh] from comment #2) > > Therefore, my first attempt was to use the nsINavHistoryService, as > > described in > > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Querying. The > > Addon SDK uses that one. I think it's not good for our use case though, half > > of my code was comprised of ugly hacks to fit the structure of the returned > > nodes and there were some issues like not being able to trivially find the > > parent folder of result nodes that I was unable to solve. Also, it's sync. > > Changing the data structure should not be a big deal, sounds like an helper > function could easily walk the result and build an array with the proper > structure. Did you hit other issues? > The fact the API is synchronous is a detail, yes it's horrible, but we can > wrap it in an async API and replace it in future when an async alternative > will exist. From the outside it doesn't make a difference. Sure, I had a helper function (which I still found kinda ugly) that transformed the "old" bookmark properties. I had troubles querying for folders but I stopped looking into that when I changed Bookmarks.jsm instead. > Finding the parent is a bigger problem that comes from a long time ago, the > current querying API is very old and we never had resources to rewrite it. > We may add the parentGuid to each result node if needed, the patch will be > very similar to the one that added bookmarkGuid. > > I ended up extending the async Bookmarks.jsm with a search function and > > would like to check with Mossop if there are any issues with doing that :) > > I'm not thrilled by this, mostly cause the original plan was to have a more > general async querying API and this will be a temporary placeholder that > will become hard to remove once add-ons start using it. > But if the alternatives are too expensive or worse we will take it > (eventually in future it may just wrap the general API). So it seems like there is ugliness on both sides of the fence. Moving forward now means investing time and technical debt into temporary solutions that will become obsolete once the ideal async querying API arrives. No matter what we do. Since this bug is not blocking webextensions in any way I don't take offense if you just say no thanks and let's wait until async querying is ready. We could just make a bug for that and put this one to blocked. Though I don't find having a placeholder as problematic as you do, apparently. As long as the external API stays backwards-compatible (something that's certainly achievable for a common task like querying) the internals of the function can be improved at will. Maybe I'm misunderstanding the meaning of "general querying", is it supposed to have any other external functionality than my simple function? :) > Now, the problem with the current approach is that LIKE is case insensitive > only for ASCII characters. Anything out of ASCII would fail to match and > that may be a problem for some of our users. > You could use the autocomplete SQL function (AUTOCOMPLETE_MATCH) that is not > exactly trivial to use, but you could just copy what nsINavHistoryQuery is > doing > (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/ > nsNavHistory.cpp#3327). The problem is that it will match a single phrase, > while the Crome API supports matching different words on URLs and Titles at > the same time... We don't have such kind of ability. > > Could you please have a look at which kind of query Chrome is running when > you search one word on the URI and one on the Title? Do they use LIKE or a > fulltext index? If I'm reading it correctly they don't do any query but iterate down from the root node comparing everything using a function aptly named DoesBookmarkContainWords (https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmarks/browser/bookmark_utils.cc&sq=package:chromium&l=97&rcl=1451997057 and https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmarks/browser/bookmark_utils.cc&sq=package:chromium&l=188&rcl=1451983004) > > In the worst case, we may need to run one of the 2 searches using > AUTOCOMPLETE_MATCH (will match both uri and title), and then apply a further > filter in javascript for the other search, throwing away some results. Not sure how Spidermonkey JS performs against raw C++ for walking the bookmark tree but we might just do it like they do it.

(In reply to Johann Hofmann [:johannh] from comment #7) > Since this bug is not blocking webextensions in any way I don't take offense > if you just say no thanks and let's wait until async querying is ready. Based on the current Places situation, it's really hard to guess when it will ever get resources to complete the many ongoing projects. I think a temporary solution is the only way to move on, or you may end up waiting too much. > Maybe I'm > misunderstanding the meaning of "general querying", is it supposed to have > any other external functionality than my simple function? :) It was a complete replacement for nsINavHistoryQuery API... But let's try to avoid going OT and concentrate on what we can do *now*. > If I'm reading it correctly they don't do any query but iterate down from > the root node comparing everything using a function aptly named > DoesBookmarkContainWords Oh right, they use JSON for bookmarks... > > In the worst case, we may need to run one of the 2 searches using > > AUTOCOMPLETE_MATCH (will match both uri and title), and then apply a further > > filter in javascript for the other search, throwing away some results. > > Not sure how Spidermonkey JS performs against raw C++ for walking the > bookmark tree but we might just do it like they do it. It's hard to predict, some of our users have thousands of bookmarks due to add-ons tagging like crazy and us storing tags as bookmarks... I still think using AUTOCOMPLETE_MATCH and then filtering out results in js may be the best solution.

(In reply to Marco Bonardo [::mak] from comment #8) > > It was a complete replacement for nsINavHistoryQuery API... But let's try to > avoid going OT and concentrate on what we can do *now*. > I appreciate the realistic approach. :) If there's anything you'd like to change about the API to make it more future-proof, I'd be happy to get it into the patch. > > It's hard to predict, some of our users have thousands of bookmarks due to > add-ons tagging like crazy and us storing tags as bookmarks... I still think > using AUTOCOMPLETE_MATCH and then filtering out results in js may be the > best solution. Ok, let me try that and get back to you :) Thanks!

Comment on attachment 8708837 [details] [diff] [review] Implement chrome.bookmarks.search Sorry Marco, took me a bit longer than expected to get around making the change. :) The updated patch simply changes the LIKE query to use AUTOCOMPLETE_MATCH. That function is, as you mentioned, not intuitive to use. Please take a look if I'm doing anything wrong with it. It apparently matches both URL and title, my tests are also still succeeding. So as far as I can see, there's no need for "manual" filtering using JS. Am I missing something?

(In reply to Johann Hofmann [:johannh] from comment #11) > It apparently matches both URL and title, my tests are also still > succeeding. So as far as I can see, there's no need for "manual" filtering > using JS. Am I missing something? It depends on what the Chrome API is doing. If it is executing an ANDed query, so "string" should appear in both title and url, especially if the 2 strings are different (not sure if it does that also if they are identical), then you can't just use our ORed query. What we do is title = "string" OR url = "string". Even if Chrome executes an OR, how do they handle the case where the 2 strings are different? It really depends on the Chrome API behavior, if we want to keep 100% compatibility. To sum up, the cases to test and compare and for which you likely want unit tests are: query = string title = string, url = string title = string1, url = string2 There are also under-specified cases where the documentation is lacking: query = string, title = string, url = string (should ignore either query or title/url?) query = string1, title = string2, url = string3 (should throw?)

Comment on attachment 8708837 [details] [diff] [review] Implement chrome.bookmarks.search Review of attachment 8708837 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/Bookmarks.jsm @@ +488,5 @@ > + return Task.spawn(function* () { > + let results = yield queryBookmarks(query); > + > + // Remove non-enumerable properties. > + results = results.map(r => Object.assign({}, r)); this is needed by other APIs cause they must query additional data like _id, _parentId, _grandParentId or _childCount... you don't need those and indeed if you remove them from the query you likely can also remove this filtering. @@ +877,5 @@ > + > +function queryBookmarks(info) { > + let queryParams = {tags_folder: PlacesUtils.tagsFolderId}; > + // we're searching for bookmarks, so exclude tags > + let queryString = "WHERE _grandParentId <> :tags_folder"; you can rewrite the query so that you don't need to query _grandParentId, here just use WHERE p.parent <>... The reason is so that you can remove the filtering from above. @@ +887,5 @@ > + > + if (info.url) { > + queryString += " AND h.url = :url"; > + queryParams.url = info.url; > + } Is this really what Chrome is doing? query a title or an url in a case sensitive way? I think it would be dumb. I actually thought if you specify title: "mozilla" it will find all bookmarks having mozilla in the title, not all bookmarks having "mozilla" as title. Please verify. you could run a query using autocomplete_match for a case insensitive match (will match title OR url), and as I said previously you could then filter results based on whether title or url should be matches. ::: toolkit/components/places/tests/bookmarks/test_bookmarks_search.js @@ +1,2 @@ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ nit: tests default to PD, so you can remove the license header if you wish. @@ +19,5 @@ > +function removeBookmarks(bookmarks) { > + return Promise.all(bookmarks.map(function(bm){ > + return PlacesUtils.bookmarks.remove(bm.guid); > + })); > +} If you want to start from a clean point, you could just use yield bookmarks.eraseEverything() @@ +23,5 @@ > +} > + > +add_task(function* search_bookmark() { > + let bm1 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, > + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, nit: type is optional if you specify a url (will default to TYPE_BOOKMARK) @@ +235,5 @@ > +}); > + > +function run_test() { > + run_next_test(); > +} nit: this is not needed anymore, you can complete remove run_test. ::: toolkit/components/places/tests/bookmarks/xpcshell.ini @@ +32,5 @@ > [test_bookmarkstree_cache.js] > [test_bookmarks.js] > [test_bookmarks_eraseEverything.js] > [test_bookmarks_fetch.js] > +[test_bookmarks_search.js] please keep manifest files alphabetically ordered

Thanks for the feedback! > > It depends on what the Chrome API is doing. If it is executing an ANDed > query, so "string" should appear in both title and url, especially if the 2 > strings are different (not sure if it does that also if they are identical), > then you can't just use our ORed query. > > What we do is title = "string" OR url = "string". > Even if Chrome executes an OR, how do they handle the case where the 2 > strings are different? So I ran it in Chrome again and I'm quite sure I got it right. `title`, `url` and `query` are ANDed together. However, `query` searches for any appearance of that string in either url OR title. I think that's exactly what this patch does. It works even if the two strings are different. > > To sum up, the cases to test and compare and for which you likely want unit > tests are: > query = string > title = string, url = string > title = string1, url = string2 > > There are also under-specified cases where the documentation is lacking: > query = string, title = string, url = string (should ignore either query or > title/url?) It will match any item where the title and the url are exactly string, ignoring query. I can add a test for that. > query = string1, title = string2, url = string3 (should throw?) It will match any item where query = string1 and title = string2, ignoring query again. I should also test that, I guess. > > @@ +887,5 @@ > > + > > + if (info.url) { > > + queryString += " AND h.url = :url"; > > + queryParams.url = info.url; > > + } > > Is this really what Chrome is doing? query a title or an url in a case > sensitive way? I think it would be dumb. > I actually thought if you specify title: "mozilla" it will find all > bookmarks having mozilla in the title, not all bookmarks having "mozilla" as > title. Please verify. > Yup, it's a case-sensitive check for exact equality of title and url. It feels kinda dumb, agreed. However, URLs seem to be downcased and sanitized a bit. It seems to apply the same transformation that also happens when you save a bookmark. AFAIK, Firefox also normalizes bookmarks on save. Do we have a utility function we can use here? (See https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmarks/browser/bookmark_utils.cc&sq=package:chromium&l=188&rcl=1451983004, https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmarks/browser/bookmark_utils.cc&sq=package:chromium&l=188&rcl=1451983004 and https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmarks/browser/bookmark_utils.cc&sq=package:chromium&l=97&rcl=1451997057) > you could run a query using autocomplete_match for a case insensitive match > (will match title OR url), and as I said previously you could then filter > results based on whether title or url should be matches. I think ANDing it in the SQL query is most performant and actually produces the least code. I can see that this is not the most flexible implementation of a search function, but it is exactly what Chrome does. :/

yeah, I was mostly worried we were doing it differently than Chrome. I think it's a dumb API honestly, especially matching a title as-is (can't really think of a valid use-case), but let's stick to it, since it's the scope of the bug.

Yeah I tried it several times to be sure, it sounds weird, but the code confirms it. Anyway, I'll fix your other remarks and go over the tests again with your feedback in mind :)

(In reply to Marco Bonardo [::mak] from comment #13) > Comment on attachment 8708837 [details] [diff] [review] > Implement chrome.bookmarks.search > > Review of attachment 8708837 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/places/Bookmarks.jsm > @@ +488,5 @@ > > + return Task.spawn(function* () { > > + let results = yield queryBookmarks(query); > > + > > + // Remove non-enumerable properties. > > + results = results.map(r => Object.assign({}, r)); > > this is needed by other APIs cause they must query additional data like _id, > _parentId, _grandParentId or _childCount... you don't need those and indeed > if you remove them from the query you likely can also remove this filtering. > > @@ +877,5 @@ > > + > > +function queryBookmarks(info) { > > + let queryParams = {tags_folder: PlacesUtils.tagsFolderId}; > > + // we're searching for bookmarks, so exclude tags > > + let queryString = "WHERE _grandParentId <> :tags_folder"; > > you can rewrite the query so that you don't need to query _grandParentId, > here just use WHERE p.parent <>... The reason is so that you can remove the > filtering from above. > I'm running into problems trying to follow your suggestion to remove these fields from the query. The rowsToItemsArray function that I'm using to convert the rows to objects tries to access e.g. _grandParentId and throws an error because it can't be found. (https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#1138). I guess there are multiple ways to solve this. From the top of my head I would either: - Catch the error in rowsToItemsArray - Inline some of the code of rowsToItemsArray where I need it What do you think? :)

(In reply to Johann Hofmann [:johannh] from comment #17) > I'm running into problems trying to follow your suggestion to remove these > fields from the query. The rowsToItemsArray function that I'm using to > convert the rows to objects tries to access e.g. _grandParentId and throws > an error because it can't be found. Ah I see, you are reusing the existing code, though rowsToItemsArray skips them if they return NULL, so one possibility could be to do SELECT ... NULL AS _grandParentId, and you could still remove the filtering code. Or, alternatively, just leave things as they were, included the filtering, but at least do NULL AS _childCount, since it's pointless to pay that subquery price when we don't need it.

Comment on attachment 8714526 [details] [diff] [review] Implement chrome.bookmarks.search So I went with your first suggestion and made the fields NULL. It's easily explained with a comment in the code and should be the most performance-efficient solution. The remaining remarks have also been fixed. Please check if everything is correct with normalizing the URL in ext-bookmarks.js on line 146, I didn't find a better way to do it. Thanks :)

Comment on attachment 8714526 [details] [diff] [review] Implement chrome.bookmarks.search Review of attachment 8714526 [details] [diff] [review]: ----------------------------------------------------------------- from a Places point of view it looks good. Maybe you want to get an additional review from someone who usually reviews webextensions, cause for example I don't know if the lasterror TODO comment you left there is critical or not. ::: browser/components/extensions/ext-bookmarks.js @@ +146,5 @@ > + if (query.url) { > + // convert to a standardized URL > + let url = Cc["@mozilla.org/network/standard-url;1"].createInstance(Ci.nsIStandardURL); > + url.init(Ci.nsIStandardURL.URLTYPE_AUTHORITY, -1, query.url, null, null); > + url.QueryInterface(Ci.nsIURI) Can you use Cu.importGlobalProperties(["URL"]); and just use new URL() on the string? Then if you modify the underlying API you could just pass it through. ::: toolkit/components/places/Bookmarks.jsm @@ +481,5 @@ > + if (query.title && typeof query.title !== "string") { > + throw new Error("Title option must be a string"); > + } > + if (query.url && typeof query.url !== "string") { > + throw new Error("Url option must be a string"); We should also accept a URL object imo, to be coherent with the other APIs. And if we get a string we should still call new URL on it and throw if that fails (and thus it's not a valid url). the scope is to hand off mistakes to the caller as soon as possible to make this less error-prone. @@ +902,5 @@ > + // hence setting them to NULL > + let rows = yield db.executeCached( > + `SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index', > + b.dateAdded, b.lastModified, b.type, b.title, h.url AS url, > + b.id AS _id, b.parent, p.parent, shouldn't this be NULL as _id?

> from a Places point of view it looks good. Maybe you want to get an > additional review from someone who usually reviews webextensions, cause for > example I don't know if the lasterror TODO comment you left there is > critical or not. Yes, actually lastError is TODO in the whole file, there's Bug 1213675 to resolve that. There's some other stuff happening right now though, for example Promise support in Bug 1234020. Kris, would it be okay if I just make a followup bug to convert bookmarks.search to the "promisified" version, since Promise support hasn't landed yet? Also, is it correct that I can just remove the parameter checking code from bookmarks.search, since it will be checked against schemas/bookmarks.json anyway?

(In reply to Johann Hofmann [:johannh] from comment #23) > > from a Places point of view it looks good. Maybe you want to get an > > additional review from someone who usually reviews webextensions, cause for > > example I don't know if the lasterror TODO comment you left there is > > critical or not. > > Yes, actually lastError is TODO in the whole file, there's Bug 1213675 to > resolve that. > > There's some other stuff happening right now though, for example Promise > support in Bug 1234020. Kris, would it be okay if I just make a followup bug > to convert bookmarks.search to the "promisified" version, since Promise > support hasn't landed yet? Basic support has landed. New APIs should return promises rather than accepting callbacks, and reject those promises when lastError needs to be set. > Also, is it correct that I can just remove the parameter checking code from > bookmarks.search, since it will be checked against schemas/bookmarks.json > anyway? Yes, please do.

Comment on attachment 8714526 [details] [diff] [review] Implement chrome.bookmarks.search Review of attachment 8714526 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/extensions/ext-bookmarks.js @@ +146,5 @@ > + if (query.url) { > + // convert to a standardized URL > + let url = Cc["@mozilla.org/network/standard-url;1"].createInstance(Ci.nsIStandardURL); > + url.init(Ci.nsIStandardURL.URLTYPE_AUTHORITY, -1, query.url, null, null); > + url.QueryInterface(Ci.nsIURI) The correct way to create a nsiURI is to use Services.io.newURI. At this point, though, we have URL format checking at the schema level, so you can just add `"format": "url"` to the property and it will be checked at call time. @@ +151,5 @@ > + query.url = url.spec; > + } > + > + Bookmarks.search(query).then(function(result) { > + runSafe(context, callback, result.map(convert)); You should be able to just `return Bookmarks.search(query).then(convert)` here.

Comment on attachment 8714526 [details] [diff] [review] Implement chrome.bookmarks.search Review of attachment 8714526 [details] [diff] [review]: ----------------------------------------------------------------- Oh, also, it would be great if you could add some tests for the failure conditions, especially now that we have support for lastError. I won't require it, though. ::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html @@ +157,5 @@ > + }).then(results => { > + browser.test.assertEq(results.length, 1); > + browser.test.assertEq(results[0].title, "Example"); > + browser.test.assertEq(results[0].url, "http://example.org/"); > + browser.test.assertEq(results[0].index, 2); I know it's strange, but the expected value is supposed to be the first argument to `assertEq`. It gets confusing trying make sense of test failures if you use it as the second argument.

(In reply to Kris Maglione [:kmag] from comment #25) > Comment on attachment 8714526 [details] [diff] [review] > Implement chrome.bookmarks.search > > Review of attachment 8714526 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/extensions/ext-bookmarks.js > @@ +146,5 @@ > > + if (query.url) { > > + // convert to a standardized URL > > + let url = Cc["@mozilla.org/network/standard-url;1"].createInstance(Ci.nsIStandardURL); > > + url.init(Ci.nsIStandardURL.URLTYPE_AUTHORITY, -1, query.url, null, null); > > + url.QueryInterface(Ci.nsIURI) > > The correct way to create a nsiURI is to use Services.io.newURI. At this > point, though, we have URL format checking at the schema level, so you can > just add `"format": "url"` to the property and it will be checked at call > time. > Yes, Marco suggested almost the same thing but also moving it to Bookmarks.jsm, which makes more sense and makes the code look nicer as well. About adding `"format": "url"`: I think it's a good idea, but it breaks compatibility with the Chrome API, since Chrome allows any nonsense as url field in chrome.bookmarks.search. I'm gonna add it anyway, just wanted to mention it. The new Promise API makes the internal code look so much nicer. :)

Comment on attachment 8716762 [details] [diff] [review] Implement chrome.bookmarks.search Alright, this patch implements all the previous comments and is rebased onto the latest m-c. Can you two do a (hopefully) final review? :)

Comment on attachment 8716762 [details] [diff] [review] Implement chrome.bookmarks.search Review of attachment 8716762 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the webextension parts. Thanks

We should make sure to document that we now check the sanity of the URL parameter.

Comment on attachment 8716762 [details] [diff] [review] Implement chrome.bookmarks.search Review of attachment 8716762 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/Bookmarks.jsm @@ +485,5 @@ > + > + if (query.url) { > + if (typeof query.url === "string") { > + query.url = new URL(query.url); > + } else if (!(query.url instanceof Ci.nsIURI)) { what we are passing to the internal queryBookmarks is an URL object (you are using .href out of it), not an nsIURI, so this looks wrong. That said, if we want to accept both URL and nsIURI, it would be fine. What I'd do is passing a string as query.url to queryBookmarks, since it's the only thing it needs: if (typeof query.url === "string" || (query.url instanceof URL)) { query.url = new URL(query.url).href; } else if (query.url instanceof Ci.nsIURI) { query.url = query.url.spec; } else { throw new Error("Url option must be a string or an URL object"); } (note it's fine to new URL an URL object, the reason to URLify a string is to check its validity) Then make queryBookmarks directly bind info.url instead of info.url.href

Comment on attachment 8718303 [details] [diff] [review] Implement chrome.bookmarks.search Thanks for spotting that, I mixed them up. I'm good with your suggestion!

backed this out in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=b9d3adb8692c seems one of this changes caused errors like https://treeherder.mozilla.org/logviewer.html#?job_id=7250321&repo=fx-team

Dammit, ESlint. I'll fix it, sorry for that :)

Comment on attachment 8719570 [details] [diff] [review] Implement chrome.bookmarks.search Carrying over the r+ since I just changed a few indents in the mochitest.

I've been investigating this behavior to try to improve the docs, and here's what I think it is: *** The "query" argument can be either a string or an object. String ------ If it's a string, it consists of zero or more search terms. Search terms are space-delimited. Each search term matches if it matches any substring in the bookmark's URL or title. Matching is case-insensitive. For a bookmark to match the query, all the query's search terms must match. Object ------ If it's an object, it has zero or more of the following 3 properties: "query", "title", "url". * "query" matches in the same way as the string version. * "title" must exactly match the bookmark's title. Matching is case-insensitive. * "url" must exactly match the bookmark's url. Matching is case-insensitive. For a bookmark to match the query, all the query's properties terms must match. *** Is that correct? If it helps, I've attached a little extension I wrote that lets you try out search queries. That's what I used to come up with this.

(In reply to Will Bamberg [:wbamberg] from comment #44) > I've been investigating this behavior to try to improve the docs, and here's > what I think it is: > > *** > > The "query" argument can be either a string or an object. > > String > ------ > If it's a string, it consists of zero or more search terms. Search terms are > space-delimited. Each search term matches if it matches any substring in the > bookmark's URL or title. Matching is case-insensitive. > > For a bookmark to match the query, all the query's search terms must match. Sounds correct. > > Object > ------ > If it's an object, it has zero or more of the following 3 properties: > "query", "title", "url". > > * "query" matches in the same way as the string version. > * "title" must exactly match the bookmark's title. Matching is > case-insensitive. It's actually case-sensitive for title. > * "url" must exactly match the bookmark's url. Matching is case-insensitive. It's case-insensitive and the URL is normalized a little beforehand. So things like trailing slashes shouldn't matter. Not sure if we have to mention that, though. > > For a bookmark to match the query, all the query's properties terms must > match. Correct. > > *** > > Is that correct? If it helps, I've attached a little extension I wrote that > lets you try out search queries. That's what I used to come up with this. I think it sounds good! :)

Thanks! I've updated the doc. As we discussed, it seems like in Chrome you can have a search term that includes spaces by enclosing the term in quotes, and this isn't yet supported in Firefox. I've filed bug 1251657 for this, but documented the behavior we want.

Performed Exploratory testing around this bug on Firefox 48.0a1 (2016-03-28) and noticed that searching by the entire “url” doesn’t return the expected result (reproducible across all platforms: Windows 10 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.11.2). Any thoughts about this?

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #48) > Performed Exploratory testing around this bug on Firefox 48.0a1 (2016-03-28) > and noticed that searching by the entire “url” doesn’t return the expected > result (reproducible across all platforms: Windows 10 64-bit, Ubuntu 14.04 > 32-bit and Mac OS X 10.11.2). > > Any thoughts about this? I'm probably not the right person to ask, as I just wrote the docs for the function. But I just tried it and it worked fine. In the console (Firefox 48.0a1): function log(e){ console.log(e); } chrome.bookmarks.search({url:"https://www.mozilla.org/en-US/about/"}, log); -> Array [ Object ] If this doesn't work, it's probably best to reopen this bug, with a concrete test case including expected and actual results.

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #48) > Performed Exploratory testing around this bug on Firefox 48.0a1 (2016-03-28) > and noticed that searching by the entire “url” doesn’t return the expected > result (reproducible across all platforms: Windows 10 64-bit, Ubuntu 14.04 > 32-bit and Mac OS X 10.11.2). > > Any thoughts about this? I'd love a concrete test case as well. I think this is pretty well covered with tests on several levels and we have a working example in the webextension-examples repo on GitHub, where we also search by url.

I’ve tested using the webextension from Comment 45: - there is no result searching by the entire url, see screenshot: http://i.imgur.com/tPNASxZ.jpg - the expected result is successfully displayed when the protocol (http://) and the sub-domaine (www.) are eliminated from the search keyword, see screenshot: http://i.imgur.com/YwtTczV.jpg

Vasilica, can I suggest that a new bug be opened for this issue of searching by URL? This bug is marked as fixed and I'm not sure it makes sense to reopen it for this issue. Please cc me and johannh on the bug.

OK, yes, I see this too, now, sorry. Only if you pass the URL as a string rather than an object: chrome.bookmarks.search("https://www.mozilla.org/en-US/about/", (t)=>{console.log(t)})

(In reply to Bob Silverberg [:bsilverberg] from comment #52) > Vasilica, can I suggest that a new bug be opened for this issue of searching > by URL? This bug is marked as fixed and I'm not sure it makes sense to > reopen it for this issue. Please cc me and johannh on the bug. Filed 1260743. I am marking this bug as Verified since the other issue is tracked separately.