1136203 - Remove history thumbnail/title replacing functionality

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

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.

Comment 1

[Ed Lee :Mardak]

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.

Comment 2

[Marina Samuel [:emtwo]]

Attachment: v1: Remove thumbnail/title replacing functionality for history tiles

Comment 3

[Ed Lee :Mardak]

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...)

Comment 4

[Ed Lee :Mardak]

Prioritizing this removal of enhancing history functionality because it's causing user confusion and customer confusion.

Comment 5

[Drew Willcoxon :adw]

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.

Comment 6

[Marina Samuel [:emtwo]]

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

Comment 7

[Ed Lee :Mardak]

Turns out the history type check wasn't enough. This causes a suggested tile to get enhanced by a directory tile.

Comment 8

[Ed Lee :Mardak]

Attachment: suggested not enhanced.png

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").

Comment 9

[Ed Lee :Mardak]

Let's do the follow up fixes per comment 7, comment 8 in bug 1105360.

Comment 10

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

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

Comment 11

[Marina Samuel [:emtwo]]

(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?

Comment 12

[Ed Lee :Mardak]

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.

Comment 13

[Ed Lee :Mardak]

https://hg.mozilla.org/releases/mozilla-beta/rev/1d9b014f0414

Comment 14

[Cornel Ionce [:noni]]

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