Drop the callback parameter to getShortcutOrURIAndPostData

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P5
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: standard8, Assigned: Siddhant085, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

9 months ago
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
Priority: -- → P5

Comment 1

9 months ago
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.
(Reporter)

Comment 2

9 months ago
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.
(Assignee)

Comment 3

9 months ago
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.
(Assignee)

Updated

9 months ago
Attachment #8996265 - Flags: review?(standard8)
(Reporter)

Comment 5

9 months ago
(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.
Assignee: nobody → dpsrkp.sid
(Reporter)

Comment 6

9 months ago
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.
Attachment #8996265 - Flags: review?(standard8)
(Assignee)

Updated

9 months ago
Attachment #8996265 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8996292 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8996335 - Flags: review?(standard8)
(Reporter)

Comment 9

9 months ago
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?
Attachment #8996335 - Flags: review?(standard8)
(Reporter)

Updated

9 months ago
Attachment #8996640 - Flags: review?(standard8)
(Reporter)

Updated

9 months ago
Attachment #8996335 - Attachment is obsolete: true
(Reporter)

Comment 12

9 months ago
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.
Attachment #8996640 - Flags: review?(standard8) → review+
(Reporter)

Updated

9 months ago
Keywords: checkin-needed

Comment 13

9 months ago
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/857884a8913e
Drop the callback parameter to getShortcutOrURIAndPostData r=standard8
Keywords: checkin-needed

Comment 14

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/857884a8913e
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.