Closed Bug 1136203 Opened 6 years ago Closed 6 years ago

Remove history thumbnail/title replacing functionality

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
normal
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 39
Iteration:
39.3 - 30 Mar
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: Mardak, Assigned: emtwo)

References

Details

(Whiteboard: .002)

Attachments

(2 files)

The Enhnaced Tiles functionality has been causing confusion without much adoption because we change the image and text without breaking the user's expected behavior of going to a specific link.

It also causes bugs that need to be fixed like bug 1105360 where some enhanced tiles should not show up as directory or some directory tiles with rollover images should not show up as enhanced.
Depends on: 1030832
Assignee: nobody → msamuel
This might not be necessary for the affiliate suggested tiles depending on how we implement it. I was originally concerned that the rollover images on suggested tiles would confuse the enhanced tiles implementation.

But if we do keep around enhanced tiles with affiliate suggested tiles, we would need to rethink bug 1126186 where the menu is simpler without needing to support enhanced tiles. If we do keep enhanced, we would either 1) add an extra menu item or 2) use the same "suggestions" menu item to turn off enhanced tiles. Both have drawbacks of confusing users or causing the user to unnecessarily turn off suggested tiles.
Attachment #8584735 - Attachment is patch: true
Comment on attachment 8584735 [details] [diff] [review]
v1: Remove thumbnail/title replacing functionality for history tiles

>+++ b/browser/base/content/newtab/sites.js
>   _render: function Site_render() {
>-    let enhanced = gAllPages.enhanced && DirectoryLinksProvider.getEnhancedLink(this.link);
>+    let enhanced = gAllPages.enhanced && this.link.type != "history" &&
>+      DirectoryLinksProvider.getEnhancedLink(this.link);
An alternative approach that might be simpler is to not add in links to the data structure behind getEnhancedLink. A directory tile and suggested tile with a enhancedImageURI should still have that attribute on the link. If we add nothing to the data structure, nothing will become additionally enhanced, i.e., history tiles, but already enhanced directory/suggested tiles should just work. (I haven't touched that code for a while, so I'm not sure what this approach might break...)
Prioritizing this removal of enhancing history functionality because it's causing user confusion and customer confusion.
Whiteboard: .? → .002
Comment on attachment 8584735 [details] [diff] [review]
v1: Remove thumbnail/title replacing functionality for history tiles

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

I like Ed's idea, but I think it's not even necessary to keep the _enhancedLinks structure up-to-date.  We could do that -- DirectoryLinksProvider already observes PlacesProvider -- or, since getEnhancedLink is the one public access point to _enhancedLinks, and it takes the original link object, how about just moving the link.type != "history" check into getEnhancedLink?  Then you don't have to update callers.

i.e., in Site._render, keep it as it is now:

let enhanced = gAllPages.enhanced && DirectoryLinksProvider.getEnhancedLink(this.link);

But change getEnhancedLink like:

getEnhancedLink: function DirectoryLinksProvider_getEnhancedLink(link) {
  return link.type == "history" ? null :
         link.enhancedImageURI && link ? link :
         this._enhancedLinks.get(NewTabUtils.extractSite(link.url));
},

Wouldn't that work?  But I'm OK with this patch too, so up to you, r+ either way.
Attachment #8584735 - Flags: review?(adw) → review+
Turns out the history type check wasn't enough. This causes a suggested tile to get enhanced by a directory tile.
I tested with this json:

https://people.mozilla.org/~elee/suggested2.json

Note the suggested tile should have a title with ".. Suggest!" but I only see it get a title of the directory tile.


This attachment is of a slightly different json where the directory link has a different domain to allow the directory tile to not be excluded by the one-tile-per-domain logic. But conceptually, we should be able to have a suggested link and a directory link with different images/titles. Where the current landed patch replaces the suggested images/title with the enhanced directory one (because suggested type is not "history").
Let's do the follow up fixes per comment 7, comment 8 in bug 1105360.
Iteration: --- → 39.3 - 30 Mar
Points: --- → 3
https://hg.mozilla.org/mozilla-central/rev/9f25f9cf6ce1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Blocks: 1148859
(In reply to Ed Lee :Mardak from comment #8)
> But conceptually, we should be able to have a
> suggested link and a directory link with different images/titles.

Do you mean that we should consider two links with the same URL as separate (and can appear simultaneously on a page) if one is directory and another is suggested?
We can just let the existing new tab logic work, i.e., new tab page only shows one link for a given site. So if there's a suggested tile, it'll hide the directory one. Arguably, the longer term desired logic is bug 1139052 where there shouldn't be any suggested tiles shown if directory tiles are visible.
Verified on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit using:
- Firefox 38 beta 4, build ID: 20150413143743;
- latest Firefox 39.0a2, build ID: 20150414004003
Status: RESOLVED → VERIFIED
Blocks: 1155443
No longer blocks: 1155443
You need to log in before you can comment on or make changes to this bug.