1139052 - Only show suggested tiles if the user has more than 8 top history sites

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

This is to avoid showing a suggested tile and potentially a sponsored directory tile at the same time.

Comment 2

[Ed Lee :Mardak]

Not prioritized for initial suggested tiles implementation.

Comment 3

[Marina Samuel [:emtwo]]

Attachment: v1: Don't show suggested tiles for users with fewer than 8 history tiles

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8591959 [details] [diff] [review]
v1: Don't show suggested tiles for users with fewer than 8 history tiles

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

r+, but I don't completely understand the logic below.  If in fact there is some problem with it, then please do post a new patch, despite the r+.

::: browser/modules/DirectoryLinksProvider.jsm
@@ +614,5 @@
> +    }
> +
> +    let currTopSiteCount = this._getCurrentTopSiteCount();
> +    if ((!mostFrecentLink.targetedSite && currTopSiteCount >= MIN_VISIBLE_HISTORY_TILES) ||
> +        (mostFrecentLink.targetedSite && currTopSiteCount < MIN_VISIBLE_HISTORY_TILES)) {

I was confused by this at first because I was thinking the point of this method is to say whether or not a suggested tile should be shown, and so the negation on the two mostFrecentLink.targetedSite terms seemed backwards.

But like the method's name says, the point is to say whether the suggested tile has changed visibility, basically, right?  So the first && expression in this conditional says, "we can now show a suggested tile," and the second && expression says, "we should now hide the suggested tile," is that basically right?  If so, I still don't completely understand the first && expression, because if the most frecent link does not have a targeted site, then no suggested tile can be shown even though there are >= 8 history tiles, right?

Actually, it does seem that this method is being used in two different ways.  The onLinkChanged caller seems to be using it as, "did the suggested tile change visibility?"  But the _updateSuggestedTile caller seems to be using it as, "should the suggested tile be visible?"

Comment 5

[Marina Samuel [:emtwo]]

> But like the method's name says, the point is to say whether the suggested
> tile has changed visibility, basically, right?

Not quite.. if mostFrecentLink has a targetedSite then mostFrecentLink is a suggested link. So the logic says if we have enough history links (8+) to show a suggested tile and we are not already showing a suggested tile, then we should update (to *attempt* to add a suggested tile). OR if we don't have enough history to show a suggested tile (<8) and we are currently showing one, we should update (to remove it) 

So I guess it's checking for both suggested tile visibility and number of history tiles.

> So the first && expression
> in this conditional says, "we can now show a suggested tile," and the second
> && expression says, "we should now hide the suggested tile," is that
> basically right?  

yes

> If so, I still don't completely understand the first &&
> expression, because if the most frecent link does not have a targeted site,
> then no suggested tile can be shown even though there are >= 8 history
> tiles, right?

If the most frecent link does not have a targeted site, that means it is not a suggested link. But I don't see why this would mean no suggested tile can be shown?

Comment 6

[Drew Willcoxon :adw]

Thanks for explaining, my understanding was wrong about what link.targetedSite means.  r+ still stands, but would you mind adding a comment in the code saying pretty much what you said in your first paragraph in comment 5?  It's really helpful.

Comment 7

[Marina Samuel [:emtwo]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b3b216d2e98

Comment 9

[Ed Lee :Mardak]

Attachment: for aurora

Comment 10

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

Comment on attachment 8591959 [details] [diff] [review]
v1: Don't show suggested tiles for users with fewer than 8 history tiles

Approving for uplift to aurora based on the request from bug 1161412.

Comment 11

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/069da5c02e22