Closed Bug 1105360 Opened 5 years ago Closed 5 years ago

Only enhance tiles that are explicitly enhanceable

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.1 - 13 Apr
Tracking Status
firefox38 --- verified
firefox39 --- verified
firefox40 --- verified

People

(Reporter: Mardak, Assigned: emtwo)

References

Details

(Whiteboard: .002)

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 1105365
Blocks: 1124045
Blocks: 1125874
Duplicate of this bug: 1132773
Depends on: 1136203
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.
Blocks: 1120311
Summary: Only enhance tiles that explicitly enhanceable → Only enhance tiles that are explicitly enhanceable
Assignee: nobody → msamuel
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().
Attachment #8586207 - Flags: review?(adw)
Attachment #8586207 - Flags: feedback?(edilee)
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.
Attachment #8586207 - Flags: feedback?(edilee) → feedback-
(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.
(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.
Attachment #8586207 - Flags: review?(adw)
Attachment #8586207 - Attachment is obsolete: true
Attachment #8586485 - Flags: review?(adw)
Attachment #8586485 - Flags: feedback?(edilee)
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},..]}
Attachment #8586485 - Flags: feedback?(edilee)
* 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
Attachment #8586485 - Attachment is obsolete: true
Attachment #8586485 - Flags: review?(adw)
Attachment #8586856 - Flags: review?(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.
Attachment #8586856 - Flags: review?(adw) → review+
Blocks: 1148859
http://hg.mozilla.org/releases/mozilla-aurora/rev/4da1a3e49fc1
Iteration: --- → 40.1 - 13 Apr
Points: --- → 5
Flags: firefox-backlog+
Whiteboard: .? → .002
https://hg.mozilla.org/mozilla-central/rev/895cf25d11fd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Flags: qe-verify?
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])
Flags: qe-verify? → qe-verify+
QA Contact: cornel.ionce
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).
Blocks: 1155443
No longer blocks: 1155443
You need to log in before you can comment on or make changes to this bug.