1105360 - Only enhance tiles that are explicitly enhanceable

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

Currently directory tiles with rollover images also enhance history tiles. Some partners want rollover directory tiles but not enhanced tiles, so we'll need to check a flag and only enhance then.

Comment 2

[Ed Lee :Mardak]

Bug 1136203 fixes this for history tiles, so a number of the duplicate/depends on bugs should be fixed now. But the enhancing from directory links still affects suggested tiles.

Comment 3

[Marina Samuel [:emtwo]]

Attachment: v1: Only enhance tiles with an explicit 'enhancedImageURI' property

Removing getEnhancedLink() and references since enhanced links will have their 'enhancedImageURI' in their link object. This will prevent a suggested tile from using the enhanced properties of a directory tile.

Also, when a suggested and directory tile have the same url, the properties for the suggested tile were not getting updated in NewTabUtils.links.onLinkChanged().

Comment 4

[Ed Lee :Mardak]

Comment on attachment 8586207 [details] [diff] [review]
v1: Only enhance tiles with an explicit 'enhancedImageURI' property

>+++ b/browser/modules/DirectoryLinksProvider.jsm
>   getLinks: function DirectoryLinksProvider_getLinks(aCallback) {
>     this._readDirectoryLinksFile().then(rawLinks => {
>       // Reset the cache of suggested tiles and enhanced images for this new set of links
>-      this._enhancedLinks.clear();
>       let setCommonProperties = function(link, length, position) {
>-        // Stash the enhanced image for the site
>-        if (link.enhancedImageURI) {
>-          this._enhancedLinks.set(NewTabUtils.extractSite(link.url), link);
>-        }
>         link.lastVisitDate = length - position;
>       }.bind(this);
> 
>       rawLinks.suggested.filter(validityFilter).forEach((link, position) => {
>         setCommonProperties(link, rawLinks.suggested.length, position);
Sorry, after seeing the necessary code changes, I think we can still support enhanced tiles for customers that really do want it. The only change we need here is to only do _enhancedLinks.set from a rawLinks.enhanced.forEach. This should make it so only tiles put in an enhanced list of tiles are allowed to enhance history.

This makes it so we don't need to remove all the enhanced related code from the rest of this patch.

Comment 5

[Ed Lee :Mardak]

(In reply to Marina Samuel [:emtwo] from comment #3)
> Also, when a suggested and directory tile have the same url, the properties
> for the suggested tile were not getting updated in
> NewTabUtils.links.onLinkChanged().
Can you file a bug for this? I think we'll be safe in that suggested links are probably different from directory links.

Comment 6

[Marina Samuel [:emtwo]]

(In reply to Ed Lee :Mardak from comment #5)
> (In reply to Marina Samuel [:emtwo] from comment #3)
> > Also, when a suggested and directory tile have the same url, the properties
> > for the suggested tile were not getting updated in
> > NewTabUtils.links.onLinkChanged().
> Can you file a bug for this? I think we'll be safe in that suggested links
> are probably different from directory links.

This patch has the relevant change for this issue.

Comment 7

[Marina Samuel [:emtwo]]

Attachment: v2: Only enhance tiles that are under the 'enhanced' key

Comment 8

[Ed Lee :Mardak]

Comment on attachment 8586485 [details] [diff] [review]
v2: Only enhance tiles that are under the 'enhanced' key

>+++ b/browser/base/content/test/newtab/browser_newtab_enhanced.js
>@@ -1,19 +1,32 @@
> /* Any copyright is dedicated to the Public Domain.
>    http://creativecommons.org/publicdomain/zero/1.0/ */
> 
> const PRELOAD_PREF = "browser.newtab.preload";
> 
> gDirectorySource = "data:application/json," + JSON.stringify({
>   "directory": [{
>-    url: "http://example.com/",
The example.com url is significant because the test creates a history item   yield setLinks("-1");

>-  is(getData(1), null, "history link pushed out by directory link");
This check is somewhat unclear in that it's actually testing that a 1000 frecency directory link pushes out / replaces the history tile.

>+  ({type, enhanced, title} = getData(1));
>+  isnot(type, "enhanced", "history link is not enhanced");
>+  is(enhanced, "", "history link has no enhanced image");
>+  is(title, "site#-1");
The 1th tile ends up not getting enhanced because your sample enhanced link data doesn't match the example.com domain of the history link. The more appropriate test would have the enhanced link's url be example.com.

>+++ b/browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
>-add_task(function test_DirectoryLinksProvider_getEnhancedLink() {
>-  let data = {"directory": [
>-    {url: "http://example.net", enhancedImageURI: "data:,net1"},
>-    {url: "http://example.com", enhancedImageURI: "data:,com1"},
>-    {url: "http://example.com", enhancedImageURI: "data:,com2"},
>-  ]};
>-  // Get the expected image for the same site
>-  checkEnhanced("http://example.net/", "data:,net1");
>-  checkEnhanced("http://example.net/path", "data:,net1");
>-  checkEnhanced("https://www.example.net/", "data:,net1");
>-  checkEnhanced("https://www3.example.net/", "data:,net1");
Any reason for the removal of these sets of checks? These are making sure history links that match the enhanced link's domain get the expected enhanced image. I would think the test could be updated by setting data = {"enhanced": [{url:example.net, enhancedImageURI:data:,net1},..]}

Comment 9

[Marina Samuel [:emtwo]]

Attachment: v3: Only enhance tiles that are under the 'enhanced' key

* Updated test_DirectoryLinksProvider_getEnhancedLink() instead of deleting
* Reverted mochitest to use example.com and test for enhanced history links as it did prior to bug 1136203

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 8586856 [details] [diff] [review]
v3: Only enhance tiles that are under the 'enhanced' key

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

r+ with the reverts of the two NewTabUtils changes we talked about on IRC.

Comment 11

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/895cf25d11fd

Comment 12

[Ed Lee :Mardak]

http://hg.mozilla.org/releases/mozilla-aurora/rev/4da1a3e49fc1

Comment 13

[Ed Lee :Mardak]

https://hg.mozilla.org/releases/mozilla-beta/rev/311733df5675

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/895cf25d11fd

Comment 15

[Ed Lee :Mardak]

verify steps (at least for nightly 40 en-us):

1) load https://www.mozilla.org/en-US from location bar
2) make sure new tab page shows a thumbnail for that history tile
(as opposed to showing the manifesto tile with sponsored as attachment 8554579 [details])

Comment 16

[Cornel Ionce [:noni] [Hubs QA]]

Verified using the steps provided in the above comment.
The newtab page now displays a thumbnail of the history tile.

Tested on Nightly 40.0a1 (build ID: 20150407030207), Aurora 39.0a2 (build ID: 20150408004004) and Firefox 38 beta 2 (build ID: 20150406174117).