Make all options parameter for domain methods default to {}
Categories
(Remote Protocol :: Agent, defect, P3)
Tracking
(firefox89 fixed)
Tracking | Status | |
---|---|---|
firefox89 | --- | fixed |
People
(Reporter: impossibus, Assigned: mathildaudufo, Mentored)
References
Details
(Whiteboard: [lang=js])
Attachments
(1 file, 1 obsolete file)
Comment 1•4 years ago
|
||
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:
To be consistent we should do the following:
- Update all public facing APIs under
content
andparent
of [/remote/domains/]https://searchfox.org/mozilla-central/source/remote/domains/) to only use theoptions
argument - 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.
Updated•4 years ago
|
Comment 2•4 years ago
|
||
Hi Henrik. Has this been fixed yet?
Comment 3•4 years ago
|
||
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.
Comment 4•4 years ago
|
||
Yeah, I'll take a look at it. Is email the best way to get in contact with you?
Comment 5•4 years ago
|
||
FYI the requested conversation happened via our Matrix channel. Feel free to further ask there if you have more questions.
Assignee | ||
Comment 6•4 years ago
|
||
Hi I'm new to open source. Can I be assigned this bug?
Comment 7•4 years ago
|
||
Feel free to get started. Once a patch has been uploaded you will be automatically assigned.
Assignee | ||
Comment 8•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 9•4 years ago
|
||
Do I need to wait for a review of the patch I just submitted before I fix the rest?
Comment 10•4 years ago
|
||
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!
Assignee | ||
Comment 11•4 years ago
|
||
Comment 12•4 years ago
|
||
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.
Comment 13•4 years ago
|
||
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.
Updated•4 years ago
|
Assignee | ||
Comment 14•4 years ago
|
||
(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.
Comment 15•4 years ago
|
||
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.
Assignee | ||
Comment 16•4 years ago
|
||
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.
Assignee | ||
Comment 17•4 years ago
|
||
(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.
Comment 18•4 years ago
|
||
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.
Comment 19•4 years ago
|
||
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.
Assignee | ||
Comment 20•4 years ago
|
||
(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.
Comment 21•4 years ago
|
||
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.
Comment 22•3 years ago
|
||
Tilda, I wanted to check back if you have all the details to continue on this bug. If not please let me know.
Assignee | ||
Comment 23•3 years ago
|
||
(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.
Updated•3 years ago
|
Comment 24•3 years ago
|
||
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
Comment 25•3 years ago
|
||
bugherder |
Comment 26•3 years ago
|
||
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.
Assignee | ||
Comment 27•3 years ago
|
||
(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.
Comment 28•3 years ago
|
||
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).
Description
•