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

Only enhance tiles that are explicitly enhanceable

VERIFIED FIXED in Firefox 38

Status

()

Firefox
New Tab Page
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Mardak, Assigned: emtwo)

Tracking

Trunk
Firefox 40
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox38 verified, firefox39 verified, firefox40 verified)

Details

(Whiteboard: .002)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

3 years ago
Blocks: 1105365
(Reporter)

Updated

3 years ago
Blocks: 1124045
(Reporter)

Updated

3 years ago
Blocks: 1125874
(Reporter)

Updated

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

Updated

3 years ago
Depends on: 1136203
(Reporter)

Comment 2

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

Updated

3 years ago
Assignee: nobody → msamuel
(Assignee)

Comment 3

3 years ago
Created attachment 8586207 [details] [diff] [review]
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().
Attachment #8586207 - Flags: review?(adw)
Attachment #8586207 - Flags: feedback?(edilee)
(Reporter)

Comment 4

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

Comment 5

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

Comment 6

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

Updated

3 years ago
Attachment #8586207 - Flags: review?(adw)
(Assignee)

Comment 7

3 years ago
Created attachment 8586485 [details] [diff] [review]
v2: Only enhance tiles that are under the 'enhanced' key
Attachment #8586207 - Attachment is obsolete: true
Attachment #8586485 - Flags: review?(adw)
Attachment #8586485 - Flags: feedback?(edilee)
(Reporter)

Comment 8

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

Comment 9

3 years ago
Created attachment 8586856 [details] [diff] [review]
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
Attachment #8586485 - Attachment is obsolete: true
Attachment #8586485 - Flags: review?(adw)
Attachment #8586856 - Flags: review?(adw)

Comment 10

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

Updated

3 years ago
Blocks: 1148859
(Assignee)

Comment 11

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/895cf25d11fd
(Reporter)

Comment 12

3 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/4da1a3e49fc1
Iteration: --- → 40.1 - 13 Apr
Points: --- → 5
status-firefox39: --- → fixed
Flags: firefox-backlog+
Whiteboard: .? → .002
(Reporter)

Comment 13

3 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/311733df5675
status-firefox38: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/895cf25d11fd
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40

Updated

3 years ago
Flags: qe-verify?
(Reporter)

Comment 15

3 years ago
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).
Status: RESOLVED → VERIFIED
status-firefox38: fixed → verified
status-firefox39: fixed → verified
status-firefox40: fixed → verified

Updated

3 years ago
Blocks: 1155443

Updated

3 years ago
No longer blocks: 1155443
You need to log in before you can comment on or make changes to this bug.