Closed Bug 1449940 Opened 6 years ago Closed 6 years ago

Merge nsSetDefaultBrowser.js into nsBrowserContentHandler.js

Categories

(Firefox :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: florian, Assigned: Usul)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p2])

Attachments

(1 file, 1 obsolete file)

Like all command line parameter handlers, nsSetDefaultBrowser.js is an xpcom component created at each startup even when the related command line parameter isn't provided by the user on the command line.

Loading the component has overhead. Here is a profile on a slow netbook: https://perfht.ml/2uwo4Da

nsSetDefaultBrowser.js also imports ShellService.jsm synchronously instead of using a lazy getter.

This file is so trivial that we could easily merge it into nsBrowserContentHandler.js which handles the other Firefox-specific command line parameters.
Attached patch 1449940.patch (obsolete) — Splinter Review
Attachment #8963931 - Flags: review?(florian)
Comment on attachment 8963931 [details] [diff] [review]
1449940.patch

Review of attachment 8963931 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. It would be nice to add a line in browser_startup.js to verify that we are no longer importing ShellService.jsm early.

For the next version, please provide 8 lines of context and include a commit message in the patch.

::: browser/components/nsBrowserContentHandler.js
@@ +475,4 @@
>  
>    get helpInfo() {
>      let info =
> +              "  --setDefaultBrowser Set this app as the default browser.\n" +

nit: This list seems mostly alphabetically sorted, I would put this line after the --search one.
Attachment #8963931 - Flags: review?(florian) → feedback+
Assignee: nobody → ludovic
Attachment #8963931 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8964114 - Flags: review?(florian)
Comment on attachment 8964114 [details] [diff] [review]
patch1449940_3.patch

Review of attachment 8964114 [details] [diff] [review]:
-----------------------------------------------------------------

I fixed the obvious issue before pushing to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dfd8a29cbc77c35a7211da3ee5537f0bc19a126 and try seems green.

::: browser/components/nsBrowserContentHandler.js
@@ +478,5 @@
>                "  --browser          Open a browser window.\n" +
>                "  --new-window <url> Open <url> in a new window.\n" +
>                "  --new-tab <url>    Open <url> in a new tab.\n" +
> +              "  --private-window <url> Open <url> in a new private window.\n"
> +              "  --setDefaultBrowser Set this app as the default browser.\n"; 

This change is broken:
- trailing whitespace
- no '+' operator between the 2 lines.
Attachment #8964114 - Flags: review?(florian) → review+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1422e74a0a87
Merge nsSetDefaultBrowser.js into nsBrowserContentHandler.js to remove one xpcom component loaded at startup, r=florian.
https://hg.mozilla.org/mozilla-central/rev/1422e74a0a87
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.