1880914 - Move Browser* helper functions used from global menubar and similar commands to a single object in a separate file, loaded as-needed

Status: RESOLVED FIXED

(Firefox :: General, task, P3)

Description

[:Gijs (he/him)]

A long, long time ago, in a galaxy far away a company called Netscape, someone decided that UI functions for the Browser should all be prefixed Browser and just live in the window scope as global functions.

So now we have BrowserBack, BrowserForward, BrowserRefresh, and... well, you get the picture.

https://searchfox.org/mozilla-central/search?q=%5E%28async+%29%3Ffunction+Browser&path=%2Fbrowser.js&case=true&regexp=true

There are about 20 of these functions. The goal of this bug is straightforward: move those functions to their own file, as methods on a singleton object, which doesn't get loaded until one of those functions is actually called.

To do this, you'd want to:

• decide on the name of the object. I suggest BrowserCommands, as they're often called via command elements.
• create a new JS file that matches the name (but as lower-case-with-dashes, so browser-commands.js in my example), in browser/base/content/
• create a new object there using the name decided above.
• make sure the file is loaded using a lazy script getter, like so:

XPCOMUtils.defineLazyScriptGetter(
  this,
  "BrowserCommands",
  "chrome://browser/content/browser-commands.js"
);

Of course using the object name and file name decided above. You can add the getter somewhere like here: https://searchfox.org/mozilla-central/rev/bf8c7a7d47debb1c22b51a72184d5c703ae57588/browser/base/content/browser.js#111-115

• move all the functions in the query above into the new file, inside the new object
• rename them all as methods on BrowserCommands, using camelCase. So e.g. BrowserBack would look like this:

let BrowserCommands = {
  back(aEvent) {
    // contents of the function goes here
  },
};

• update all the callers of these functions to use their new names (so BrowserCommands.back() or BrowserCommands.refresh or whatever). You can find them by using case-sensitive searchfox queries. This is the one for BrowserBack: https://searchfox.org/mozilla-central/search?q=BrowserBack%28&path=&case=true&regexp=false .

Comment 1

[Yi Xiong Wong]

Hey, I'm interested in working on this bug.

I had a quick look, and there seem to be a few functions that are used in both the Browser* functions and other functions in browser.js, what should be done about them?

Comment 2

[:Gijs (he/him)]

(In reply to Yi Xiong Wong from comment #1)

Hey, I'm interested in working on this bug.

Great! Thanks for your interest in helping make Firefox better. :-)

I had a quick look, and there seem to be a few functions that are used in both the Browser* functions and other functions in browser.js, what should be done about them?

Not 100% sure which functions you ran into, but I expect it will depend a bit on the function.

In terms of the patch "working", you could leave the called functions as-is and it would work (I think), because the lazy script getter will just load the new script file in the same global as browser.js so if the function was available before in that same global scope, it will still be available now. The linter might complain but I think it shouldn't. I could be wrong about that though.

If you wanted to make things neater, some items could be replaced, e.g. whereToOpenLink, getRootEvent and a few others like it are just a forwarding function for methods that exist on BrowserUtils or URILoadingHelper, and could be called directly on those helpers to avoid the confusion. If you'd like to start with something a bit smaller than this bug, you could do some of that part first, in bug 1742889 - it should be more straightforward with no other dependencies.

As a different example, duplicateTabIn is only called from Browser* functions and goToHistoryIndex, which could both also move into the new file together (which helps with grouping similar/related code together, as goToHistoryIndex is only used from the back/forward buttons' menus anyway, similar to BrowserBack and friends). FillHistoryMenu should probably move along with those as well.

Basically, fixing this bug will require being somewhat creative with the "dependency tree" of these functions. It would probably be a good idea to split up any patches to move a few functions at a time, to help keep the problems addressed in a single patch manageable. I'm happy to help with figuring out the details if you get stuck on anything, though I also understand if now that you've looked at it you decide that this is maybe more than you wanted to take on right now. Let me know! :-)

Comment 3

[Yi Xiong Wong]

I'm happy to help with figuring out the details if you get stuck on anything, though I also understand if now that you've looked at it you decide that this is maybe more than you wanted to take on right now. Let me know! :-)

Nah, I still want to work on this. I expected this anyways, refactoring code is never simple. :)

Comment 4

[:Gijs (he/him)]

(In reply to Yi Xiong Wong from comment #3)

I'm happy to help with figuring out the details if you get stuck on anything, though I also understand if now that you've looked at it you decide that this is maybe more than you wanted to take on right now. Let me know! :-)

Nah, I still want to work on this. I expected this anyways, refactoring code is never simple. :)

Heya, how is this going - are you stuck on anything / is there anything I can do to help?

Comment 5

[Yi Xiong Wong]

Hey, I'm almost done with this, just need to double check that I didn't miss anything, then I can submit the patch. These functions are used in quite a few places (and tests) so I've got to make sure those still work.

Comment 6

[Yi Xiong Wong]

Attachment: Bug 1880914 - Refactor BrowserForward into a function in BrowserCommand. r=Gijs

Comment 7

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move BrowserBack into BrowserCommands. r=Gijs

Comment 8

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move BrowserHandle(Shift)Backspace to BrowserCommands. r=Gijs

Comment 9

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move gotoHistoryIndex. r=Gijs

Comment 10

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move all reload functions. r=Gijs

Comment 11

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move BrowserHome. r=Gijs

Comment 12

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move BrowserOpenTab. r=Gijs

Comment 13

[Yi Xiong Wong]

Hmm, it looks like I accidentally removed duplicateTabIn from the globals array. How do I edit a commit to remove that change? hg histedit gives me a warning about content divergence.

Comment 14

[:Gijs (he/him)]

(In reply to Yi Xiong Wong from comment #13)

Thanks for the patches!

Hmm, it looks like I accidentally removed duplicateTabIn from the globals array. How do I edit a commit to remove that change? hg histedit gives me a warning about content divergence.

I'm sorry, I didn't see this before reviewing the patches.

I haven't seen a warning like this before; can you provide a full log (either as attachment or in backtick "fences" (ie triple backtick) in a comment?)

The output of hg wip (assuming you've run ./mach bootstrap) or hg show stack (if you haven't) may also help figuring this out. :-)

Comment 15

[:Gijs (he/him)]

Err, I guess you figured it out based on the updates - clearing needinfo. But feel free to ping me again if you get stuck with hg.

Comment 16

[:Gijs (he/him)]

I'm going to land all the approved patches to avoid bitrot. :-)

Comment 17

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #16)

I'm going to land all the approved patches to avoid bitrot. :-)

Err, though it looks like D205524 and later may need to be rebased because of the change to use the browser-window env?

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/753d8d4a2048
Refactor BrowserForward into a function in BrowserCommand. r=Gijs,dao

Comment 19

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/753d8d4a2048

Comment 20

[Yi Xiong Wong]

(In reply to :Gijs (he/him) from comment #15)

Err, I guess you figured it out based on the updates - clearing needinfo. But feel free to ping me again if you get stuck with hg.

Yup, figured it out eventually, and forgot to clear the needinfo, sorry!

(In reply to :Gijs (he/him) from comment #17)

(In reply to :Gijs (he/him) from comment #16)

I'm going to land all the approved patches to avoid bitrot. :-)

Err, though it looks like D205524 and later may need to be rebased because of the change to use the browser-window env?

Done, should be fine now?

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c86ff401df9a
Move BrowserBack into BrowserCommands. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/8148bc52cd1b
Move BrowserHandle(Shift)Backspace to BrowserCommands. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/9c8aae84022c
Move gotoHistoryIndex. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/7c7af557e71c
Move all reload functions. r=Gijs,devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/242be80ea3f5
Move BrowserHome. r=Gijs,extension-reviewers,sessionstore-reviewers,dao

Comment 22

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c86ff401df9a
https://hg.mozilla.org/mozilla-central/rev/8148bc52cd1b
https://hg.mozilla.org/mozilla-central/rev/9c8aae84022c
https://hg.mozilla.org/mozilla-central/rev/7c7af557e71c
https://hg.mozilla.org/mozilla-central/rev/242be80ea3f5

Comment 23

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move BrowserStop. r=Gijs

Comment 24

[:Gijs (he/him)]

I'm on a short break over Easter, until Tuesday, but will definitely be reviewing the new incoming patch(es) then. I pushed the existing r+ patches and those seem to have landed successfully. Thanks again for picking this up!

Comment 25

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move BrowserOpenFileWindow. r=Gijs

Comment 26

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move browser/tab closing functions. r=Gijs

Comment 27

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move view source functions. r=Gijs

Comment 28

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move BrowserPageInfo. r=Gijs

Comment 29

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move BrowserFullScreen. r=Gijs

Comment 30

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move BrowserDownloadsUI. r=Gijs

Comment 31

[:Gijs (he/him)]

OK, I just finished reviewing all the new patches - thank you!

Once those are out of the way, only BrowserOpenAddonsMgr and BrowserForceEncodingDetection remain, I think?

For the former, could we move it to browser-addons.js instead (perhaps as a method on BrowserAddonUI) ? That would seem more topical.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9edcaf442ba7
Move BrowserOpenTab. r=Gijs,webdriver-reviewers,perftest-reviewers,extension-reviewers,sessionstore-reviewers,tabbrowser-reviewers,home-newtab-reviewers,thecount,whimboo,dao,afinder,omc-reviewers,aminomancer
https://hg.mozilla.org/integration/autoland/rev/6a4bc1fdc54a
Move BrowserStop. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/701859440f6f
Move BrowserOpenFileWindow. r=Gijs

Comment 33

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/9edcaf442ba7
https://hg.mozilla.org/mozilla-central/rev/6a4bc1fdc54a
https://hg.mozilla.org/mozilla-central/rev/701859440f6f

Comment 34

[tobias]

Should this bug also move the BrowserSearch object? It is used in a similar way to the Browser* functions.

Comment 35

[Mark Banner (:standard8)]

(In reply to tobias from comment #34)

Should this bug also move the BrowserSearch object? It is used in a similar way to the Browser* functions.

That needs splitting up differently, and we have bug 1880913 filed to cover that.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71c30fc6409c
Move browser/tab closing functions. r=Gijs,perftest-reviewers,devtools-reviewers,fxview-reviewers,sessionstore-reviewers,sfoster,kshampur
https://hg.mozilla.org/integration/autoland/rev/64d268bc8edd
Move view source functions. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/8912f476fc77
Move BrowserPageInfo. r=Gijs,anti-tracking-reviewers
https://hg.mozilla.org/integration/autoland/rev/5430bc9502b0
Move BrowserFullScreen. r=Gijs

Comment 37

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/71c30fc6409c
https://hg.mozilla.org/mozilla-central/rev/64d268bc8edd
https://hg.mozilla.org/mozilla-central/rev/8912f476fc77
https://hg.mozilla.org/mozilla-central/rev/5430bc9502b0

Comment 38

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move BrowserForceEncodingDetection. r=Gijs

Comment 39

[Yi Xiong Wong]

Attachment: Bug 1880914 - Move BrowserOpenAddonsMgr. r=Gijs

Comment 40

[Yi Xiong Wong]

(In reply to :Gijs (he/him) from comment #31)

OK, I just finished reviewing all the new patches - thank you!

Thanks to you too!

Once those are out of the way, only BrowserOpenAddonsMgr and BrowserForceEncodingDetection remain, I think?

For the former, could we move it to browser-addons.js instead (perhaps as a method on BrowserAddonUI) ? That would seem more topical.

Done

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07ae6e6eaa04
Move BrowserDownloadsUI. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/ccf28f25eec7
Move BrowserForceEncodingDetection. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/177f569e8625
Move BrowserOpenAddonsMgr. r=Gijs,extension-reviewers,settings-reviewers,firefox-desktop-core-reviewers ,home-newtab-reviewers,robwu,thecount

Comment 42

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/07ae6e6eaa04
https://hg.mozilla.org/mozilla-central/rev/ccf28f25eec7
https://hg.mozilla.org/mozilla-central/rev/177f569e8625

Comment 43

[:Gijs (he/him)]

Thanks so much for your help here, Yi Xiong Wong! If you're enjoying the code cleanup, although bug 1882774 and bug 1882776 are not marked "good first bug" and probably slightly trickier than the refactoring here, they should be very manageable after all your work here. If you'd prefer non-refactoring mentored bugs I could look for some of those, too - just let me know. :-)