When a user pins a Suggested Tile, the following functionality is desired: - remove [SUGGESTED] label - remove contextual explanation text - remove overlay disclaimer when clicked on the [SUGGESTED] label - if a user changes new tab option from "Show suggested sites" to "Show top sites" the pinned Suggested Tile will remain on the new tab page

Additionally, when the user pins a Suggested Tile, the same Tile should not appear. Perhaps this can be done via the same frequency click capping mechanism mentioned on bug 1140496.

Additionally, we should consider: - this should apply to Directory Tiles becoming a pinned and history tile - after removing the sponsored tile, we should consider reverting back to a screenshot tile or a log tile if it's supplied by client

What's a log tile?

*logo

A logo is just an image for a directory tile, so I don't understand how you want a pinned suggested/directory tile to revert to a screenshot or the logo because it was originally a logo. ?

When we receive creative assets from the client, we can ask for the sponsored tile specific to a campaign as well as a default logo tile. When the sponsorship campaign end date is reached, the tile display the logo tile.

End of campaign is separate from the user pinning the tile. Those are two completely different events. What you've described is if the user pins the tile, it should become a history tile, so we shouldn't change around the user's history tile. Although this could be confusing in that the user could have pinned the tile because of the image, and having it change to something else would be unexpected and undesired.

Yes, we should account for both. 1) End of campaign, 2a) user pin, 2b) user pin then end of campaign. If a user pins a sponsored tile and the campaign is over, we should be careful of the contractual agreement. We can involve legal with this in structuring safeguards against user-involved actions. So, I agree with you from a user's perspective, but for commercial partners, there could be legal reasons why they wouldn't want their ad to continue to be displayed. For example, time sensitive campaigns like tax date, or promotional deals that end on a certain date, or movie air dates, etc.

Geoff, I think you should be aware of these types of scenarios.

Yeah, the pinning during campaign that continues after the campaign end date brings about a lot of legal challenges that we need to discuss more deeply. Can we set time limits on the creative such that upon end date of the campaign, the Tile is still pinned however replaced with a history tile? There are significant business and legal reasons from the agreements that make this risky probably for both Mozilla and the partner.

Is this something we want for 39? If so, we need to define what pinning a tile is supposed to do. Currently a tile has its title, url, and up to 2 images. If you want it to switch urls or switch images or switch titles -- those all require sending additional data to Firefox as well as making server changes to store those extra values and getting those values from the client in the first place. I.e., the client might need to provide 2 titles, 2 urls, 4 images. If creative is the biggest issue, a simple fix is to just not use the server provided image and default to the usual thumbnail that history tiles have. However, the pinned tile would still be going to the original url. There's a lot of questions, so I would propose moving this out of 39.

As discussed, we can do something simple initially without making strong guarantees to partners. To avoid needing extra data from the servers and limiting scope, we'll only do something special to pinned tiles that were suggested with an end time (from bug 1156549). The behavior is that a pinned suggestion will stay as is until its end time at which point the custom images are removed (resulting in normal thumbnailing) and url reverts to the root page. So for example assuming a suggested tile with end date, a pinned https://marketplace.firefox.com/feed/collection/puzzle-gamesdesktop-essentials?src=collection-element with custom image/rollover with tile id 1095 becomes a regular pinned history tile with no tile id, url of https://marketplace.firefox.com/ and no imageURI/enhancedImageURI. For implementing this, pinned tiles no longer are handled by DirectoryLinksProvider although we do get a callback when the new tab page is shown.

This sounds right to me and should be how we handle pinned tiles in general (i.e., that is as a matter of trafficking and to be correct under the IO Start to End Date). So if I understand correctly, and in my less then sophisticated technical speak, upon the occurrence of the End Date for a pinned Tile that s subject to an IO, that pinned Tile will turn into a standard History Tile. Right?

Talked to adw. He feels timeout in NewTabUtils.js is an overkill. When the campaign runs out of time, the newtab will display a newtab with incorrect pinned tile at most once. Here's exact sequence of events: 1. Suggested tile is pinned and turned into a history tile 2. Suggested tile still retains partner landing page url and partner background image 3. When rendering a tile FX checks if endTime is passed, if so it resets landing url and removes background image. 4. After campaign runs out, rendering logic will display the tile as if it came from history 5 However! FX will display preloaded new tab page once before it replaces preloaded page with newly rendered one. So, a user may see incorrect image and observe wrong landing url only once after the campaign is over. We may partially help that by setting timeouts for the end of campaign, but this will not work if a user restarts FX before campaign ends. Having persistent timeouts just to fix that hypothetical error case seems unjustified. I would like Mardak/Kevin verify that it's OK to skip timeout implementation.

I think this is fine. I made a similar suggestion for implementing the original endTime to not require timers and just filter out the view on render. Even with the preload, we know when the page is being shown, so we could just have the logic trigger at that point. Basically scan through looking for pinned tiles to see if anything should have been expired then change up the image/url at that point.

implementation details: - any tile (enhanced, suggested, etc..) is turned into history tile when pinned - if a tile has endTime set and it's passed Date.now(), the tile's url is replaced with baseDomain and imageURI is removed to trigger thumbnail loader

Comment on attachment 8608230 [details] [diff] [review] v1. first cut at pinned suggested tile >+ // Bug 1145428 - handle suggested tile campaign end time You don't need to reference the bug. It'll be in the commit log for blame. >+ this.link.url = "http://" + this.link.baseDomain; You should probably grab the protocol from the link instead of assuming http.

Comment on attachment 8608829 [details] [diff] [review] v4. Fixed browser_newtab_reportLinkAction failures Review of attachment 8608829 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/newtab/browser_newtab_reportLinkAction.js @@ +44,5 @@ > // Click the pin button on the link in the 1th tile spot > let siteNode = getCell(1).node.querySelector(".newtab-site"); > let pinButton = siteNode.querySelector(".newtab-control-pin"); > expected.action = "pin"; > + // tiles become "history" when pined nit: pined -> pinned ::: toolkit/modules/NewTabUtils.jsm @@ +500,5 @@ > + * @param aLink The replacement link > + */ > + replace: function PinnedLinks_replace(aUrl, aLink) { > + let index = this._indexOfLink({url: aUrl}); > + if (index == -1) Please wrap with braces @@ +502,5 @@ > + replace: function PinnedLinks_replace(aUrl, aLink) { > + let index = this._indexOfLink({url: aUrl}); > + if (index == -1) > + return; > + links[index] = aLink; I don't see where `links` is declared? Did you mean this.links?

Comment on attachment 8608829 [details] [diff] [review] v4. Fixed browser_newtab_reportLinkAction failures >+ let oldUri = Services.io.newURI(this.url, null, null); >+ this.link.url = oldUri.scheme + "://" + this.link.baseDomain; Fyi, you already have a URI, so you could do oldUri.path = "" and read out the .spec. For example.. < let u = Services.io.newURI("https://bugzilla.mozilla.org/attachment.cgi?id=8608829&action=edit", null, null); > XPCWrappedNative_NoHelper { .. } < u.path = "" > "" < u.spec > "https://bugzilla.mozilla.org/" That might be more correct than using baseDomain which would result in "mozilla.org"

Oh sorry, even easier :p < Services.io.newURI("https://bugzilla.mozilla.org/attachment.cgi?id=8608829&action=edit", null, null).resolve("/") > "https://bugzilla.mozilla.org/"

test is still failing in linux debug, looking into it now. However, we would like a review to land it before weekends

Comment on attachment 8609101 [details] [diff] [review] v5. fixed reviewer comments and some failing tests >+ if (aLink.enhancedImageURI && aLink.imageURI) { >+ // since we display enhancedImage as the background image >+ // and show imageURI on roll-over, replace imageURI with >+ // enhancedImageURI to avoid user surprise >+ aLink.imageURI = aLink.enhancedImageURI; >+ delete aLink.enhancedImageURI; Why do you think it would be surprising to have a pinned tile with rollover images? I would expect the user to want both images, and sometimes the image might not make sense without both of them.

we still observe strange failures on linux debug that are not reproducible on local dev macOS machines.

Comment on attachment 8609220 [details] [diff] [review] v7. fix Mardak requests and added potential fixes to test Review of attachment 8609220 [details] [diff] [review]: ----------------------------------------------------------------- r+ on the non-test parts. As Max and I talked about, I'd prefer to see Directory Tiles-specific logic live only in DirectoryLinksProvider.jsm and not spread out into sites.js (link.endTime, imageURI, enhancedImageURI) and NewTabUtils.jsm (link.targetedSite, whether a link is a "history" link). i.e., the provider interface would be extended so that a link's provider is notified when a tile that represents the link becomes visible and pinned. But it's not a huge deal and it can certainly be improved in a follow up, and I don't want to block you from landing. Max and I also talked about fixing the test failures he's seeing, and he's working on a new patch. ::: browser/base/content/newtab/sites.js @@ +199,5 @@ > this.refreshThumbnail(); > }, > > /** > + * Called when preloaded newtab becomes visible first time Preloading can be disabled, so it's more accurate to say, Called when the site's tab becomes visible for the first time because it either moved out of the preloader or preloading is disabled.

Comment on attachment 8609635 [details] [diff] [review] v8. incorporated all reviewers comments, fixed try-server failures >+++ b/browser/base/content/newtab/page.js > onPageFirstVisible: function () { >- site.captureIfMissing(); >+ // let site handle first visible event >+ site.onFirstVisible(); Repeating what the code does as comments isn't useful. > /** >+ * Checks for and modifies link at campaign end time >+ * - replace landing url with baseDomain url of same protocol >+ * - remove imageURI and enhancedImageURI to trigger tumbnail catcher thumbnail capture >+ * - delete endTime to avoid further checks >+ * - clear enhanced-content backgroundImage >+ * - replace entry in gPinnedLinks with modified link >+ */ >+ _checkLinkEndTime: function Site_checkLinkEndTime() { >+ if (this.link.endTime && this.link.endTime < Date.now()) { >+ let oldUrl = this.url; >+ // chop off the path part from url >+ this.link.url = Services.io.newURI(this.url, null, null).resolve("/"); >+ delete this.link.imageURI; >+ delete this.link.enhancedImageURI; >+ delete this.link.endTime; >+ // clear enhanced-content backgroundImage >+ this._querySelector(".enhanced-content").style.backgroundImage = ""; >+ gPinnedLinks.replace(oldUrl, this.link); Again repeating what the code does as comments isn't useful. > _render: function Site_render() { >+ // first check for end time, as it may modify the link >+ this._checkLinkEndTime(); >+ // setup display vairables variables > /** >+ * Called when the site's tab becomes visible for the first time due to: >+ * - moving out of the preloader >+ * - or when netab is clicked if preloading is disabled newtab >+ */ Commenting on where a method is called from doesn't seem right.

Verified with osx nightly 2015-05-26: 1) new profile 2) update about:config pref browser.newtabpage.directory.source = https://people.mozilla.org/~elee/suggested.1145428.json (if for later verification, you'll need to download the file and manually edit "time_limits": { "end": "2015-05-26T17:15", to be a few minutes after the current time 3) visit enough pages to cause the suggestion to show (at least 8 sites including mozilla sites) 4) see the suggestion [first row of attachment] with hover and static images and [suggested] 5) pin the tile and see [suggested] disappear but retain image and url 6) wait until end time expires and open a new tab page seeing title is reverted to domain, no static or rollover image, and url There is bug 1168655 with the hovered title that shows the original title and new url. There's also bug 1168650 for images being stretched to the incorrect size when pinned.

Comment on attachment 8610022 [details] [diff] [review] for aurora (Mardak will land) Approval Request Comment: See bug 1140185 comment 11

Comment on attachment 8610022 [details] [diff] [review] for aurora (Mardak will land) approval-mozilla-aurora+ granted in bug 1140185 comment 14

Verified on latest Aurora, build ID: 20150528004000. While testing this I've also ran into bug 1168655 and bug 1168650.