Closed Bug 1631570 Opened 4 years ago Closed 3 years ago

Make all options parameter for domain methods default to {}

Categories

(Remote Protocol :: Agent, defect, P3)

defect

Tracking

(firefox89 fixed)

RESOLVED FIXED
89 Branch
Tracking Status
firefox89 --- fixed

People

(Reporter: impossibus, Assigned: mathildaudufo, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 1 obsolete file)

We actually worked on that a while ago via bug 1601037, but looks like we still miss some of those. Probably this happened because we only looked for (options). Some of the methods (like Page.reload) don't define that named argument, but destructure the dict immediately:

https://searchfox.org/mozilla-central/rev/d2cec90777d573585f8477d5170892e5dcdfb0ab/remote/domains/content/Page.jsm#135

To be consistent we should do the following:

  1. Update all public facing APIs under content and parent of [/remote/domains/]https://searchfox.org/mozilla-central/source/remote/domains/) to only use the options argument
  2. Destructure its components afterward within the method

Benefit of that would also be that we unclutter those long argument blocks.

It's not critical so I'm happy to mentor this bug.

Mentor: hskupin
Depends on: 1601037
Priority: -- → P3
Summary: Some protocol methods are expecting params when they shouldn't → Make all options parameter default to {}
Whiteboard: [lang=js]
Severity: -- → S3

Hi Henrik. Has this been fixed yet?

Hey Mark. No, this hasn't been fixed yet. Would you be interested to have a look at this bug? I'm happy to help whenever you run into issues.

Yeah, I'll take a look at it. Is email the best way to get in contact with you?

FYI the requested conversation happened via our Matrix channel. Feel free to further ask there if you have more questions.

Hi I'm new to open source. Can I be assigned this bug?

Feel free to get started. Once a patch has been uploaded you will be automatically assigned.

Assignee: nobody → mathildaudufo
Status: NEW → ASSIGNED

Do I need to wait for a review of the patch I just submitted before I fix the rest?

The patch has been submitted over the weekend. So you should give us a little bit of time to have a look at it. Also when there are questions please set the "Request information from" flag for the mentor here on Bugzilla, and by asking the question in a comment.

I had a brief look at the change and it looks fine to me. Please check the remaining methods under domains/ for similar cases. Thanks a lot!

Tilda, you pushed a new review. I wonder if you created a new bookmark or committed a different changeset. Usually you can do updates and then amend the changes via hg commit --amend. If you don't do that each time you push the patch to phabricator a new revision (attachment) would be created.

For this time mind marking the first patch as abandoned? Thanks.

Flags: needinfo?(mathildaudufo)

Tilda, you pushed a new review. I wonder if you created a new bookmark or committed a different changeset. Usually you can do updates and then amend the changes via hg commit --amend. If you don't do that each time you push the patch to phabricator a new revision (attachment) would be created.

For this time mind marking the first patch as abandoned? Thanks.

Summary: Make all options parameter default to {} → Make all options parameter for domain methods default to {}
Attachment #9201638 - Attachment is obsolete: true

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #13)

Tilda, you pushed a new review. I wonder if you created a new bookmark or committed a different changeset. Usually you can do updates and then amend the changes via hg commit --amend. If you don't do that each time you push the patch to phabricator a new revision (attachment) would be created.

For this time mind marking the first patch as abandoned? Thanks.

I'm sorry contributing to OS is still very new to me.

Flags: needinfo?(mathildaudufo)

No worries at all. It's indeed a not always easy learning phase. If you want direct help you might also want to join us on https://chat.mozilla.org/#/room/#interop:mozilla.org.

So I changed the argument of this method to options = {} and then destructured it within the method. I then ran ./mach mochitest --headless remote/test/browser like you told me to to. It returned an output saying 415 tests had failed and that it couldn't find a file. At first I thought it was because of the changes I had made so I reverted them and ran the tests again but it still returned the same output. I asked around in the chatroom and they said that they couldn't understand why I was having that issue since the tests ran okay when they did it on their machine. I've looked everywhere for things that I might have changed mistakenly but came up with nothing.

Flags: needinfo?(hskupin)

(In reply to Tilda Udufo from comment #16)

So I changed the argument of this method to options = {} and then destructured it within the method. I then ran ./mach mochitest --headless remote/test/browser like you told me to to. It returned an output saying 415 tests had failed and that it couldn't find a file. At first I thought it was because of the changes I had made so I reverted them and ran the tests again but it still returned the same output. I asked around in the chatroom and they said that they couldn't understand why I was having that issue since the tests ran okay when they did it on their machine. I've looked everywhere for things that I might have changed mistakenly but came up with nothing.

mach clobber fixed it.

Flags: needinfo?(hskupin)

Yes, maybe the reason here was that all the files got moved to a different directory. See bug 1690474 for details. If you haven't pulled the latest code from the repository yet please try to do so now, and rebase your local bookmark against central. All changes you did should be applied without merge conflicts.

Tilda, I would like to check back if the recent information was good enough to get you unblocked. Please let me know if you are facing other troubles. Thanks.

Flags: needinfo?(mathildaudufo)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #19)

Tilda, I would like to check back if the recent information was good enough to get you unblocked. Please let me know if you are facing other troubles. Thanks.

Thanks your info was really helpful but I ran into another problem. My repo got corrupted so I'm following the directions on how to fix it from the mercurial docs. I'll let you know if none of the available solutions work.

Flags: needinfo?(mathildaudufo)

Ok, if you want and cannot find a solution feel free to join us at https://chat.mozilla.org/#/room/#interop:mozilla.org, and we can get it investigated.

Tilda, I wanted to check back if you have all the details to continue on this bug. If not please let me know.

Flags: needinfo?(mathildaudufo)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #22)

Tilda, I wanted to check back if you have all the details to continue on this bug. If not please let me know.

Thanks, I have everything I need. I'm almost done with the bug.

Flags: needinfo?(mathildaudufo)
Attachment #9202927 - Attachment description: Bug 1631570 - make public facing APIs use only options as argument r=whimboo → Bug 1631570 - [remote] Use optional options arguments for all public CDP commands
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb49a8f1adc9
[remote] Use optional options arguments for all public CDP commands r=whimboo,remote-protocol-reviewers
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch

Tilda, thank you a lot for working on this bug. I hope it gave you a good insight in how our process works when offering patches for the Remote Agent by using Mercurial. If you are interested to learn more please let me know, and we can find something else for you.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #26)

Tilda, thank you a lot for working on this bug. I hope it gave you a good insight in how our process works when offering patches for the Remote Agent by using Mercurial. If you are interested to learn more please let me know, and we can find something else for you.

Thank you Henrik, I'd like to learn more.

Great. Best would be to join us on https://chat.mozilla.org/#/room/#interop:mozilla.org. It will help to better stay in contact. Otherwise maybe you are interested in bug 1666456, which is for Marionette (our implementation for the WebDriver specification).

You need to log in before you can comment on or make changes to this bug.