1151876 - A blocked link should not appear in the _topSitesWithSuggestedLinks cache or trigger a suggested tile

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Comment 1

[Marina Samuel [:emtwo]]

To be more specific, the siteMap of the blocked link should be decremented and this should impact _topSitesWithSuggestedLinks if the siteMap count for that site reaches 0.

Comment 2

[Marina Samuel [:emtwo]]

Attachment: v1: Blocking a targeted tile should remove its associated suggest tile in the next new tab

Comment 3

[Drew Willcoxon :adw]

Comment on attachment 8589864 [details] [diff] [review]
v1: Blocking a targeted tile should remove its associated suggest tile in the next new tab

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

Leaving the r? for now, pending the _decrementSiteMap question below.

::: toolkit/modules/NewTabUtils.jsm
@@ +498,5 @@
>      return this._links;
>    },
>  
> +  _adjustSiteMapAndNotify: function(aLink, increment=true) {
> +    for (let [provider, cache] of NewTabUtils.links._providers) {

Please add a brief comment explaining in English what this for-loop is doing and why.  Or actually a method comment explaining the same thing might be better.  And, I actually forgot what siteMap is used for.  So I checked the comment above the definition of _providers, and it explains what's inside the map but not what it's used for.  Would you mind expanding that comment to describe what siteMap is used for?  (I see that you add to it below in this patch, which is great, but it could still use more explanation.)

Also, please use Links instead of NewTabUtils.links.

@@ +501,5 @@
> +  _adjustSiteMapAndNotify: function(aLink, increment=true) {
> +    for (let [provider, cache] of NewTabUtils.links._providers) {
> +      if (cache.linkMap.get(aLink.url)) {
> +        if (increment) {
> +          NewTabUtils.links._incrementSiteMap(cache.siteMap, aLink);

From an architecture-astronaut POV, I'd prefer for onLinkBlocked and onLinkUnblocked methods to be called on observers; Links would implement those methods and then do all this logic, instead of having BlockedLinks call "private" methods on Links.

But I don't think it's a big deal, so I won't ask you to do that, but don't let me stop you from making that change if you'd like.

@@ +903,5 @@
>    },
>  
>    _incrementSiteMap: function(map, link) {
> +    if (NewTabUtils.blockedLinks.isBlocked(link)) {
> +      // Don't count blocked URLs.

Since this check is present in _increment but not _decrement, it makes makes me worry that the count might go negative or NaN.  e.g., someone calls this on a blocked link, so the count doesn't change, and then later on the link goes away so _decrement gets called.  Could a scenario like that happen?  It seems like it to me: _decrement is called by onLinkChanged when a link is deleted or gets pushed out.  If it happens to be a blocked link...

So I think _decrement should probably do the same check, but I'm not sure.  What do you think?

Comment 4

[Marina Samuel [:emtwo]]

Attachment: v2: Blocking a targeted tile should remove its associated suggest tile in the next new tab

> From an architecture-astronaut POV, I'd prefer for onLinkBlocked and
> onLinkUnblocked methods to be called on observers; Links would implement
> those methods and then do all this logic, instead of having BlockedLinks
> call "private" methods on Links.

I made this change, it does make a little more sense. Though I also needed to add an observers list to Links so that it can send notifications to DirectoryLinksProvider.


> So I think _decrement should probably do the same check, but I'm not sure. 
> What do you think?

Yes I did miss this scenario. Thank you. I made that update.

Comment 5

[Marina Samuel [:emtwo]]

Attachment: v3: Blocking a targeted tile should remove its associated suggest tile in the next new tab

comment was incomplete

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8590396 [details] [diff] [review]
v3: Blocking a targeted tile should remove its associated suggest tile in the next new tab

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

Thanks, Marina.

Comment 7

[Marina Samuel [:emtwo]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3c1b43d1b703

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8590396 [details] [diff] [review]
v3: Blocking a targeted tile should remove its associated suggest tile in the next new tab

[Triage Comment]  Approved for uplift to aurora from the request in bug 1161412.

Comment 10

[Ed Lee :Mardak]

Attachment: for aurora

Comment 11

[Ed Lee :Mardak]

Attachment: for aurora

Comment 12

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ca2f685a10a