1742889 - Rewrite consumers of whereToOpenLink to use BrowserUtils.whereToOpenLink

Status: RESOLVED FIXED

(Firefox :: General, task)

Description

[:Gijs (he/him)]

I'm moving the functionality in bug 1742801 but while there's still a slim chance we might want to uplift that to 95, I don't want to bloat the patch with trying to teach all the consumers of that method about its new location.

Comment 1

[Leeya]

Hello! I’m a new contributor, apologies for not commenting sooner. I’ll be submitting a patch for this bug soon!

Comment 2

[Leeya]

Hello,

I have two questions.

First, I'm seeing BrowserUtils being imported in one of two ways:

• with the keyword import or
• added to the object being passed to the ChromeUtils.defineESModuleGetters(lazy, { }) function

If I need to rewrite whereToOpen... in a file that isn't already using the BrowserUtils module, is one of those ways preferred over the other?

Secondly, are there any tests I should run to confirm my code is working?

Thank you!

Comment 3

[:Gijs (he/him)]

(In reply to Leeya from comment #2)

Hello,

I have two questions.

They're good questions!

First, I'm seeing BrowserUtils being imported in one of two ways:

• with the keyword import or
• added to the object being passed to the ChromeUtils.defineESModuleGetters(lazy, { }) function

If I need to rewrite whereToOpen... in a file that isn't already using the BrowserUtils module, is one of those ways preferred over the other?

The short answer is that for this bug, the latter is almost always going to be the right thing to do.

The longer answer starts with: "It depends". import will read, load and run the module immediately, whereas defineESModuleGetters will set up the module so that it's imported lazily when first used.

Because of this, we use the import keyword if and only if all of the following are true:

1. we're in another sys.mjs file
2. the module we're importing (BrowserUtils here) is (likely) used immediately after the module has been imported.

As an example for this, https://searchfox.org/mozilla-central/rev/6121b33709dd80979a6806ff59096a561e348ae8/browser/modules/URILoadingHelper.sys.mjs#6 imports immediately, on the basis that when URILoadingHelper is used, we probably do that to call one of the openUILink or similar methods, which will immediately use the BrowserUtils module.

Secondly, are there any tests I should run to confirm my code is working?

Well, we'd want to run tests for all the consumers that you're fixing up, and they all live in different bits of code, so this question is a little tricky to answer. So, honestly, I'd expect that we should just push the patch to tryserver to see if anything broke.

Before doing that, to check locally I'd suggest just rebuilding (./mach build faster should be fine) and then running (./mach run) and checking that middle clicks on link or clicks with a modifier key (ctrl/cmd) work the same before and after the patch. Between that and eslint you should at least be able to weed out obvious syntax errors etc. :-)

Comment 4

[Leeya]

Attachment: Bug 1742889 - Rewrite consumers of whereToOpenLink to use BrowserUtils.whereToOpenLink. r?Gijs!

Comment 5

[:Gijs (he/him)]

Hi Leeya, off-hand your WIP looks pretty good! You can remove the commented out "previous" lines - Phabricator will show reviewers / us what code was there before your changes. 🙂

Are you or Stephanie pushing this to try, or do you need me to help with that? If so just let me know!

Comment 6

[Leeya]

Hi Gijs!

That's right! Okay, I'll be sure to remove the commented out previous code soon.

Stephanie did push this to try and is helping me with the results. It broke some things 🫠, so debugging is definitely in order! 🧐

Comment 7

[Stephanie Cunnane [:scunnane]]

Link to try push.

Update: Most of these failures are intermittent failures (aka flaky tests), but there is one failure in browser/components/downloads/test/browser/browser_pdfjs_preview.js related to this patch that Leeya can repro locally and is working to debug.

Comment 8

[Leeya]

Hi! I was able to debug it and get that test passing! As you can see, I submitted my patch for review 😀.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ede7a0b15af0
Rewrite consumers of whereToOpenLink to use BrowserUtils.whereToOpenLink. r=Gijs,search-reviewers,places-reviewers,firefox-desktop-core-reviewers ,home-newtab-reviewers,reusable-components-reviewers,hjones,nbarrett

Comment 10

[Serban Stanca [:SerbanS]]

Backed out for causing node tests failures.

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | karma /builds/worker/checkouts/gecko/browser/components/newtab | activity-stream:Downloads Manager:#onAction should open the file on OPEN_DOWNLOAD_FILE if the type is download: lazy.BrowserUtils.whereToOpenLink is not a function

L.E. Also caused these mochitests failures.

• Failure Log

Comment 11

[Leeya]

Commenting to acknowledge the issue with my patch. I'll look into this failing test.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d85012b3aa3d
Rewrite consumers of whereToOpenLink to use BrowserUtils.whereToOpenLink. r=Gijs,search-reviewers,places-reviewers,firefox-desktop-core-reviewers ,home-newtab-reviewers,reusable-components-reviewers,hjones,nbarrett

Comment 13

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/d85012b3aa3d