We moved all the write queries into `PlacesSyncUtils` in bug 1274108, but there's still one read query left: http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/services/sync/modules/engines/bookmarks.js#799-806 Let's move it into a `PlacesSyncUtils` method, like `PlacesSyncUtils.history.fetchURLFrecency(url)`, and change the bookmark engine to use it. (Have a look at http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/toolkit/components/places/PlacesSyncUtils.jsm#98-111 for inspiration). We can then get rid of `_getStringUrlForId`, `_getStmt`, and related code.

I would like to take this as my first bug and try my best to patch it.

I would also like to work on this.

(In reply to Teo Toplak from comment #1) > I would like to take this as my first bug and try my best to patch it. (In reply to ahsan.r.kazmi from comment #2) > I would also like to work on this. It's somewhat unusual to have multiple people offer to work on the same bug in such a short period, so thank you both for your interest! I think it is fair to give Teo a few days to put up a work-in-progress patch - Teo, if you can get a first-cut at a patch up within a few days I'll assign the bug to you - note that this patch doesn't need to be complete, just enough to show solid progress - it will likely take a few iterations of such patches before it is ready. Ahsan, if no such patch is forthcoming within (say) 10 days, then please do the same, and I'll assign the bug to you. Again, thank you both for you interest in helping Firefox stay awesome!

Thank you Mark, but I am going to yield on this one. I would like to give ahsan first opportunity. I'm having big difficulties and bug fix would take significant time. I need to study more about all this working environment. I'm gonna catch another bug later when I'm more confident, have more knowledge and more time. So akshan, you're good to go - good luck with your first bug!

Thanks Teo, and I know what you mean. It is quite difficult to understand all of the code and what it does. Would something like this function work in PlacesSyncUtils.jsm? fetchURLFrecency: Task.async(function* (url) { let db = yield PlacesUtils.promiseDBConnection(); let frecency = yield db.executeCached( `SELECT frecency FROM moz_places WHERE url_hash = hash(:url) AND url = :url LIMIT 1`, { url } ); return frequency; }) And should it be placed in the PlacesSyncUtils.bookmarks object definition in PlacesSyncUtils.jsm? I couldn't find any PlacesSyncUtils.history object. Thank you

Exactly like that, yes. Thanks for taking this bug! * `PlacesSyncUtils.history` doesn't exist yet, so let's add it. `fetchFrecency` will be the first method on that object, and we can add other methods later. * `db.executeCached` returns an array of rows, not just the value. I think we'll want something like `rows[0].getResultByName("frecency")`, and we also need to handle the case where there are no rows because the URL doesn't exist in the database. * Let's normalize the passed URL, so that we can correctly find URLs with trailing slashes, fragments, and so on. `let canonicalURL = PlacesUtils.SYNC_BOOKMARK_VALIDATORS.url(url)` will validate the argument and give us a URL object. We can then pass `canonicalURL.href` to `executeCached`. * Let's also add some tests to http://searchfox.org/mozilla-central/rev/848c29538ab007fb95dc6cff194f0e3d3809613d/toolkit/components/places/tests/unit/test_sync_utils.js. We can use `PlacesTestUtils.addVisits` (http://searchfox.org/mozilla-central/rev/848c29538ab007fb95dc6cff194f0e3d3809613d/toolkit/components/places/tests/PlacesTestUtils.jsm#21-80) to add visits, check that we can retrieve their frecencies, and check that trying to fetch the frecency of a nonexistent URL returns a sensible value like -1.

Hello Kit, Thanks for responding and for the help. I put the function inside a PlacesSyncUtils.history object and also fixed some of the other things you mentioned. However, I couldn't get let canonicalURL = PlacesUtils.SYNC_BOOKMARK_VALIDATORS.url(url) to work when running the tests. I couldn't find a SYNC_BOOKMARK_VALIDATORS.url() function in PlacesUtils.jsm. Should I write this function as well? Here is what I have for the fetchURLFrecency function in PlacesSyncUtils.jsm so far const HistorySyncUtils = PlacesSyncUtils.history = Object.freeze({ fetchURLFrecency: Task.async(function* (url) { let db = yield PlacesUtils.promiseDBConnection(); let rows = yield db.executeCached( `SELECT frecency FROM moz_places WHERE url_hash = hash(:url) AND url = :url LIMIT 1`, { url } ); return rows.length ? rows[0].getResultByName("frecency") : null; }), }); Here is a basic test I wrote in /toolkit/components/places/tests/unit/test_sync_utils.js add_task(function* test_fetchURLFrecency() { yield PlacesTestUtils.addVisits("https://www.mozilla.org/en-US/"); let frecency = yield PlacesSyncUtils.history.fetchURLFrecency("https://www.mozilla.org/en-US/"); notEqual(frecency, null); frecency = yield PlacesSyncUtils.history.fetchURLFrecency("https://google.com/"); equal(frecency, null); }); All the tests seem to pass when I run ./mach xpcshell-test ./toolkit/components/places/tests/unit/test_sync_utils.js I am sorry if any of this wrong. This is all pretty new to me as this is my first patch. Additionally, is this enough to make a patch after I make the appropriate changes in bookmarks.js (i.e. call the fetchURLFrecency function from there and take out the now unneeded code)? Thank you, Ahsan

Sorry about that. I found the PlacesUtils.SYNC_BOOKMARK_VALIDATORS.url function. I didn't search for it correctly in dxr the first time. I was making an error when passing canonicalURL.href into db.executeCached the first time. It now passes the tests. const HistorySyncUtils = PlacesSyncUtils.history = Object.freeze({ fetchURLFrecency: Task.async(function* (url) { let canonicalURL = PlacesUtils.BOOKMARK_VALIDATORS.url(url); let URL = canonicalURL.href; let db = yield PlacesUtils.promiseDBConnection(); let rows = yield db.executeCached( `SELECT frecency FROM moz_places WHERE url_hash = hash(:URL) AND url = :URL LIMIT 1`, { URL } ); return rows.length ? rows[0].getResultByName("frecency") : null; }), });

Awesome. That looks great, Ahsan; thanks again for contributing! I'll go ahead and assign this bug to you, since you're a few steps away from a patch. This is definitely enough to make a patch, and patches are a lot easier to review than Bugzilla comments! :-) Do you already have MozReview set up? (https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html) Most of the Firefox team uses it for patches, and you can now use it without having to request commit access. If not, no worries; you're welcome to upload one using Bugzilla.

Comment on attachment 8835131 [details] Bug 1336282 - Moved bookmark frecency query into `PlacesSyncUtils`. https://reviewboard.mozilla.org/r/110822/#review112184 Excellent work, Ahsan! Just a few nits, and this should be ready to land. \o/ Please amend the commit with the changes, and add "r?kitcambridge" to the commit message. That way, it'll show up in my review queue. ::: services/sync/modules/engines/bookmarks.js:759 (Diff revision 1) > record.sortindex = this._calculateIndex(record); > > return record; > }, > > _stmts: {}, Please remove `_stmts` and the `"places-shutdown"` observer in `BookmarksStore`, too. ::: services/sync/modules/engines/bookmarks.js:787 (Diff revision 1) > if (record.parentid == "toolbar") > index += 150; > > // Add in the bookmark's frecency if we have something. > if (record.bmkUri != null) { > - this._frecencyStm.params.url = record.bmkUri; > + let result = Async.promiseSpinningly(PlacesSyncUtils.history.fetchURLFrecency(record.bmkUri)); Let's call this `frecency` instead of `result`... ::: services/sync/modules/engines/bookmarks.js:789 (Diff revision 1) > // Add in the bookmark's frecency if we have something. > if (record.bmkUri != null) { > - this._frecencyStm.params.url = record.bmkUri; > - let result = Async.querySpinningly(this._frecencyStm, this._frecencyCols); > + let result = Async.promiseSpinningly(PlacesSyncUtils.history.fetchURLFrecency(record.bmkUri)); > + if (result != null) > - if (result.length) > index += result[0].frecency; And this should just be `index += frecency`. ::: toolkit/components/places/PlacesSyncUtils.jsm:67 (Diff revision 1) > + let canonicalURL = PlacesUtils.SYNC_BOOKMARK_VALIDATORS.url(url); > + let URL = canonicalURL.href; > + > + let db = yield PlacesUtils.promiseDBConnection(); > + let rows = yield db.executeCached( > + `SELECT frecency Nit: Two spaces for indentation, not four. ::: toolkit/components/places/PlacesSyncUtils.jsm:71 (Diff revision 1) > + let rows = yield db.executeCached( > + `SELECT frecency > + FROM moz_places > + WHERE url_hash = hash(:URL) AND url = :URL > + LIMIT 1`, > + { URL } Nit: Let's lowercase this param name (`:url`), remove the `URL` variable, and pass the param as `{ url: canonicalURL.href }`. ::: toolkit/components/places/PlacesSyncUtils.jsm:73 (Diff revision 1) > + FROM moz_places > + WHERE url_hash = hash(:URL) AND url = :URL > + LIMIT 1`, > + { URL } > + ); > + return rows.length ? rows[0].getResultByName("frecency") : null; Let's return -1 instead of `null`. `null` coerces to 0 in JavaScript; I find it a bit cleaner for methods to return the same type for primitives. ::: toolkit/components/places/tests/unit/test_sync_utils.js:180 (Diff revision 1) > + // Add visits to the following URLs and then check if frecency for those URLs is not null. > + let arrayOfURLsToVisit = ["https://www.mozilla.org/en-US/", "http://getfirefox.com", "http://getthunderbird.com"]; > + let n = arrayOfURLsToVisit.length; > + for (let i = 0; i < n ; i++){ > + let url = PlacesUtils.SYNC_BOOKMARK_VALIDATORS.url(arrayOfURLsToVisit[i]); > + yield PlacesTestUtils.addVisits(url.href); Nit: `for (let url of arrayOfURLsToVisit)`. Also, no need to call the validator method here; the call in `fetchURLFrecency` should be enough. ::: toolkit/components/places/tests/unit/test_sync_utils.js:190 (Diff revision 1) > + notEqual(frecency, null); > + } > + // Do not add visits to the following URLs, and then check if frecency for those URLs is null. > + let arrayOfURLsNotVisited = ["https://bugzilla.org", "https://example.org"]; > + n = arrayOfURLsNotVisited.length; > + for (let i = 0; i < n ; i++){ `for (let url of arrayOfURLsNotVisited)` here, too.

Comment on attachment 8835131 [details] Bug 1336282 - Moved bookmark frecency query into `PlacesSyncUtils`. https://reviewboard.mozilla.org/r/110822/#review112294 I pushed your patch to Try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=30606127990209227af0932cd7a1ff42e43e60a6 Looks great, except for a couple of ESLint failures. Fix those up, and then we can land this.

Comment on attachment 8835695 [details] Bug 1336282 - Moved bookmark frecency query into `PlacesSyncUtils`. https://reviewboard.mozilla.org/r/111348/#review112614

Comment on attachment 8835674 [details] Bug 1336282 - Moved bookmark frecency query into `PlacesSyncUtils`. Great work, let's land this! I squashed your commits and re-pushed.

Pushed by kcambridge@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3f812edf1194 Moved bookmark frecency query into `PlacesSyncUtils`. r=kitcambridge

Ok, thank you Kit. Thanks for helping me on my first patch.

Thank you for contributing! Your patch should make it in to tomorrow's Nightly. :-)