1126182 - Extract related tiles data from links json and store for later selection

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

The json payload from the server will have tiles with "type": "related" as well as "related": ["array.com", "of.org", "sites.net"]

We'll want to extract them in a way that they don't show up as directory tiles and persist the data so we can do decisioning on which related tile to show.

Comment 1

[Marina Samuel [:emtwo]]

If we want to store the related tiles in a way that we can access them separately, why not have the server send directoryLinks.json with a format like:

{
"related": "en-US": [...],
"directory": "en-US": [...]
}

instead of having to iterate through directoryTiles.json, look for related tiles, then store them separately? Would this be a big change? Also, by persisting the data, do you mean as a separate json file also in the profile directory? (e.g. relatedTiles.json?)

Comment 2

[Ed Lee :Mardak]

(In reply to Marina Samuel [:emtwo] from comment #1)
> If we want to store the related tiles in a way that we can access them
> separately, why not have the server send directoryLinks.json with a format
> like:
> 
> {
> "related": "en-US": [...],
> "directory": "en-US": [...]
> }
That's a fair point and would affect what we need to do for bug 1126191 as well.

I believe I was trying to keep the v2/links/fetch endpoint usable in a backwards compatible way. Where current v2 responds with { "en-US": [tiles] }. But I'm not sure if backwards compatibility is completely desirable anyway. What would older clients do with related tiles it didn't know about? Probably show them as if they were directory tiles?

So assuming we do want a v3, do we need to keep the extra locale information around in the response? I believe it was originally there so that the server could theoretically provide multiple locales of tiles, but any given Firefox installation will have a single locale anyway?

So just a response of:
{
  "related": [tiles..],
  "directory": [tiles..]
}

?

> persisting the data, do you mean as a separate json file also in the profile
> directory? (e.g. relatedTiles.json?)
Nah, we have the response from the server on disk already, and reparsing the json should be okay for now. I meant keep it in memory so that we can pick out a related tile to show. Although technically, if we don't change which related tile we show right now, we only really need to pick one and save that one for when its information is needed in rendering.

Comment 3

[Marina Samuel [:emtwo]]

Attachment: v1: Cache related tiles for easy lookup by site

Comment 4

[Ed Lee :Mardak]

Do you have an idea of how you'll implement bug 1126184 to select the related tile? I ask because currently, DirectoryLinksProvider_getLinks() is the place to let NewTabUtils know about the selected related tile.

Stashing the _relatedTiles with link data probably will help us make better decisions and be able to show appropriate information in the UI.

Comment 5

[Marina Samuel [:emtwo]]

Yes, I was thinking since we're reading the file in getLinks() already, it's a good time to store _relatedTiles here.

We will also need to provide notifications to DirectoryLinksProvider when the frecency of a history tile (or many history tiles) changes so that we can adjust the related tile accordingly. Both on these notifications and in DirectoryLinksProvider.getLinks() we will do decisioning for which related tile to show, then notify NewTabUtils that a link has changed.

As you suggested earlier, Ed, we can initially choose a related tile at random. We can create a list of all potential related tiles given a user's frecent tiles then choose one at random from it.

Comment 6

[Ed Lee :Mardak]

(In reply to Marina Samuel [:emtwo] from comment #5)
> We will also need to provide notifications to DirectoryLinksProvider when
> the frecency of a history tile (or many history tiles) changes
Hmm.. That does make sense, although it seems like we would need to call PlacesUtils.history.addObserver just like what PlacesProvider does except for DirectoryLinksProvider. I'm not sure if there's a better way though.

> Both on these notifications and in DirectoryLinksProvider.getLinks() 
> we will do decisioning for which related tile to show
Seems like we'll probably want a helper function that returns a link for getLinks to prepend or for the notification to trigger potentially 2 onLinkChanged (one to add and one to remove). In either case, with the helper function, we'll indeed want to stash away related links info as you've done.

Comment 7

[Marina Samuel [:emtwo]]

(In reply to Ed Lee :Mardak from comment #6)
> Hmm.. That does make sense, although it seems like we would need to call
> PlacesUtils.history.addObserver just like what PlacesProvider does except
> for DirectoryLinksProvider. I'm not sure if there's a better way though.

Mentioned in IRC but adding the comment here: Perhaps we can use Services.obs to send the notification instead of listening directly to frecency changes and also delay decisioning to after siteMap is updated.

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8559369 [details] [diff] [review]
v1: Cache related tiles for easy lookup by site

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

Looks fine to me.

::: browser/modules/DirectoryLinksProvider.jsm
@@ +86,5 @@
>  
> +  /**
> +   * A mapping from site to a list of related link objects
> +   */
> +  _relatedTiles: new Map(),

_relatedLinks

@@ +191,5 @@
>        Services.prefs.removeObserver(prefName, this);
>      }
>    },
>  
> +  _cacheRelatedTiles: function(link) {

Links

@@ +193,5 @@
>    },
>  
> +  _cacheRelatedTiles: function(link) {
> +    for (let relatedSite of link.related) {
> +      let relatedArray = this._relatedTiles.get(relatedSite) || [];

Are you sure that this doesn't want to be a Set instead of an Array?

@@ +421,2 @@
>  
> +        // Related tiles will have a higher frecency.

I don't understand what this comment means.  The link's frecency isn't set at all in this case.  Is it in the JSON?  Where's it set?  And why skip adding the link to the list?  Please make the comment clearer.

Comment 9

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #8)
> > +      let relatedArray = this._relatedTiles.get(relatedSite) || [];
> Are you sure that this doesn't want to be a Set instead of an Array?
Good question that got me thinking of a related question. Do we expect the array of related sites (from the server) to have meaningful ordering? E.g., [irs.gov, taxact.com] vs [taxact.com, irs.gov]? The patch + review right now just splits up the array as keys for the _relatedLinks map to keep track of which link could be shown if the user's top sites matches (optimizing for looking up by user's sites).

To adw's question, does the access pattern matter affect it being a Set vs array? E.g., if for now we want to pick a random Link that matches a user's top site, it being a Set would require us to convert it to an Iterator/array to pick something, no? The main benefit of the Set would be if we wanted to check if a Link was in the set, and I'm not sure if we would do that..?

Comment 10

[Drew Willcoxon :adw]

If I could offer my opinion, I think of it in kind of a different way, not how am I going to use it, but what is the data?  If the data is ordered or can contain duplicates, then it's a list/array.  Otherwise, it's a set -- even if I use a JS array to store it.  If it were me I wouldn't really care about whether I need to convert a set to an array just so I could, for example, easily pick out a random item from it.  (Although Sets are iterable, so I don't think that particular example is a good reason to use an array anyway.)

Comment 11

[Ed Lee :Mardak]

Fair enough. :) The "using it" aspect is more of an optimization, but that's probably premature and probably nowhere close to being a bottleneck.

Comment 12

[Marina Samuel [:emtwo]]

Attachment: v2: Cache related tiles for easy lookup by site

I just wanted to confirm this change makes sense to both of you since there was some back and forth on it.

I changed from an array to a Map. I believe a related tile in our list should be unique, although I think that ensuring directoryLinks.json has unique related links would already be done by the server. Also, I used a map because I think URL is sufficient enough to uniquely identify a link (otherwise it looks like we'd need a toString() function for links so Set can identify uniques)

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 8560541 [details] [diff] [review]
v2: Cache related tiles for easy lookup by site

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

Good point!  I didn't think that through, sorry.  Please don't let me stop you from using an array then.  No reason to jump through hoops and use a Map if you don't really need it. :-)  r+ either way.

Comment 14

[Marina Samuel [:emtwo]]

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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22dde7d7f911

Comment 16

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

This does not seem to be a high impact fix, and seems to be covered in browser/modules/test/xpcshell/test_DirectoryLinksProvider.js.

Marking as qe-verify-.

Comment 17

[Ed Lee :Mardak]

https://hg.mozilla.org/releases/mozilla-beta/rev/83bcf11d00ef