1370930 - DirectoryLinksProvider is started too early during startup

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement)

Description

[Florian Quèze [:florian]]

DirectoryLinksProvider.init() is currently called from _finalUIStartup in nsBrowserGlue.js. This is before the first window browser is created and before command line handlers have been invoked.

When it's the first startup (ie. we are creating the profile), initializing DirectoryLinksProvider makes it download a JSON file, which forces initialization of NSS on the main thread. See this profile where this takes 451ms before first paint on the quantum reference hardware: https://perfht.ml/2qX4KZo

Comment 1

[:Gijs (he/him)]

:Mardak, does AS still use this? I think it's only the "old" about:newtab that uses it, right? I don't see any consumers under the AS add-on directory (or anywhere else, really).

So maybe we can just get rid of it altogether? Is there a bug on file about removing the old about:newtab now that the prefs are gone? I didn't see it in the deps of bug 1433324...

Given the perf focus, I expect I might just rip this out independently from anything else, if possible. Does that sound OK?

Comment 2

[Ed Lee :Mardak]

I believe it's unused, although it's not as simple as doing a search for DirectoryLinksProvider. In particular NewTabUtils.links.addProvider(DirectoryLinksProvider); modifies the behavior of NewTabUtils via what links it returns and related events. I did end up removing one caller of NewTabUtils.links.getLinks in bug 1433324 https://hg.mozilla.org/mozilla-central/rev/088e727e5cf7#l2.102

I also removed quite a bit of the DirectoryLinksProvider code in bug 1241390.

It looks like a web extension can specifically request a provider's link, but I don't think it could have specifically requested DirectoryLinksProvider anyway? darktrojan, https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-topSites.js#20-24 ?

ursula, any additional thoughts?

Comment 3

[:Gijs (he/him)]

(In reply to Ed Lee :Mardak from comment #2)
> It looks like a web extension can specifically request a provider's link,
> but I don't think it could have specifically requested
> DirectoryLinksProvider anyway? darktrojan,
> https://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> ext-topSites.js#20-24 ?

ISTM that code should ideally be switched to rely on AS's top sites anyway, probably fetching them the same way the Library menu code does?

Comment 4

[Ursula Sarracini (:ursula)]

I can't think of any reason not to just delete the DirectoryLinksProvider code altogether at this point.

Comment 5

[Geoff Lankow (:darktrojan)]

I don't think it's possible to access the DirectoryLinksProvider from a web extension.

Comment 6

[:Gijs (he/him)]

Useless (non-mainthread, thankfully) IO and jsm load on startup, plus what seems like a trivial patch, let's just make this P1 and get it over with... :-)

Comment 7

[:Gijs (he/him)]

Attachment: Bug 1370930 - remove DirectoryLinksProvider,

Review commit: https://reviewboard.mozilla.org/r/224362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224362/

Comment 8

[Ed Lee :Mardak]

Comment on attachment 8955196 [details]
Bug 1370930 - remove DirectoryLinksProvider,

https://reviewboard.mozilla.org/r/224362/#review230362

Could clean up a little bit more although it the additional stuff is probably only really affecting tests. So if you want to just note that in bug 1433133 to properly clean up prefs, that's fine too.

::: browser/base/content/test/newtab/head.js:5
(Diff revision 1)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  const PREF_NEWTAB_ENABLED = "browser.newtabpage.enabled";
>  const PREF_NEWTAB_DIRECTORYSOURCE = "browser.newtabpage.directory.source";

Could additionally clean up things related to "browser.newtabpage.directory.source", e.g., defaults (firefox.js) / overrides (in this file, testing / reftest / talos)

Comment 9

[:Gijs (he/him)]

Comment on attachment 8955196 [details]
Bug 1370930 - remove DirectoryLinksProvider,

https://reviewboard.mozilla.org/r/224362/#review230380

::: browser/components/nsBrowserGlue.js:1820
(Diff revision 1)
>      }
>    },
>  
>    // eslint-disable-next-line complexity
>    _migrateUI: function BG__migrateUI() {
> -    const UI_VERSION = 62;
> +    const UI_VERSION = 63;

Self-nit: bump to bug 1370930 because of impending release changes that will bump to 63.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8955196 [details]
Bug 1370930 - remove DirectoryLinksProvider,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224362/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87d555dea4f4
remove DirectoryLinksProvider, r=Mardak

Comment 12

[:Gijs (he/him)]

(In reply to Ed Lee :Mardak from comment #8)
> Could clean up a little bit more although it the additional stuff is
> probably only really affecting tests. 

Tests that are disabled, right, because the pref is gone? :-)

> So if you want to just note that in
> bug 1433133 to properly clean up prefs, that's fine too.


I removed it in here. Is bug 1433133 just basically remove the newtab directory and all the pieces that hook up to it? Do we want to land that for 60 or are we deliberately waiting? My understanding is that, with the AS pref gone, it's impossible to switch back so I assume it's just dead code at this point. I might volunteer to remove it if nobody else is doing it soon...

Comment 13

[Ed Lee :Mardak]

Yup bug 1433133 is for removing newtab (browser/base/content/newtab related stuff) whereas bug 1409054 is for browser/base/content/abouthome. Both should just be dead code and could be removed in 60 but been busy with feature work.

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/87d555dea4f4