There are a few reasons I'd like to do this:

• these functions (all very similarly named and somewhat confusing at this point) clutter up the global scope
• openLinkIn is something like 500 lines long and has an eslint complexity rule disabled because it's massive, but factoring out that complexity in-place would add yet more globals.
• these functions living in a per-window script is annoying for non-window-based code that wants to open URLs. Being able to call them from modules would help with reusing code (see also bug 1485961)
• I'd like to (not in this bug) change some of these functions to also be able to take an nsIURI instead of just a string (so we can call loadURI instead of fixupAndLoadURIString on the other end, cf. bug 1810141). Because it's JS-only, adding the polymorphism is relatively straightforward -- but adding yet more complexity to these methods without being able to do clearer separation of concerns is really yucky. Having a dedicated module makes it easier to have the helper methods not be [inadvertently] exposed as API surface. These functions already create URIs internally, so having them work out the final URI (if an nsIURI doesn't get passed in) seems like a useful abstraction. See also the performance profile in bug 1800596 comment 27.

Initially I'll keep the utilityOverlay.js functions as simple forwarding calls to the module-based implementations. I'll file a follow-up to rewrite all the callers with jscodeshift.

openLinkIn would really benefit from being split up a bit, and adding more
globals to the browser window is icky. Also, the story for opening new tabs if
you're not inside a window is a nightmare right now. Moving this code
to a module is a first step to making that story nicer.

I kept wrappers for all the functions I'm moving, and added the window as the
first argument. In the future we can update these functions to support being
called without a window ref. The one exception is getTopWin, where I updated
the callers in this patch.

I had to tweak the parameter detection of the different arguments supported by
openUILinkIn because forwarding calls means arguments.length is always larger
than 3. This brought it in line with the other methods anyway.

When splitting out the window/tab/current cases, not all of them need all
these individual arguments. But it's hard to reason about which ones are used
where/how when we rename them all the time, and passing them all
individually is more verbose as a result of the renaming, so let's just not do
that at all.

There are a few "odd ones out" - private isn't a valid local variable name,
and some of the other items were easier to deal with by just referencing
directly on params.

Depends on D170210

Depends on D170211

'fromChrome' really meant "force tabs to open in the foreground", so let's
rename it accordingly.

This removes the attempt to document arguments for openUILinkIn.
I'll add documentation back on the end of this stack, for openLinkIn, when
various bits are reorganized anyway.

Depends on D170210

Depends on D170212

Backed out 5 changesets (bug 1817443) for newtab failure

Backout: https://hg.mozilla.org/integration/autoland/rev/8d30527db5c579eedec7068028f0e2307141c8ab

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=ba33c510d0080eeb626305fbd2b30ad975e43102&selectedTaskRun=CX34Bg9TTK2OL-TYTUHEqQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=406685191&repo=autoland&lineNumber=114

(In reply to Cristina Horotan [:chorotan] from comment #7)

Backed out 5 changesets (bug 1817443) for newtab failure

Backout: https://hg.mozilla.org/integration/autoland/rev/8d30527db5c579eedec7068028f0e2307141c8ab

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=ba33c510d0080eeb626305fbd2b30ad975e43102&selectedTaskRun=CX34Bg9TTK2OL-TYTUHEqQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=406685191&repo=autoland&lineNumber=114

Boo. Didn't realize newtab unit tests mocked up all these calls. That's annoying. Relanding with that fixed.

https://hg.mozilla.org/mozilla-central/rev/44fe1f033770
https://hg.mozilla.org/mozilla-central/rev/c1267fbfcb92
https://hg.mozilla.org/mozilla-central/rev/c7bdd75a316b
https://hg.mozilla.org/mozilla-central/rev/c25910781954
https://hg.mozilla.org/mozilla-central/rev/1e3a192261a8