1126184 - Select a single related tile to show as the first unpinned tile

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

Using the top sites check from bug 1126183 and related tiles data from bug 1126182, we can determine which of them should be shown.

It's not entirely clear at this point what should be done if multiple related tiles match the user's browsing behavior.

The simplest approach would be to take the first related tile that matches the user's top sites (this assumes the server provides more important related tiles first).

A potentially more user focused approach would be to find a related tile that matches a site highest on the user's top sites.

Comment 1

[Ed Lee :Mardak]

adw, I don't expect which related tile we end up showing to need to change very frequently, but if we were to randomly pick 1 of multiple potential valid related tiles after several new tab opens, it seems like we would need to trigger a onManyLinksChanged (as the previous related tile is being removed and a new one being inserted to the front). Would this the right approach?

Comment 2

[Drew Willcoxon :adw]

Yeah, I think so.  Right now there's no finer-grained way for a provider to tell NewTabUtils that a single link has been replaced with another.  As an optimization maybe you could add a way, but it might not be worth the trouble.

Comment 3

[Ed Lee :Mardak]

adw, some discussion from bug 1126182 comment 6:

Seems like we might duplicate observe PlacesUtils.history from DirectoryLinksProvider to be aware of a new link being added to potentially trigger a new related tile to show. But this is somewhat complicated in that we only really should be updating the related tile if there's a new top site (as opposed to new link).

Alternatively, maybe we could just watch for changes to the SiteMap being added in bug 1126183? addTopPlacesSiteObserver?

(In reply to Drew Willcoxon :adw from comment #2)
> Right now there's no finer-grained way for a provider to
> tell NewTabUtils that a single link has been replaced with another.
Lacking the finer-grained way, would be it okay to just call onManyLinksChanged? It seems that onLinkChanged doesn't have a direct way to remove a link other than pushing it off the end of the list. Although it seems that DirectoryLinksProvider doesn't even set a maxNumLinks.. ??

Comment 4

[Drew Willcoxon :adw]

I don't think I completely understand the related links idea, but could you just have DirectoryLinksProvider observe PlacesProvider?  Expose PlacesProvider directly on NewTabUtils if you need to, no problem.  It should really be in its own module anyway.

(In reply to Ed Lee :Mardak from comment #3)
> Lacking the finer-grained way, would be it okay to just call
> onManyLinksChanged?

Sure.

> Although it seems that DirectoryLinksProvider doesn't even set a
> maxNumLinks.. ??

Yikes, that should be fixed.  I think it hasn't been a problem because DirectoryLinksProvider never fires onLinkChanged, right?  Will it?  onLinkChanged is the only places that uses maxNumLinks.

Comment 5

[Marina Samuel [:emtwo]]

Attachment: Part1: v1: Notify DirectoryLinksProvider of siteMap updates and update _topSitesWithRelatedLinks.

This patch sends notifications to DirectoryLinksProvider when PlacesProvider updates its siteMap. This notification is so DirectoryLinksProvider knows that it's time to check if any of these new top sites happen to have a related tile for them.

Once DirectoryLinksProvider receives this notification, it updates a set of top sites that we have a related tile for (_topSitesWithRelatedLinks).

The next patch will use _topSitesWithRelatedLinks to choose a related tile.

Comment 6

[Marina Samuel [:emtwo]]

Attachment: Part1: v1: Notify DirectoryLinksProvider of siteMap updates and update _topSitesWithRelatedLinks.

Replacing with correct patch.

Comment 7

[Drew Willcoxon :adw]

Can't you just have DirectoryLinksProvider observe PlacesProvider?

Comment 8

[Marina Samuel [:emtwo]]

Drew, could you elaborate on that please? Do you mean in the same way Links observes the providers?

Comment 9

[Drew Willcoxon :adw]

Right, you would need to expose PlacesProvider on NewTabUtils but that's perfectly fine IMO.  That way you don't have to make a new API or resort to global notifications.  I guess since you need the site and not the URL, your DirectoryLinksProvider observer would need to call extractSite or whatever it's called on the changed links.

Comment 10

[Drew Willcoxon :adw]

But if that doesn't work for some reason let me know!

Comment 11

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #9)
> I guess since you need the site and not the URL, your
> DirectoryLinksProvider observer would need to call extractSite or whatever
> it's called on the changed links.
There is indeed a problem of PlacesProvider giving links instead of sites, but there's a deeper problem of PlacesProvider doesn't have a concept of "top 100" outside of its getLinks where it passes in 100 as a history query options.maxResults.

Links is what enforces the "top 100" for PlacesProvider when onLinkChanged is called. That's why emtwo wrote the patch to send a notification from onLinkChanged -> increment/decrementSiteMap.

Comment 12

[Marina Samuel [:emtwo]]

If I understand correctly, we're talking about notifying DirectoryLinksProvider with "onManyLinksChanged" and "onLinkChanged" sent from PlacesProvider.

As Ed mentions, 'onLinkChanged' is where the logic takes place looking at maxNumLinks. There's also a bunch of other logic here deciding how to update the cached links (including for siteMap). Ultimately, once we update siteMap, if there's a new site, we want to check if it has a related link.

If we keep _decrementSiteMap and _incrementSiteMap calls where they are now, when PlacesProvider notifies DirectoryLinksProvider of 'onLinkChanged', it may or may not happen after the increment/decrement occurs so we can't be certain we get to update our related link (same goes for 'onManyLinksChanged')

Alternatively, we could move logic for when to increment/decrement to be triggered once DirectoryLinksProvider is notified of 'onLinkChanged' so we can ensure this happens before we update the related link, but then we may end up duplicating some logic.

I think it's possible to have DirectoryLinksProvider observe PlacesProvider if we added another notification, e.g. 'onSiteMapChanged' that would be sent in the same spots I'm sending site-map-updated in this patch.

Comment 13

[Drew Willcoxon :adw]

OK, I think I understand.  Thanks for patiently explaining.

I still don't like using the global notification service and effectively adding a new type of observer (SiteMapUpdateType) though, and having Links send notifications on behalf of providers.  It all feels like it's going against the grain of this API when the API already has a notification mechanism.

How about this?

Add these methods to Links:

  getProviderLinks(provider)
    => returns Links._providerLinks.get(provider).sortedLinks

  getProviderSites(provider)
    => returns new Set(Links._providerLinks.get(provider).siteMap.keys())

  populateProviderCache(provider, callback)
    i.e., a public version of _populateProviderCache.  It should throw an error
    if the given provider is not in _providerLinks.  (We should remove the
    _providers Set because there's no point in keeping it when the keys of the
    _providerLinks Map provide the same info.)  Probably it should not have a
    `force` param and should pass false to the private method.

In DirectoryLinksProvider.init (I think?), do:

  NewTabUtils.PlacesProvider.addObserver(this);
  this._populatePlacesLinks();

And elsewhere in DirectoryLinksProvider:

  _populatePlacesLinks: function () {
    NewTabUtils.links.populateProviderCache(NewTabUtils.PlacesProvider, () => {
      this._doSomethingWithPlacesSites();
    });
  },

  _doSomethingWithPlacesSites: function () {
    let sites = NewTabUtils.links.getProviderSites(NewTabUtils.PlacesProvider);
    ...
  },

  onLinkChanged: function () {
    // Make sure NewTabUtils.links handles the notification first.
    setTimeout(() => { // setTimeout from Timer.jsm
      this._doSomethingWithPlacesSites();
    }, 0);
  },

  onManyLinksChanged: function () {
    // Make sure NewTabUtils.links handles the notification first.
    setTimeout(() => {
      this._populatePlacesLinks();
    }, 0);
  },

The setTimeouts are a little gross but I like the way these pieces fit together overall.  Sorry if I'm being an architecture astronaut, but I think this extends the current API in a nice, general way that satisfies your needs.  Let me know what you think.

Comment 14

[Marina Samuel [:emtwo]]

Attachment: Part1: v2: Make DirectoryLinksProvider listen to PlacesProvider and update its _topSitesWithRelatedLinks cache

Ah! Hadn't thought of the timeout workaround. How's this patch? It does what you suggested with a few minor exceptions:

* Didn't need a getProviderLinks() or getProviderSites(), just using isTopPlacesSite()

* Didn't need to populatePlacesLinks() on init, but instead I'm doing it after DirectoryLinks are populated since both providers need to have populated caches before we can start deciding on a related link/tile.

* Didn't need to populatePlacesLinks() in onManyLinksChanged() since that's already done when we wait with the timeout. (Unless I'm missing something here?)

* When replacing _providers with _providerLinks, I initially add a provider by setting its _providerLinks value to null. This resulted in a couple of checks elsewhere for _providerLinks to not be null. Is this what you had in mind?

Comment 15

[Drew Willcoxon :adw]

Comment on attachment 8564443 [details] [diff] [review]
Part1: v2: Make DirectoryLinksProvider listen to PlacesProvider and update its _topSitesWithRelatedLinks cache

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

(In reply to Marina Samuel [:emtwo] from comment #14)
> * Didn't need a getProviderLinks() or getProviderSites(), just using
> isTopPlacesSite()

OK, fair enough.

> * Didn't need to populatePlacesLinks() on init, but instead I'm doing it
> after DirectoryLinks are populated since both providers need to have
> populated caches before we can start deciding on a related link/tile.

Makes sense.

> * Didn't need to populatePlacesLinks() in onManyLinksChanged() since that's
> already done when we wait with the timeout. (Unless I'm missing something
> here?)

You're right.  The problem I was trying to guard against is that populating the PlacesProvider cache takes a nondeterministic number of event loop spins because the PlacesProvider has to go ask the SQLite DB, which depends on file I/O of course.

For some reason I was thinking that calls to _populateProviderCache are queued up.  So PlacesProvider calls Links.onManyLinksChanged, Links would trigger a Places DB fetch, your consumer would wait one turn of the event loop to allow that to happen, and then after that when your consumer called populateProviderCache, it wouldn't be "resolved" until the DB fetch finished and the PlacesProvider data was up-to-date.  Because if it's resolved before then, the data may be out of date since the DB fetch might not have finished.

Maybe the reason I was thinking that is that I meant to ask you to do that. :-/  It seems like it would be doable to keep a _populatePromise in the provider's cache -- when _populateProviderCache has to call provider.getLinks(), create a new promise and make all future calls to _populateProviderCache for that provider wait until the promise is resolved to have their callbacks called.  What do you think?

I'm sorry this is getting complex.  I don't mean to block your work. :-/

> * When replacing _providers with _providerLinks, I initially add a provider
> by setting its _providerLinks value to null. This resulted in a couple of
> checks elsewhere for _providerLinks to not be null. Is this what you had in
> mind?

Thanks for making that change, yes.  Would you mind renaming the map simply _providers?  And this resetCache method needs to be updated to null out the values instead of clearing the map: http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/NewTabUtils.jsm#842

::: toolkit/modules/NewTabUtils.jsm
@@ +888,5 @@
>     *               already filled.
>     */
>    _populateProviderCache: function Links_populateProviderCache(aProvider, aCallback, aForce) {
> +    if (this._providerLinks.has(aProvider) &&
> +        this._providerLinks.get(aProvider) && !aForce) {

It should be OK to remove the has() check and go with only the get() check.  For one, it's an error to call this private method on a provider that's not in the map, so if that were to throw an error, then that's what should happen.  But for two, if has() returned false then get() would return undefined, which is also falsey.

Comment 16

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #15)
> Maybe the reason I was thinking that is that I meant to ask you to do that.
> :-/  It seems like it would be doable to keep a _populatePromise

Didn't mean to prefix that with an underscore, not that it's a big deal.

Comment 17

[Marina Samuel [:emtwo]]

Attachment: Part1: v3: Make DirectoryLinksProvider listen to PlacesProvider and update its _topSitesWithRelatedLinks cache

Hadn't thought of the nondeterministic number of event loop spins scenario. Thanks for pointing that out. I've made your suggested changes to _populateProviderCache().

Comment 18

[Drew Willcoxon :adw]

Comment on attachment 8566121 [details] [diff] [review]
Part1: v3: Make DirectoryLinksProvider listen to PlacesProvider and update its _topSitesWithRelatedLinks cache

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

Thanks!  I think this is basically good to go, but I'd like to see one more patch with the _populateProviderCache comments below addressed.

::: browser/modules/DirectoryLinksProvider.jsm
@@ +450,5 @@
>      // setup directory file path and last download timestamp
>      this._directoryFilePath = OS.Path.join(OS.Constants.Path.localProfileDir, DIRECTORY_LINKS_FILE);
>      this._lastDownloadMS = 0;
> +
> +    NewTabUtils.placesProvider.addObserver(this);

Since this is added in init, but the initial _populatePlacesLinks happens in getLinks, it's possible that changes in the PlacesProvider will happen and you'll be notified of them before your initial _populatePlacesLinks is called.  I don't think that should be a problem given how _handleManyLinksChanged works, but it's something to be aware of.

@@ +480,5 @@
> +      this._topSitesWithRelatedLinks.delete(changedLinkSite);
> +      return;
> +    }
> +
> +    if (this._relatedLinks.get(changedLinkSite) !== undefined) {

has()?  If it's really possible that changedLinkSite with a value of undefined is present in the map, then a comment here explaining that would be helpful.

::: toolkit/modules/NewTabUtils.jsm
@@ +893,5 @@
>     * @param aForce When true, populates the provider's cache even when it's
>     *               already filled.
>     */
>    _populateProviderCache: function Links_populateProviderCache(aProvider, aCallback, aForce) {
> +    Task.spawn(function() {

Nice!  So the populatePromise is created if the cache doesn't exist or if aForce, and thereafter all calls wait on the promise until it needs to be created again.  I like it.

Could you please use Task.async instead?  And note the asterisk after "function":

_populateProviderCache: Task.async(function* (aProvider, aCallback, aForce) {

@@ +899,5 @@
> +          this._providers.get(aProvider).populatePromise) {
> +        yield this._providers.get(aProvider).populatePromise.promise;
> +        aCallback();
> +      }
> +      else if (this._providers.get(aProvider) && !aForce) {

Please cuddle the `else if` to match style.

@@ +902,5 @@
> +      }
> +      else if (this._providers.get(aProvider) && !aForce) {
> +        aCallback();
> +      } else {
> +        this._providers.set(aProvider, {populatePromise: Promise.defer()});

I think we're trying to move away from Promise.jsm in favor of native promises.  Could you please use them instead?  Something like:

} else {
  this._providers.set(aProvider, { populatePromise: new Promise((resolve, reject) => {

@@ +911,5 @@
> +
> +          // Store the populatePromise so we can resolve it after we've replaced it.
> +          let providerCache = this._providers.get(aProvider);
> +          let populatePromise = null;
> +          if (providerCache) {

Hmm, when would providerCache ever not exist at this point?  Only if the provider was removed or resetCache was called while we were waiting on aProvider.getLinks, right?  I think in that case we should just bail here and not touch this._providers at all -- but aCallback should still probably be called.  Because from the consumer's POV, they removed the provider or reset the cache after populating it.

So I think inside the aProvider.getLinks callback here, this._providers.get(aProvider) would either be undefined, or it should be the exact same value as it was before the getLinks call.  Right?  In that case, you could store providerCache = this._providers.get(aProvider) before the get links call and then modify providerCache inside the getLinks callback.  Then you're not touching this._providers at all inside the callback, so you don't have to worry about whether it's still present in the map.

As for populatePromise inside the callback -- if this._providers.get(aProvider) exists but it doesn't have populatePromise, that seems like an error, like a bug.  So I don't think it's something you should make sure exists first.

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js
@@ +19,5 @@
> +
> +  let getLinksFcn = function(callback) {
> +    count++;
> +    if (count > 1) {
> +      do_throw("Should not be calling getLinksFcn twice");

Nit: Please write as do_check_eq(count == 1).

@@ +23,5 @@
> +      do_throw("Should not be calling getLinksFcn twice");
> +    }
> +
> +    setTimeout(() => {
> +      callback(expectedLinks);

Using a timeout should be safe pretty much all of the time, but the test machines are pretty crazy sometimes, and seemingly safe code that relies on specific timings can randomly fail.  Could you please use a promise instead?  I think that should work.  Make a promise outside getLinksFcn, make the call to callback wait until that promise is resolved, and then after the second time you call populateProviderCache below, resolve the promise.

Comment 19

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #18)
> So I think inside the aProvider.getLinks callback here,
> this._providers.get(aProvider) would either be undefined, or it should be
> the exact same value as it was before the getLinks call.  Right?

Er, no, the provider could be both removed and then populated again before the getLinks() finishes.  In that case the value in the map could be different, although in reality I don't think the second getLinks call would actually finish before the first.  But that's more reason to store providerCache = this._providers.get(aProvider) before the getLinks call -- to make sure you're not modifying a newer providerCache object.

Comment 20

[Marina Samuel [:emtwo]]

Attachment: Part1: v4: Make DirectoryLinksProvider listen to PlacesProvider and update its _topSitesWithRelatedLinks cache

This addresses your comments (hopefully I understood them all correctly). There are a couple of things I wanted to point out:

1) The way you described the changes to the test seemed to be getting into a deadlock. The 2nd call to populateProviderCache() was waiting on populatePromise, populatePromise is resolved when getLinks() finishes, and getLinksFcn would be waiting on the 2nd call to populateProviderCache() to finish.

Instead, I yield on an immediate Promise.resolve(). This results in an async getLinksFcn so that we can test the functionality of populatePromise.

2) Since getLinks() is synchronous in most of the tests, I needed to add a couple of lines to handle that (commented in the code).

Comment 21

[Drew Willcoxon :adw]

Comment on attachment 8567311 [details] [diff] [review]
Part1: v4: Make DirectoryLinksProvider listen to PlacesProvider and update its _topSitesWithRelatedLinks cache

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

::: toolkit/modules/NewTabUtils.jsm
@@ +899,5 @@
>        aCallback();
>      } else {
> +      // Clear the cache since we're about to re-populate it and we can
> +      // check how many keys are in cache to know if getLinks() has been called.
> +      this._providers.set(aProvider, {});

Sorry, I think I goofed when I rewrote this part of your previous patch above.  I don't think we want to set the provider cache to a new object.  If we did that, then people asking for the provider's cached links or siteMap or whatnot (e.g., via NewTabUtils.isTopPlacesSite) would get nothing while provider.getLinks() is busy.  The cache is and should be valid until provider.getLinks() finishes.

@@ +902,5 @@
> +      // check how many keys are in cache to know if getLinks() has been called.
> +      this._providers.set(aProvider, {});
> +
> +      let providerCache = Links._providers.get(aProvider);
> +      providerCache.populatePromise = new Promise((resolve, reject) => {

Yeah, this is the only thing I was asking for.  (And not a big deal, but why use Links instead of `this`?)

@@ +909,5 @@
> +          // them in getLinks when merging links from providers.
> +          links = links.filter((link) => !!link);
> +          providerCache.sortedLinks = links;
> +          providerCache.siteMap = links.reduce((map, link) => {
> +            Links._incrementSiteMap(map, link);

Same nit here for Links -- `this` should work.

@@ +923,3 @@
>        });
> +      // If the provider's cache has > 1 keys then getLinks() is synchronous
> +      // (For now, this is only the case in tests) so we should delete the promise.

Hmm, I don't understand how getLinks() being sync or async is relevant...  If you didn't delete the promise here, then the next call to _populateProviderCache would hit the first branch in the conditional at the top, yield to the promise, which since it's already resolved would cause aCallback to be called on the next tick of the event loop...  oh, is that why?  The tests assume that their callbacks will be called syncly?  I'm looking at the existing tests though and none of them do anything in their populateCache  callbacks.  They just expect the cache to be populated by the time populateCache returns, which should still be the case?

Comment 22

[Marina Samuel [:emtwo]]

(In reply to Drew Willcoxon :adw from comment #21)
> Comment on attachment 8567311 [details] [diff] [review]
> Part1: v4: Make DirectoryLinksProvider listen to PlacesProvider and update
> its _topSitesWithRelatedLinks cache
> 
> Review of attachment 8567311 [details] [diff] [review]:
> -----------------------------------------------------------------
> I don't think we want to set the provider cache to a new object.

I think I meant this for the case when we call _populateProviderCache() but this._providers.get(aProvider) is null. Make sense to wrap this in an if clause for that case?

> Hmm, I don't understand how getLinks() being sync or async is relevant... 
> If you didn't delete the promise here, then the next call to
> _populateProviderCache would hit the first branch in the conditional at the
> top, yield to the promise, which since it's already resolved would cause
> aCallback to be called on the next tick of the event loop...  oh, is that
> why?  The tests assume that their callbacks will be called syncly?  I'm
> looking at the existing tests though and none of them do anything in their
> populateCache  callbacks.  They just expect the cache to be populated by the
> time populateCache returns, which should still be the case?

Sorry I didn't explain this more thoroughly. This issue came up specifically in a test that called notifyManyLinksChanged() after calling populateCache(). This test assumes populateCache() will be complete by the time notifyManyLinksChanged() is called. However, the call to notifyManyLinksChanged() ends up in the first if-clause yielding on populatePromise. However, notifyManyLinksChanged() should end up in the else{} clause, refilling the entire cache.

In fact, I think this may not be an issue specific to synchronous getLinks(), if we don't delete populatePromise, then we would never end up repopulating the cache if we wanted to.

Comment 23

[Drew Willcoxon :adw]

Oh wow, you're right.  Sorry for completely missing that.  If I were writing this, I'd do:

_populateProviderCache: function (aProvider, aCallback, aForce) {
  let cache = this._providers.get(aProvider);
  let creatCache = !cache;
  if (creatCache) {
    cache = {
      // Start with a resolved promise.
      populatePromise: new Promise(resolve => resolve()),
    };
    this._providers.set(aProvider, cache);
  }
  // Chain the populatePromise so that calls are effectively queued.
  cache.populatePromise = cache.populatePromise.then(() => {
    return new Promise(resolve => {
      if (!createCache && !aForce) {
        aCallback();
        resolve();
        return;
      }
      aProvider.getLinks(links => {
        // Set properties on `cache`...
        aCallback();
        resolve();
      });
    });
  });
}

I think that solves both problems, and for me at least it's simpler.  There's probably something wrong I'm not seeing but that's the best I've got on a Friday night!

Comment 24

[Marina Samuel [:emtwo]]

Attachment: Part1: v5: Make DirectoryLinksProvider listen to PlacesProvider and update its _topSitesWithRelatedLinks cache

No worries, I like your _populateProviderCache(). It's simple and seems to work well :)

I've made that change here and a few changes in the tests since we can no longer rely on the fact that a sync getLinks() will make _populateProviderCache() sync.

Comment 25

[Drew Willcoxon :adw]

Comment on attachment 8568003 [details] [diff] [review]
Part1: v5: Make DirectoryLinksProvider listen to PlacesProvider and update its _topSitesWithRelatedLinks cache

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

Great, thanks!

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js
@@ +46,5 @@
>    provider.maxNumLinks = expectedLinks.length;
>  
>    NewTabUtils.initWithoutProviders();
>    NewTabUtils.links.addProvider(provider);
> +  let promise = new Promise(resolve => {

Nit: This can be a little more succinct:

yield new Promise(resolve => NewTabUtils.links.populateCache(resolve));

Comment 26

[Marina Samuel [:emtwo]]

Attachment: Part 2: v1: Choose and display a related tile based on a user's top sites

Comment 27

[Drew Willcoxon :adw]

Comment on attachment 8569320 [details] [diff] [review]
Part 2: v1: Choose and display a related tile based on a user's top sites

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

::: browser/modules/DirectoryLinksProvider.jsm
@@ +493,3 @@
>      }
>  
> +    if (this._relatedLinks.has(changedLinkSite) && !linkStored) {

&& NewTabUtils.isTopPlacesSite(changedLinkSite) right?

@@ +534,5 @@
> +    let initialLength = sortedLinks.length;
> +    if (initialLength) {
> +      let mostFrecentLink = sortedLinks[0];
> +      if ("related" == mostFrecentLink.type) {
> +        this.maxNumLinks = initialLength - 1; // So we delete the related tile

Hmm.  This is a pretty indirect way of removing the first link.  (But I don't blame you since there's no better way to do it!)

onLinkChanged was designed with the idea that the provider knew that one of its links had changed, but it didn't really know which one or how the change affected the sorted list.  But in this case you can be precise, so maybe we should change onLinkChanged.

What do you think about:

* Add aIndex=-1 and aDeleted=false params (please document them)
* In the `if (existingLink)` if-branch:
  * Set idx to aIndex if aIndex >= 0
  * Throw an Error if this.compareLinks(aLink, sortedLinks[idx]) != 0
  * If aDeleted, don't set insertionLink, but do set updatePages=true,
    and update linkMap and call _decrementSiteMap

@@ +554,5 @@
> +    let possibleLinks = new Map();
> +    this._topSitesWithRelatedLinks.forEach(topSiteWithRelatedLink => {
> +      let relatedLinksMap = this._relatedLinks.get(topSiteWithRelatedLink);
> +      relatedLinksMap.forEach((relatedLink, url) => {
> +        possibleLinks.set(url, relatedLink);

Why does possibleLinks need to be a map?  Wouldn't it be simpler to keep a set?  Is it because multiple top sites could map to the same set of related links, and you're trying to keep possibleLinks/flattenedLinks as small as possible?  If so, good idea, but a brief comment would help.

@@ +557,5 @@
> +      relatedLinksMap.forEach((relatedLink, url) => {
> +        possibleLinks.set(url, relatedLink);
> +      })
> +    });
> +    let flattenedLinks = [...possibleLinks.values()];

Cool, I didn't know you could do that.

Comment 28

[Marina Samuel [:emtwo]]

Attachment: Part 2: v2: Choose and display a related tile based on a user's top sites

Comment 29

[Drew Willcoxon :adw]

Comment on attachment 8570140 [details] [diff] [review]
Part 2: v2: Choose and display a related tile based on a user's top sites

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

Thanks Marina, r+ with the changes below and requested test.

::: browser/modules/DirectoryLinksProvider.jsm
@@ +554,5 @@
> +    // Create a flat list of all possible links we can show as related.
> +    // Note that many top sites may map to the same related links, but we only
> +    // want to count each related link once (based on url), thus possibleLinks is a map
> +    // from url to relatedLink. Thus, each link has an equal chance of being chosen at
> +    // random from flattenedLinks if it appears only once.

Ah.

::: toolkit/modules/NewTabUtils.jsm
@@ +966,5 @@
>     * @param aProvider The provider whose link changed.
>     * @param aLink The link that changed.  If the link is new, it must have all
>     *              of the _sortProperties.  Otherwise, it may have as few or as
>     *              many as is convenient.
> +   * @param aIndex The current index of a changed link in the sortedLinks

a changed link -> the changed link

Please also say it's -1 if the provider doesn't know.

@@ +968,5 @@
>     *              of the _sortProperties.  Otherwise, it may have as few or as
>     *              many as is convenient.
> +   * @param aIndex The current index of a changed link in the sortedLinks
> +                   cache in _providers.
> +   * @param aDeleted Boolean indicating if the provider has deleted a link.

a link -> the link

@@ +1001,4 @@
>          if (idx < 0) {
>            throw new Error("Link should be in _sortedLinks if in _linkMap");
>          }
> +        let removedLink = sortedLinks.splice(idx, 1);

splice returns an array, so the lines below that use removedLink shouldn't actually work...  And you don't really need the spliced-out link anyway, you can just use existingLink.

Would you mind adding a small test to test_NewTabUtils.js to make sure this case does work?  (sortedLinks, linkMap, and siteMap are updated correctly.)

Comment 30

[Marina Samuel [:emtwo]]

Attachment: Part 3: v1: Mochitest fixes for suggested tiles.

The mochitest failures turned out to be 3 separate issues that are addressed here.

1) 754 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/newtab/browser_newtab_drop_preview.js | uncaught exception - TypeError: links.sortedLinks is undefined at resource://gre/modules/NewTabUtils.jsm:940

This one occurred when browser-thumbnails.js called getLinks() in _topSiteURLs() before provider had populated the cache, so it only had populatePromise. In this case I think the cache should still be considered 'empty' since we are not yet done populating it. So an additional check for 'Object.keys(links).length > 1' treats the cache as empty.

2) 704 INFO Console message: [JavaScript Error: "TypeError: sortedLinks is undefined" {file: "resource:///modules/DirectoryLinksProvider.jsm" line: 535}]

This one occurred when NewTabUtils.links.resetCache() is called before getting to _updateRelatedTile(). So I added a check confirming that the cache is not empty before we start doing things with it in _updateRelatedTile().

3) 695 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/newtab/browser_newtab_drag_drop.js | grid status = 0,1,2,3,4,5,6,7,8 - Got , expected 0,1,2,3,4,5,6,7,8

This was a weird one (and I'm still not sure I really get what's going on here) But it looked like we were checking links on the newtab page before it had fully loaded. But this seemed to *only* happened in the browser_newtab_drag_drop if browser_newtab_disable was called before it. Each test ran fine on their own so they seemed to be interfering with each other. I tried to track down what the exact interference was with no success. Adding a yield to restore state after browser_newtab_disable seems to have resolved the issue.

Here are the try results with these changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65772cea028a

Comment 31

[Drew Willcoxon :adw]

Comment on attachment 8575330 [details] [diff] [review]
Part 3: v1: Mochitest fixes for suggested tiles.

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

Thanks for figuring all that out.

(In reply to Marina Samuel [:emtwo] from comment #30)
> This was a weird one (and I'm still not sure I really get what's going on
> here)

I'm not sure what's going on either without spending a lot of time on it and even then maybe I couldn't tell.  restore() calls whenPagesUpdated(), so maybe not waiting for the pages to be updated before starting the next test was a problem.  It also calls NewTabUtils.restore(), which populates the cache, so maybe not triggering cache population before starting the next test was a problem.  I'd guess the first, so you might try yielding only whenPagesUpdated instead of restore and see if that works.

Anyway, if tryserver is happy, that's fine.  But be careful of intermittent failures, which are definitely a possibility given all the asynchronicity.  If your fix is good then you should be able to retrigger green/passing test runs multiple times and still have them pass.

::: toolkit/modules/NewTabUtils.jsm
@@ +935,5 @@
>    _getMergedProviderLinks: function Links__getMergedProviderLinks() {
>      // Build a list containing a copy of each provider's sortedLinks list.
>      let linkLists = [];
>      for (let links of this._providers.values()) {
> +      if (links && Object.keys(links).length > 1) {

links && links.sortedLinks would be better, wouldn't it?

Comment 32

[Marina Samuel [:emtwo]]

Attachment: Part 3: v2: Mochitest fixes for suggested tiles.

I made the `links && links.sortedLinks` change. For yield restore(), it seems that both whenPagesUpdated() and NewTabUtils.restore() are needed. Using only one or the other resulted in a test hang.

I've re-triggered these tests a bunch of times on try (still waiting for results, so far a couple of retries have completed and look good).

Once the retries are complete, if they pass, is this good to go?

If they don't pass, what would be the best approach since I wasn't very successful debugging this otherwise? Would it be ok to file it as a separate bug so we don't need to hold this one up?

Thanks!

Comment 33

[Drew Willcoxon :adw]

Comment on attachment 8575503 [details] [diff] [review]
Part 3: v2: Mochitest fixes for suggested tiles.

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

Sure, if try is green then this is fine.

Comment 34

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/e29b97e5e30c
https://hg.mozilla.org/integration/fx-team/rev/a1fa056e0c0d
https://hg.mozilla.org/integration/fx-team/rev/7d48a8bad488

Comment 35

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

I had to back this out for causing a spike in bc1 failures in new tab tests:
https://hg.mozilla.org/integration/fx-team/rev/1677ee4551b7

https://treeherder.mozilla.org/logviewer.html#?job_id=2222379&repo=fx-team

Comment 36

[Marina Samuel [:emtwo]]

Attachment: Part 3: v3: Mochitest fixes for suggested tiles.

Drew, sorry to bug you again with this, but looks like that last attempt still had intermittent failures.

I have another attempt here. Since the opened tab seemed to be in the wrong state at the beginning of those tests, using a fresh, new tab seems to be helping. Here's try with a ton of reruns on those tests. Seems to be no unexpected failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f946b43f277f

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Backed out for newtab mochitest-bc failures (OSX 10.10 debug so far).
https://hg.mozilla.org/integration/fx-team/rev/14460ab11c3b

https://treeherder.mozilla.org/logviewer.html#?job_id=2265805&repo=fx-team

Comment 38

[Ed Lee :Mardak]

I retriggered some 10.10 debug bc1 https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=8522ea4f4621

emtwo, I'll try to reproduce on my 10.10 with debug build.

Comment 39

[Marina Samuel [:emtwo]]

Thanks Mardak, the error in browser_newtab_drop_preview looks fairly similar to the errors in the other drag/drop files which seemed to disappear by adding 'yield addNewTabPageTab()' at the beginning of the tests.

Here's a try where I added that in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a482778b500

So far results for bc1 on osx 10.10 still failed, but looks like the cause wasn't this patch. Waiting on the other bc1s

Comment 40

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dab6b12a0ff6
https://hg.mozilla.org/mozilla-central/rev/973fab97c0b2
https://hg.mozilla.org/mozilla-central/rev/d0e5df2bd844

Comment 41

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

Seems to be covered by several tests. Marking as qe-verify-.

Comment 42

[Ed Lee :Mardak]

https://hg.mozilla.org/releases/mozilla-beta/rev/869ba4681d1b
https://hg.mozilla.org/releases/mozilla-beta/rev/48564fb0e663
https://hg.mozilla.org/releases/mozilla-beta/rev/1a007e477655