I have four livemarks, A, B, C and D. I construct a query like so: place:&annotation=livemark/bookmarkFeedURI&folder=A&folder=B&folder=D&sort=1&maxResults=20 I expect to see the contents of A, B & C, sorted by title, capped at 20 results. What I see instead are the contents of D, sorted by title, capped at 20 results!

need to write a collapsed test to see if this is still valid.

writing the test could be a good first bug, if additional issues should come up we could address them after the test.

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

mak, is this a bug that you might sign up to mentor? Or can you suggest another mentor? I notice it's been untouched for years. Maybe making it a mentored bug would encourage someone to try it out.

sure, no problem.

i would love to work on this work mak, please assign it to me

is this bug still open, i want to work on it as my first bug ,need help :)

hi, the scope here is to write a test that: - adds 3 folders and a bookmark into each of them - queries multiple folders, for example: place:folder=A&folder=B&folder=C&sort=1&maxResults=20 - check what is returned (based on that we'll take a direction) the test should be an xpcshell-test (https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests) added to the /toolkit/components/places/tests/queries folder. While not extremely up-to-date, this documentation should help: https://developer.mozilla.org/en-US/docs/Querying_Places There are lots of tests in toolkit/components/places/tests/ you can use for inspiration, even if I suggest looking just at the ones written in the last 6-12 months (to avoid using old code paths)

Hi, Karthik - Are you still working on this?

hey Mike, I'm stuck up with exams, you can work on it

Can I work on this?

Hi, Akshendra - You certainly can. Because we generally assign bugs to new contributors after we receive their first patch, the thing to do now is get your development environment set up. There's some good documentation here: https://developer.mozilla.org/en-US/docs/Introduction Thanks, and good luck! Please let me know if you've got any more questions.

Comment 8 should have everything needed to start here. The test should go into toolkit/components/places/tests/queries/

I am trying to use queryStringtoQueries in > let query = {}; > let options = {}; > let queryCount = {}; > htsvc.queryStringToQueries("place:folder=" + folderA + "&folder=" + folderB, query, queryCount, > options); > let result = htsvc.executeQuery(query.value[0],options.value); with folderA , folderB the results createFolder() of bookmarkServices. It works if I use single folder but with multiple folders the test crashes. Any help? This might be the relevant part of log. > ASSERTION: The statement 'SELECT h.id, h.url, h.title AS page_title, h.rev_host, h.visit_count, > h.last_visit_date, f.url, null, null, null, null, null AS tags , h.frecency, h.hidden, h.guid FROM > moz_places h LEFT JOIN moz_favicons f ON h.favicon_id = f.id WHERE 1 AND hidden = 0 AND > last_visit_date NOTNULL AND (( b.parent IN( 6 , 7 ) )) ORDER BY h.id ASC ' failed to compile > with the error message 'no such column: b.parent'.: 'Error', file ../../../dist/include/mozilla > /storage/StatementCache.h, line 124

Looks like you hit a bug already! that query cannot work cause b.parent is not selected (should be instead of those NULLs). Off-hand this seems to be due to taking the wrong branch in this switch: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#1479 that basically means mQueryType is nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY... while it should probably be nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS I think a possible way to fix this, might be to change the query type every time SetFolders is invoked on the query object http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistoryQuery.cpp#1295 here should basically do mQueryType = QUERY_TYPE_BOOKMARKS; Let me know if you are not comfortable with cpp code, you can just pick something else from the mentored bugs list. Once that is done, you should try again and see if we are now taking the right path and executing a meaningful query. Note that additionally from your method to build a query, you should also test the alternative API method: let query = PlacesUtils.history.getNewQuery(); let options = PlacesUtils.history.getNewQueryOptions(); query.setFolders([2, 3], 2); // just an example let root = PlacesUtils.history.executeQuery(query, options).root;

I did the changes in the cpp file, that made it work. The results for both the cases are same ie > 0:16.46 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://abookmarkc1.com/ - true == true > 0:16.46 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://abookmarkc2.com/ - true == true > 0:16.47 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://abookmarkc3.com/ - true == true > 0:16.47 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://abookmarkc4.com/ - true == true > 0:16.48 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://abookmarkc5.com/ - true == true > 0:16.48 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://abookmarkc6.com/ - true == true > 0:16.49 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://kbookmarka1.com/ - true == true > 0:16.49 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://kbookmarka2.com/ - true == true > 0:16.50 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://kbookmarka3.com/ - true == true > 0:16.50 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://kbookmarka4.com/ - true == true > 0:16.51 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://kbookmarka5.com/ - true == true > 0:16.51 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://kbookmarka6.com/ - true == true > 0:16.52 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://kbookmarka7.com/ - true == true > 0:16.52 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://wbookmarkb1.com/ - true == true > 0:16.52 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://wbookmarkb2.com/ - true == true > 0:16.52 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://wbookmarkb3.com/ - true == true > 0:16.53 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://wbookmarkb4.com/ - true == true > 0:16.53 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://wbookmarkb5.com/ - true == true > 0:16.54 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://wbookmarkb6.com/ - true == true > 0:16.54 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://wbookmarkb7.com/ - true == true Seems like it selects the 20 results first and then sorts them according to the uri?

Comment on attachment 8518074 [details] [diff] [review] 334050_queriesAgainstMultipleFolder.patch Review of attachment 8518074 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/nsNavHistoryQuery.cpp @@ +854,2 @@ > query->SetFolders(folders.Elements(), folders.Length()); > + aOptions->SetQueryType(nsNavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS); this fixes parsing a query string into queries, but I think it would not still work if you just setFolders using newQuery and newQueryOptions. Indeed you had to specify options.queryType = 1; to make that work I think you can write a more general fix by enforcing the query type in nsNavHistory::QueryToSelectClause where we check folders.length > 0 http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#3379 then you should not need anymore to specify options.queryType = 1; (FWIW, you should have used Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS and not a raw value) ::: toolkit/components/places/tests/queries/test_queryMultipleFolder.js @@ +3,5 @@ > + > +"use strict"; > + > +let bmsvc = Components.classes["@mozilla.org/browser/nav-bookmarks-service;1"] > + .getService(Components.interfaces.nsINavBookmarksService); Please use PlacesUtils.bookmarks.(method) accessor everywhere instead @@ +10,5 @@ > + run_next_test(); > +} > + > +add_task(function* test_multiplePlaces() { > + let menuFolder = bmsvc.bookmarksMenuFolder; here you should use PlacesUtils.bookmarksMenuFolderId. that's because going through the service you are crossing xpcom boundaries (javascript to cpp) and that has some overhead (minor, but worth to avoid it). @@ +25,5 @@ > + > + // adding bookmarks in the folder B > + for(let i = 1; i <= 7; ++i) { > + bmsvc.insertBookmark(folderB, uri("http://WBookmarkB" + i + ".com"), bmsvc.DEFAULT_INDEX, ""); > + } I think using nested loops would make this code simpler (instead of A,B,C you could just use a numeric index), you can push the ids to an array to keep them around @@ +38,5 @@ > + let query = {}; > + let options = {}; > + PlacesUtils.history.queryStringToQueries("place:folder=" + folderA + > + "&folder=" + folderB + > + "&folder=" + folderC + supposing you have ids in an array, you can ids.map(id => "folder=" + id).join("&") to get a string like "folder=1&folder=2&..." @@ +41,5 @@ > + "&folder=" + folderB + > + "&folder=" + folderC + > + "&sort=1&maxResults=20", > + query, {}, options); > + options.queryType = 1; you should not need to set this. @@ +44,5 @@ > + query, {}, options); > + options.queryType = 1; > + let rootNode = PlacesUtils.history.executeQuery(query.value[0],options.value).root; > + rootNode.containerOpen = true; > + for (let i=0; i<rootNode.childCount; ++i) { per coding style please add whitespaces in "i = 0" and "i < rootNode..." @@ +46,5 @@ > + let rootNode = PlacesUtils.history.executeQuery(query.value[0],options.value).root; > + rootNode.containerOpen = true; > + for (let i=0; i<rootNode.childCount; ++i) { > + let node = rootNode.getChild(i); > + Assert.ok(true, node.uri); this is ok to print out the uris, but we'll actually need to test the returned order makes sense according to the sorting we defined sort=1 as you defined is SORT_BY_TITLE_ASCENDING http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#987 but you haven't defined any title. thus I think sorting fallbacked to URI cause titles are identical.. I think you should use SORT_BY_URI_ASCENDING directly (sort=5) the results look correct afaict (should be sorted by uri and then cut at 20) @@ +49,5 @@ > + let node = rootNode.getChild(i); > + Assert.ok(true, node.uri); > + } > + rootNode.containerOpen = false; > + } catch(ex) { you don't need to try/catch, if the test throws it will fail automatically. @@ +54,5 @@ > + Assert.ok(false, ex.message); > + } > + > + // using the query search parameters and the query configuration options > + try { try is not needed... though now strict mode will complain you are redefining vars... so pay attention to that @@ +57,5 @@ > + // using the query search parameters and the query configuration options > + try { > + let query = PlacesUtils.history.getNewQuery(); > + let options = PlacesUtils.history.getNewQueryOptions(); > + options.queryType = 1; should not be needed @@ +59,5 @@ > + let query = PlacesUtils.history.getNewQuery(); > + let options = PlacesUtils.history.getNewQueryOptions(); > + options.queryType = 1; > + query.setFolders([folderA, folderB, folderC], 3); > + options.sortingMode = 1; please use SORT_BY_URI_ASCENDING constant from the interface

mak: May you assign that to me please!

> this fixes parsing a query string into queries, but I think it would not > still work if you just setFolders using newQuery and newQueryOptions. > Indeed you had to specify options.queryType = 1; to make that work Isn't this the idea that when one uses newQuery and newQueryOptions, they have to specify all options by hand?

(In reply to Akshendra Pratap Singh from comment #19) > > this fixes parsing a query string into queries, but I think it would not > > still work if you just setFolders using newQuery and newQueryOptions. > > Indeed you had to specify options.queryType = 1; to make that work > > Isn't this the idea that when one uses newQuery and newQueryOptions, they > have to specify all options by hand? some options can be optional, in this case since you set Folders it's pretty clear we support only a bookmarks query. defining it regardless shouldn't hurt, but it should not be mandatory in this case.

Comment on attachment 8520568 [details] [diff] [review] 334050_queriesAgainstMultipleFolder.patch Review of attachment 8520568 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/tests/queries/test_queryMultipleFolder.js @@ +6,5 @@ > +function run_test() { > + run_next_test(); > +} > + > +add_task(function* test_queryMultiplePlaces() { s/test_queryMultiplePlaces/queryMultipleFolders/ @@ +9,5 @@ > + > +add_task(function* test_queryMultiplePlaces() { > + // adding bookmarks in the folders > + var folderIds = []; > + var bookmarkIds = []; please use "let" everywhere @@ +29,5 @@ > + }).join('&') + "&sort=5&maxResults=20"; > + PlacesUtils.history.queryStringToQueries(queryString, query, {}, options); > + let rootNode = PlacesUtils.history.executeQuery(query.value[0],options.value).root; > + rootNode.containerOpen = true; > + for (let i = 0; i < rootNode.childCount; ++i) { please also check childCount equals maxResults (20) @@ +43,5 @@ > + options.sortingMode = options.SORT_BY_URI_ASCENDING; > + options.maxResults = 20; > + rootNode = PlacesUtils.history.executeQuery(query, options).root; > + rootNode.containerOpen = true; > + for (let i=0; i<rootNode.childCount; ++i) { ditto plus, please add whitespaces in i=0 and i<rootNode...

Comment on attachment 8521412 [details] [diff] [review] 334050_queriesAgainstMultipleFolder.patch Review of attachment 8521412 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, do you need help to push to the Try server?

Yes I do not have access to that.

could you please change the commit message to Bug 334050 - Add a test for Places queries against multiple folders. r=mak thank you.

Comment on attachment 8521474 [details] [diff] [review] 334050_queriesAgainstMultipleFolder.patch Review of attachment 8521474 [details] [diff] [review]: ----------------------------------------------------------------- no need for further reviews.