Closed Bug 1139052 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.2 - 27 Apr
Tracking Status
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: Mardak, Assigned: emtwo)

References

(Blocks 1 open bug)

Details

(Whiteboard: .007)

Attachments

(2 files)

This is to avoid showing a suggested tile and potentially a sponsored directory tile at the same time.
Duplicate of this bug: 1139226
Not prioritized for initial suggested tiles implementation.
No longer blocks: 1120311
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: .?
Blocks: 1140185
Points: --- → 2
Whiteboard: .005
Whiteboard: .005 → .007
Assignee: nobody → msamuel
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?"
Attachment #8591959 - Flags: review?(adw) → review+
> 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?
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.
https://hg.mozilla.org/mozilla-central/rev/9b3b216d2e98
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Blocks: 1160372
Blocks: 1161412
No longer blocks: 1160372
Attached patch for auroraSplinter Review
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.
Attachment #8591959 - Flags: approval-mozilla-aurora+
No longer blocks: 1161412
Iteration: --- → 40.2 - 27 Apr
You need to log in before you can comment on or make changes to this bug.