1406778 - Lazily load the font inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1406778 - Lazily load the font inspector.

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

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Alex, you will noticed that I removed the pref check for the font inspector enabled. I don't think we are ever gonna try and disable this again, so I am removing that pref check. I think there might be some extra work where we can also remove the check for the canGetUsedFontFaces, but I am gonna save that for another bug.

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8917962 [details]
Bug 1406778 - Lazily load the font inspector.

https://reviewboard.mozilla.org/r/188858/#review194186

Thanks, I completely forgot about that one...

You should also remove the pref from:
http://searchfox.org/mozilla-central/source/devtools/client/preferences/devtools.js
http://searchfox.org/mozilla-central/source/devtools/client/inspector/fonts/test/head.js
http://searchfox.org/mozilla-central/source/devtools/client/inspector/inspector.js#624

::: devtools/client/inspector/inspector.js:674
(Diff revision 1)
>      }
>  
> -    if (Services.prefs.getBoolPref("devtools.fontinspector.enabled") &&
> -        this.canGetUsedFontFaces) {
> -      const FontInspector = this.browserRequire("devtools/client/inspector/fonts/fonts");
> +    if (this.canGetUsedFontFaces) {
> +      let fontId = "fontinspector";
> +      let fontTitle = INSPECTOR_L10N.getStr("inspector.sidebar.fontInspectorTitle");
> +      this.sidebar.addTab(

May be you could copy paste comment from line 640 to help understanding this non obvious lazy loading setup.

Comment 4

[Alexandre Poirot [:ochameau]]

Oh BTW, could you push this change to talos before pushing, to see if that reports any win?
  ./mach try -b do -p linux64 -u none -t g2-e10s --rebuild-talos 6

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a03637b229b6059844c78b141347ea0961573daf

Talos
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39c1b994815b1557627e7a8a85836f7c07b873ae
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=39c1b994815b1557627e7a8a85836f7c07b873ae

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb6f23916cb1
Lazily load the font inspector. r=ochameau

Comment 7

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2559f86f67f6 for eslint bustage, https://treeherder.mozilla.org/logviewer.html#?job_id=136698483&repo=mozilla-inbound

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/193014cd4fd4
Lazily load the font inspector. r=ochameau

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/193014cd4fd4

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #5)
> Talos
> https://treeherder.mozilla.org/perf.html#/
> comparechooser?newProject=try&newRevision=39c1b994815b1557627e7a8a85836f7c07b
> 873ae

DAMP reported a win with medium confidence on inspector.open as the win was around the variance of these tests.
Variance is typically around 1-3% and the win here seems to be around 2-3%.

The long term graph confirmed a ~3% win on complicated inspector open:
https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,1418016,1,1