1374686 - Lazily load LoginManagerContextMenu and SpellCheckHelper in nsContextMenu.js

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Matthew N. [:MattN]]

browser_startup.js made it obvious that we're importing LoginManagerContextMenu.jsm before the first context menu opening which isn't necesssary.

Comment 1

[Matthew N. [:MattN]]

Attachment: Bug 1374686 - Don't import LoginManagerContextMenu.jsm during startup.

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

Comment 2

[Matthew N. [:MattN]]

Attachment: Bug 1374686 - Lazily import SpellCheckHelper in nsContextMenu.js.

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

Comment 3

[Matthew N. [:MattN]]

I didn't add a test for SpellCheckHelper since I don't know enough about it to know when it should load from other callers but it didn't hurt to make it lazy here anyways. It did remove it from browser_startup on my local machine though.

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8879607 [details]
Bug 1374686 - Don't import LoginManagerContextMenu.jsm during startup.

https://reviewboard.mozilla.org/r/150942/#review155744

::: browser/base/content/nsContextMenu.js:16
(Diff revision 1)
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "LoginHelper",
>    "resource://gre/modules/LoginHelper.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "LoginManagerContextMenu",

It seems that making this a lazy getter will load it only when needed while showing the context menu (there's a check at http://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/browser/base/content/nsContextMenu.js#605 ), but will load it unconditionally when hiding the context menu (at http://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/browser/base/content/nsContextMenu.js#149 ).

Should we add a check in the hiding method? Could be done using Cu.isModuleLoaded.

::: browser/base/content/test/performance/browser_startup.js:77
(Diff revision 1)
>        "resource:///modules/AboutNewTab.jsm",
>        "resource:///modules/DirectoryLinksProvider.jsm",
>        "resource://gre/modules/BookmarkHTMLUtils.jsm",
>        "resource://gre/modules/Bookmarks.jsm",
>        "resource://gre/modules/ContextualIdentityService.jsm",
> +      "resource://gre/modules/LoginManagerContextMenu.jsm",

If the patch is making this completely disappear from the test's output, you want to blacklist the module for the "before handling user events" phase.

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8879608 [details]
Bug 1374686 - Lazily import SpellCheckHelper in nsContextMenu.js.

https://reviewboard.mozilla.org/r/150944/#review155752

Looks good, thanks!

I have a vague plan to load nsContextMenu.js lazily in the future, but I haven't experimented with that yet, so I'm not sure it'll actually happen.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8879607 [details]
Bug 1374686 - Don't import LoginManagerContextMenu.jsm during startup.

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

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8879608 [details]
Bug 1374686 - Lazily import SpellCheckHelper in nsContextMenu.js.

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

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8879607 [details]
Bug 1374686 - Don't import LoginManagerContextMenu.jsm during startup.

https://reviewboard.mozilla.org/r/150942/#review155916

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/838dc4a2ed61
Don't import LoginManagerContextMenu.jsm during startup. r=florian
https://hg.mozilla.org/integration/autoland/rev/798a5d133c0b
Lazily import SpellCheckHelper in nsContextMenu.js. r=florian

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/838dc4a2ed61
https://hg.mozilla.org/mozilla-central/rev/798a5d133c0b