1126183 - Provide a way to check if a site is part of the top 100 PlacesProvider links

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

PlacesProvider happens to have a list of 100 urls already, so we'll roughly estimate "top sites" from the sites contained in those 100 urls. This will be used to power the logic of whether a related tile should be shown or not.

adw, alternatively we could get closer to the actual top 100 sites by looking at moz_hosts which has frecency calculated, but I think it might be simpler for now to just use the available data from PlacesProvider?

Comment 1

[Drew Willcoxon :adw]

I agree -- but as far as newtab is concerned, PlacesProvider's links are the actual top sites, no?

Comment 2

[Marina Samuel [:emtwo]]

I think what Ed means (correct me if I'm wrong), is that for example, someone may go to http://www.expedia.ca/Hotels, http://www.expedia.ca/Flights and http://www.expedia.ca/Deals a few times, but not enough for them to individually show up in a user's top 100 sites. However, combining these, http://www.expedia.ca may be part of a user's top 100 sites and we can provide a related tile to them. Looking at moz_hosts can help us identify this.

Comment 3

[Marina Samuel [:emtwo]]

Attachment: is-frecent

Comment 4

[Marina Samuel [:emtwo]]

Attachment: v2: Provide a function for checking if a site is frecent.

Couple of small adjustments

Comment 5

[Ed Lee :Mardak]

Comment on attachment 8557945 [details] [diff] [review]
v2: Provide a function for checking if a site is frecent.

>+++ b/toolkit/modules/NewTabUtils.jsm
>         links = links.filter((link) => !!link);
>+        let sites = new Set();
>+        links.forEach((link) => {
nit: the ()s around |link| aren't necessary. Although the previous line does it.. adw?

>+  isFrecentSite: function(aSite) {
>+    return this.isFrecentSiteGivenProvider(aSite, PlacesProvider);
>+  },
Clever! I was about to say doing things from NewTabUtils would result in directory links being treated as top frecent sites.

>+++ b/toolkit/modules/tests/xpcshell/test_NewTabUtils.js
>+add_test(function isFrecentSiteGivenProvider() {
>+  let expectedLinks = makeLinks(0, 10, 2);
Is this doing what you're expecting? I believe this makes links 0, 2, 4, 6, 8, 10, so that's why example1.com is not a frecent sites.

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8557945 [details] [diff] [review]
v2: Provide a function for checking if a site is frecent.

Review of attachment 8557945 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just some nits and a correctness question.

(In reply to Ed Lee :Mardak from comment #5)
> >+++ b/toolkit/modules/NewTabUtils.jsm
> >         links = links.filter((link) => !!link);
> >+        let sites = new Set();
> >+        links.forEach((link) => {
> nit: the ()s around |link| aren't necessary. Although the previous line does
> it.. adw?

Yeah, personally I prefer no parens when they aren't necessary, but I've been accidentally inconsistent.  Whichever is OK with me.

> >+++ b/toolkit/modules/tests/xpcshell/test_NewTabUtils.js
> >+add_test(function isFrecentSiteGivenProvider() {
> >+  let expectedLinks = makeLinks(0, 10, 2);
> Is this doing what you're expecting? I believe this makes links 0, 2, 4, 6,
> 8, 10, so that's why example1.com is not a frecent sites.

You're right, but I think the test is also right?  example1 should not be a frecent site because it's not in the provider's list of links (0, 2, 4, 6, 8, 10).  But example2 is.  And then when example3 is added, example2 is at the end of the list so it gets pushed out.

::: toolkit/modules/NewTabUtils.jsm
@@ +885,2 @@
>          this._providerLinks.set(aProvider, {
> +          siteSet: sites,

Nit: You could do `siteSet: links.reduce(() => {}, new Set())`, similar to linkMap.  Then you wouldn't have to introduce a local variable and you could use reduce(), which is always fun. :-D  (But in seriousness, this is just a style suggestion, feel free to keep what you have.)

@@ +997,5 @@
>        sortedLinks.splice(idx, 0, insertionLink);
>        if (sortedLinks.length > aProvider.maxNumLinks) {
>          let lastLink = sortedLinks.pop();
>          linkMap.delete(lastLink.url);
> +        siteSet.delete(NewTabUtils.extractSite(lastLink.url));

This isn't right, is it?  Because the provider may have two links with the same site.  If one link is removed here, the other one is still present, so the site should remain in siteSet.  So siteSet needs to be a counted set.  I don't think counted sets are actually a thing in JS, so could you please use a Map?  i.e., site => number of links with that site.  Or if you can think of a better way that's fine too.

@@ +1198,5 @@
> +  isFrecentSiteGivenProvider: function(aSite, aProvider) {
> +    return Links._providerLinks.get(aProvider).siteSet.has(aSite);
> +  },
> +
> +  isFrecentSite: function(aSite) {

Hmm, the link objects of all providers have a frecency property, not only PlacesProvider links, so calling this isFrecentSite but using PlacesProvider is kind of a mismatch.  I'd call it isFrecentPlacesSite.

And maybe use "top" instead of "frecent" to be less jargony (for both this method and isFrecentSiteGivenProvider)?  Just an idea.

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js
@@ +10,5 @@
>  function run_test() {
>    run_next_test();
>  }
>  
> +add_test(function isFrecentSiteGivenProvider() {

Please use add_task, and then you don't need run_next_test() at the end.

I don't know why I wrote some of these to use add_test and some add_task...  That's weird.  Would you mind changing them all to use add_task?

Comment 7

[Marina Samuel [:emtwo]]

Attachment: v3: Provide a function for checking if a site is frecent.

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8558798 [details] [diff] [review]
v3: Provide a function for checking if a site is frecent.

Review of attachment 8558798 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the test updates!

::: toolkit/modules/NewTabUtils.jsm
@@ +721,5 @@
>  
>    /**
> +   * A mapping from each provider to an object { sortedLinks, siteMap, linkMap }.
> +   * sortedLinks is the cached, sorted array of links for the provider.
> +   * siteMap is a mapping from base domains to url count associated with the domain.

Nit: url -> URL

@@ +868,5 @@
> +    let previousURLCount = map.get(site);
> +    if (previousURLCount === undefined) {
> +      previousURLCount = 0;
> +    }
> +    map.set(site, previousURLCount + 1);

Style suggestion: map.set(site, (map.get(site) || 0) + 1);

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js
@@ +30,5 @@
> +  // Push out frecency 2 because the maxNumLinks is reached when adding frecency 3
> +  let newLink = makeLink(3);
> +  provider.notifyLinkChanged(newLink);
> +
> +  // There is another frecent url with example4 domain, so it's still frecent.

Not a correctness problem, but I think this part of the test could be made clearer to the person reading it in a couple of ways.  First, give the next-to-last URL the same domain as the last URL -- the one that gets pushed out -- instead of the other way around.  Second, change the comment to say "still a frecent url" instead of "another frecent url".

Comment 9

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/f74d8b95d58a

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f74d8b95d58a

Comment 11

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

This doesn't look like something that would need manual testing, so setting "qe-verify-". Please set "qe-verify+" if you think otherwise.