If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 39

Status

()

Firefox
New Tab Page
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Mardak, Assigned: emtwo)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 40
Points:
2
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 fixed, firefox40 fixed)

Details

(Whiteboard: .007)

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
This is to avoid showing a suggested tile and potentially a sponsored directory tile at the same time.
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1139226
(Reporter)

Comment 2

3 years ago
Not prioritized for initial suggested tiles implementation.
No longer blocks: 1120311
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: .?
(Reporter)

Updated

3 years ago
Blocks: 1140185
(Assignee)

Updated

3 years ago
Points: --- → 2

Updated

3 years ago
Whiteboard: .005

Updated

3 years ago
Whiteboard: .005 → .007
(Assignee)

Updated

3 years ago
Assignee: nobody → msamuel
(Assignee)

Comment 3

3 years ago
Created attachment 8591959 [details] [diff] [review]
v1: Don't show suggested tiles for users with fewer than 8 history tiles
Attachment #8591959 - Flags: review?(adw)

Comment 4

3 years ago
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+
(Assignee)

Comment 5

3 years ago
> 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

3 years ago
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.
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/9b3b216d2e98
https://hg.mozilla.org/mozilla-central/rev/9b3b216d2e98
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
(Reporter)

Updated

2 years ago
Blocks: 1160372
(Reporter)

Updated

2 years ago
Blocks: 1161412
(Reporter)

Updated

2 years ago
No longer blocks: 1160372
(Reporter)

Comment 9

2 years ago
Created attachment 8602799 [details] [diff] [review]
for aurora
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+
(Reporter)

Updated

2 years ago
No longer blocks: 1161412
https://hg.mozilla.org/releases/mozilla-aurora/rev/069da5c02e22
status-firefox39: --- → fixed
(Reporter)

Updated

2 years ago
Iteration: --- → 40.2 - 27 Apr
You need to log in before you can comment on or make changes to this bug.