1478267 - Drop the callback parameter to getShortcutOrURIAndPostData

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P5)

Description

[Mark Banner (:standard8)]

getShortcutOrURIAndPostData in browser.js no longer needs to have the callback option - all the in-tree consumers are using the promise returned, and we no longer support add-ons calling it directly.

https://searchfox.org/mozilla-central/search?q=symbol:%23getShortcutOrURIAndPostData&redirect=false

The function should be made async, the callback dropped and iife [0] wrapper dropped.

[0] https://en.wikipedia.org/wiki/Immediately-invoked_function_expression

Comment 1

[Prashanth]

Hi,
I'm new to open source contributions.
If this is a good first issue to work on, I'm interested in picking up this bug.
I may need some assistance with it.

Comment 2

[Mark Banner (:standard8)]

Thanks for the offer Prashanth, please start working on it, we generally only assign a bug once we get the first patch.

If you haven't already built Firefox, https://developer.mozilla.org/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build is a good place to start.

Comment 3

[Siddhant [:Siddhant085]]

I made some changes to fix this issue. I don't know how to run tests to check if things are working correctly. I will be attaching the patch.

Comment 4

[Siddhant [:Siddhant085]]

Attachment: Drop the callback parameter to getShortcutOrURIAndPostData

Comment 5

[Mark Banner (:standard8)]

(In reply to Siddhant from comment #3)
> I made some changes to fix this issue. I don't know how to run tests to
> check if things are working correctly. I will be attaching the patch.

Thanks for the patch. There's a couple of things you can do for testing:

- Run ESLint on the file (https://developer.mozilla.org/docs/Mozilla/Developer_guide/ESLint) to check the syntax and general rules are met, e.g.

./mach lint /path/to/file.

- Run some of the tests. Doing a search for the function brings up a few results, e.g.

https://searchfox.org/mozilla-central/search?q=getShortcutOrURIAndPostData&case=false&regexp=false&path=

which you can then run with:

./mach test /path/to/test


I've run those already and they ran fine. However I'll probably just put it through our try server before we land it just to make sure.

There's one minor issue to fix that I'll comment on in a moment.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8996265 [details] [diff] [review]
Drop the callback parameter to getShortcutOrURIAndPostData

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

::: browser/base/content/browser.js
@@ -2428,1 @@
>      let mayInheritPrincipal = false;

Please can you shift all the remaining lines of the function to reduce the indent by two spaces.

We generally use two-space indentation in our files.

Comment 7

[Siddhant [:Siddhant085]]

Attachment: Drop the callback parameter to getShortcutOrURIAndPostData

Comment 8

[Siddhant [:Siddhant085]]

Attachment: Drop the callback parameter to getShortcutOrURIAndPostData

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8996335 [details] [diff] [review]
Drop the callback parameter to getShortcutOrURIAndPostData

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

This looks like an interdiff between the latest and the previous patch? Please could you combine the two?

Comment 10

[Siddhant [:Siddhant085]]

Attachment: Drop the callback parameter to getShortcutOrURIAndPostData

Comment 11

[Mark Banner (:standard8)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b47f27b6ec747a0558ad0b2c65e1faf5c5b818a

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8996640 [details] [diff] [review]
Drop the callback parameter to getShortcutOrURIAndPostData

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

Great. Thank you for the update. The patch looks good, and the try server is showing that the tests are still passing, so this is good to go.

I'll mark this as checkin-needed, at which point someone will come along soon after and land it on an integration branch. Once it gets merged to the main mozilla-central, it'll automatically get marked as fixed.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/857884a8913e
Drop the callback parameter to getShortcutOrURIAndPostData r=standard8

Comment 14

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/857884a8913e