Suggested Tiles were a feature of the Newtab page whose goal was to show an image and metadata to a user based on their frecent tiles. Some of those images were paid-for advertisements. In order to display advertisement, some additional features were implemented, including frequency capping and "inadjacency". We are no longer continuing Suggested Tiles or paid advertisement. This bug is about removing the functionality related to Suggested Tiles and advertising.

I just spotted this bug today after tracking down the ping removal from bug 1240245. I suppose this removal is a bit overdue, so I'll remove! Just confirming that suggested and enhanced tiles are not being used and all the related functionality should be gone?

As noted on irc, nanj said the only tiles that have been published are the affiliate Mozilla tiles and functionality for suggested tiles on the server side had already been removed.

is there an update on this bug? possibly ready to land a patch?

emtwo says she can review tomorrow. tspurway, can you provide the confirmation in this bug that the suggested tiles related code can be removed?

Just some random stats from blame for before and after removal.. ;) $ git blame browser/modules/DirectoryLinksProvider.jsm | cut -d'(' -f2 | cut -d2 -f1 | sort | uniq -c | sort -n > before 95 Olivier Yiptong 175 Ed Lee 260 Marina Samuel 613 Maxim Zhilyaev and after removal: 53 Marina Samuel 88 Olivier Yiptong 104 Maxim Zhilyaev 110 Ed Lee

LGTM, Ed. Suggested Tiles are no longer a thing, and I can't think of how they would ever be used.

Comment on attachment 8864002 [details] Bug 1241390 - Remove Suggested Tiles and related features. https://reviewboard.mozilla.org/r/135724/#review141162 YAY FOR REMOVING LOTS OF CODE!! ::: browser/app/profile/firefox.js:1224 (Diff revision 1) > > // Toggles the content of 'about:newtab'. Shows the grid when enabled. > pref("browser.newtabpage.enabled", true); > > // Toggles the enhanced content of 'about:newtab'. Shows sponsored tiles. > sticky_pref("browser.newtabpage.enhanced", true); Probably don't need this too, right? ::: browser/base/content/newtab/grid.js:275 (Diff revision 1) > // exactly on the column boundary > this._node.style.width = gridColumns * this._cellWidth + "px"; > this._node.style.maxWidth = gGridPrefs.gridColumns * this._cellWidth + > GRID_WIDTH_EXTRA + "px"; > this._node.style.height = this._computeHeight() + "px"; > this._node.style.maxHeight = this._computeHeight(gridRows) - SPONSORED_TAG_BUFFER + "px"; Do we still need this SPONSORED_TAG_BUFFER? ::: browser/base/content/newtab/page.js:254 (Diff revision 1) > } > }, > > onPageVisibleAndLoaded() { > - // Send the index of the last visible tile. > - this.reportLastVisibleTileIndex(); > + // This no longer reports actions, but is used to trigger data refresh > + DirectoryLinksProvider.reportSitesAction(); Nit: Can we directly call DirectoryLinksProvider._fetchAndCacheLinksIfNecessary() since reportSitesAction() is just doing that now? Or at the very least give it a more accurate name so we don't need to have that comment? ::: browser/docs/DirectoryLinksProvider.rst:7 (Diff revision 1) > Directory Links Architecture and Data Formats > ============================================= > > Directory links are enhancements to the new tab experience that combine content > -Firefox already knows about from user browsing with external content. There are > -3 kinds of links: > +Firefox already knows about from user browsing with external content. There is 1 > +kind of links: nit: s/links/link ::: browser/docs/DirectoryLinksProvider.rst:27 (Diff revision 1) > > > Preferences > =========== > > -There are two main preferences that control downloading links and reporting > +There is one main preference that control downloading links. nit: s/control/controls ::: browser/locales/en-US/chrome/browser/newTab.dtd:8 (Diff revision 1) > - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > > <!-- These strings are used in the about:newtab page --> > <!ENTITY newtab.pageTitle "New Tab"> > <!ENTITY newtab.customize.classic "Show your top sites"> > <!ENTITY newtab.customize.cog.enhanced "Include suggested sites"> Missed this one! ::: browser/modules/DirectoryLinksProvider.jsm:264 (Diff revision 1) > - * @param sites Array of sites shown on newtab page > - * @param action String of the behavior to report > - * @param triggeringSiteIndex optional Int index of the site triggering action > * @return download promise > */ > - reportSitesAction: function DirectoryLinksProvider_reportSitesAction(sites, action, triggeringSiteIndex) { > + reportSitesAction() { I mentioned this earlier. Wondering if we can get rid of reportSitesAction() and call fetchAndCacheLinksIfNecessary() directly?

(In reply to Marina Samuel [:emtwo] from comment #10) > Comment on attachment 8864002 [details] > ::: browser/app/profile/firefox.js:1224 > > // Toggles the enhanced content of 'about:newtab'. Shows sponsored tiles. > > sticky_pref("browser.newtabpage.enhanced", true); > Probably don't need this too, right? I can change the comment, but this is still used for directory tiles albeit poorly named. > ::: browser/base/content/newtab/grid.js:275 > > this._node.style.maxHeight = this._computeHeight(gridRows) - SPONSORED_TAG_BUFFER + "px"; > Do we still need this SPONSORED_TAG_BUFFER? I did see that and decided not to given how it affects layout of the page. I can play around with it to see if it breaks resizing. > ::: browser/base/content/newtab/page.js:254 > > onPageVisibleAndLoaded() { > > - // Send the index of the last visible tile. > > - this.reportLastVisibleTileIndex(); > > + // This no longer reports actions, but is used to trigger data refresh > > + DirectoryLinksProvider.reportSitesAction(); > Nit: Can we directly call > DirectoryLinksProvider._fetchAndCacheLinksIfNecessary() since > reportSitesAction() is just doing that now? Or at the very least give it a > more accurate name so we don't need to have that comment? I think that's reasonable to do. Initially as I removed things, I was concerned about multiple callers of reportSitesAction from code and tests, but there aren't actually any other callers. :p > ::: browser/docs/DirectoryLinksProvider.rst:7 > > Directory links are enhancements to the new tab experience that combine content > > +Firefox already knows about from user browsing with external content. There is 1 > > +kind of links: > nit: s/links/link Wellll... there are multiple links provided by directory tiles, but there's only one kind. ;) > ::: browser/docs/DirectoryLinksProvider.rst:27 > > -There are two main preferences that control downloading links and reporting > > +There is one main preference that control downloading links. > nit: s/control/controls Argh grammar! :p > ::: browser/locales/en-US/chrome/browser/newTab.dtd:8 > (Diff revision 1) > > <!ENTITY newtab.customize.cog.enhanced "Include suggested sites"> > Missed this one! As per the enhanced pref comment, this behavior controls showing directory tiles or not. (We used to have separate prefs for each type, but they all got merged to this single "enhanced" pref.) > ::: browser/modules/DirectoryLinksProvider.jsm:264 > > - reportSitesAction: function DirectoryLinksProvider_reportSitesAction(sites, action, triggeringSiteIndex) { > > + reportSitesAction() { > I mentioned this earlier. Wondering if we can get rid of reportSitesAction() > and call fetchAndCacheLinksIfNecessary() directly? Yup! Sounds good.

Comment on attachment 8864002 [details] Bug 1241390 - Remove Suggested Tiles and related features. https://reviewboard.mozilla.org/r/135724/#review141302 Pour one out folks

(In reply to Ed Lee :Mardak from comment #11) > (In reply to Marina Samuel [:emtwo] from comment #10) > > Comment on attachment 8864002 [details] > > ::: browser/app/profile/firefox.js:1224 > > > // Toggles the enhanced content of 'about:newtab'. Shows sponsored tiles. > > > sticky_pref("browser.newtabpage.enhanced", true); > > Probably don't need this too, right? > I can change the comment, but this is still used for directory tiles albeit > poorly named. Ok let's just change the comment then. > > ::: browser/base/content/newtab/grid.js:275 > > > this._node.style.maxHeight = this._computeHeight(gridRows) - SPONSORED_TAG_BUFFER + "px"; > > Do we still need this SPONSORED_TAG_BUFFER? > I did see that and decided not to given how it affects layout of the page. I > can play around with it to see if it breaks resizing. Ah don't worry about it if it's tricky, maybe a diff variable name or something. > > ::: browser/locales/en-US/chrome/browser/newTab.dtd:8 > > (Diff revision 1) > > > <!ENTITY newtab.customize.cog.enhanced "Include suggested sites"> > > Missed this one! > As per the enhanced pref comment, this behavior controls showing directory > tiles or not. (We used to have separate prefs for each type, but they all > got merged to this single "enhanced" pref.) Oh! Gotcha! I missed that.

Comment on attachment 8864002 [details] Bug 1241390 - Remove Suggested Tiles and related features. https://reviewboard.mozilla.org/r/135724/#review141162 > Nit: Can we directly call DirectoryLinksProvider._fetchAndCacheLinksIfNecessary() since reportSitesAction() is just doing that now? Or at the very least give it a more accurate name so we don't need to have that comment? Ended up removing the call as directory links are rarely updated, so relying on init to check for updates is sufficient.

Pushed by edilee@gmail.com: https://hg.mozilla.org/integration/autoland/rev/09efa7fe3482 Remove Suggested Tiles and related features. r=adw,emtwo

Marking a number of bugs that are intermittents of tests that are being removed by this bug. jmaher, I also triggered 50 runs of each browser-chrome that runs the newtab/ tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fa6bf712b71&filter-searchStr=browser-chrome&group_state=expanded&selectedJob=98324113

I see performance improvements from this patch: == Change summary for alert #6521 (as of May 11 2017 00:33 UTC) == Improvements: 3% tp5o summary linux64 opt 338.63 -> 327.02 3% tsvg_static summary windows7-32 opt 76.62 -> 74.36 2% tp5o summary windows8-64 opt e10s311.11 -> 303.92 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6521