The default bug view has changed. See this FAQ.

Move bookmark frecency query into `PlacesSyncUtils`

RESOLVED FIXED in Firefox 54

Status

()

Firefox
Sync
P3
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: kitcambridge, Assigned: ahsan.r.kazmi, Mentored)

Tracking

unspecified
Firefox 54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [good first bug][lang=js])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 months ago
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.

Comment 1

2 months ago
I would like to take this as my first bug and try my best to patch it.
(Assignee)

Comment 2

2 months ago
I would also like to work on this.

Comment 3

2 months ago
(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!

Comment 4

2 months ago
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!

Updated

2 months ago
Priority: -- → P3
(Assignee)

Comment 5

2 months ago
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

Updated

2 months ago
Flags: needinfo?(kit)
(Reporter)

Comment 6

2 months ago
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.
Flags: needinfo?(kit)
(Assignee)

Comment 7

2 months ago
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
(Assignee)

Comment 8

2 months ago
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;
  }),
});
(Reporter)

Comment 9

2 months ago
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.
Assignee: nobody → ahsan.r.kazmi
Comment hidden (mozreview-request)
(Reporter)

Comment 11

2 months ago
mozreview-review
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 hidden (mozreview-request)
(Reporter)

Comment 13

2 months ago
mozreview-review
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.
Attachment #8835131 - Flags: review?(kit)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 16

2 months ago
mozreview-review
Comment on attachment 8835695 [details]
Bug 1336282 - Moved bookmark frecency query into `PlacesSyncUtils`.

https://reviewboard.mozilla.org/r/111348/#review112614
Attachment #8835695 - Flags: review?(kit) → review+
(Reporter)

Comment 17

2 months ago
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.
Attachment #8835674 - Attachment is obsolete: true
Attachment #8835674 - Flags: review?(kit)
(Reporter)

Updated

2 months ago
Attachment #8835131 - Attachment is obsolete: true

Comment 18

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

Comment 19

2 months ago
Ok, thank you Kit. Thanks for helping me on my first patch.
(Reporter)

Comment 20

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

Comment 21

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f812edf1194
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.