Closed Bug 1241390 Opened 8 years ago Closed 7 years ago

Remove Suggested Tiles and related features

Categories

(Firefox :: New Tab Page, defect)

Product:
Firefox
Component:
New Tab Page
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox46 --- affected
firefox55 --- fixed

People

(Reporter: oyiptong, Assigned: Mardak)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1241390 - Remove Suggested Tiles and related features.
59 bytes, text/x-review-board-request
adw
: review+
emtwo
: review+
Details 
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?
Assignee: nobody → edilee
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?
Flags: needinfo?(edilee)
emtwo says she can review tomorrow.

tspurway, can you provide the confirmation in this bug that the suggested tiles related code can be removed?
Flags: needinfo?(edilee) → needinfo?(tspurway)
Attachment #8864002 - Flags: review?(msamuel)
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.
Flags: needinfo?(tspurway)
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?
Attachment #8864002 - Flags: review?(msamuel) → review+
(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
Attachment #8864002 - Flags: review?(adw) → review+
(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
Blocks: 1314619, 1244406, 1244713, 1255288, 1244285
https://hg.mozilla.org/mozilla-central/rev/09efa7fe3482
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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
Blocks: 1370930
Blocks: 1329955
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: