Closed Bug 1601037 Opened 6 years ago Closed 6 years ago

Make options parameter default to {}

Categories

(Remote Protocol :: Agent, enhancement, P3)

Product:
Remote Protocol
Component:
Agent
Type:
enhancement

Tracking

(firefox73 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 73
Tracking Flags:
Tracking Status
firefox73 --- fixed

People

(Reporter: impossibus, Assigned: mohitsingh1930, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(2 files)

Bug 1601037 - initialized option parameter present in 3 methods to default {}
47 bytes, text/x-phabricator-request
Details | Review
Bug 1601037 - options parameter set default to {} in Page.jsm:printToPDF() and IO.jsm:close()
47 bytes, text/x-phabricator-request
Details | Review

We have a few methods in the form of someMethod(options), where options is then unpacked in the method body. To prevent hitting a TypeError when options is undefined, we have to make sure it defaults to an object literal: someMethod(options = {})

Here's a search that shows the affected methods:

https://searchfox.org/mozilla-central/search?q=options%29+%7B&path=remote%2F**.jsm

Documentation about the Remote Protocol product is at:
https://firefox-source-docs.mozilla.org/remote/index.html

Whiteboard: [lang=js] → [lang=js][goodfirstbug]
Whiteboard: [lang=js][goodfirstbug] → [lang=js][good-first-bug]
Keywords: good-first-bug
Whiteboard: [lang=js][good-first-bug] → [lang=js]

Beside the fix it would be good to also add a new browser chrome test specific for the changed method. Those can be found at:

https://searchfox.org/mozilla-central/source/remote/test/browser/

Hello sir, i would like to work on this issue.

You may begin. We will assign the bug to you once you submit your patch for review.

Here is an overview of the contribution process; it includes instructions for getting the code, building, and submitting a patch: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction

At some point in the instructions, you'll be asked to run ./mach bootstrap -- when you do, make sure to choose Firefox for Desktop Artifact Mode

And here is documentation about the tests Henrik mentioned: https://firefox-source-docs.mozilla.org/remote/Testing.html#browser-chrome-tests

If you have any questions along the line, feel free to ask in #interop on Mozilla's IRC, or you can put questions in bug comments here and check "Request information from..." at the bottom of this page.

i am new to open source, and this is my first contribution can you guide me through the process?

Welcome! Happy to guide you: please read the comments and resources linked above carefully and try following the instructions. Ask questions about the instructions if you get stuck in the process. The setup process may take some time -- that's normal. :)

Do i have to use mercurial for downloading mozilla-central repo?

Also i want to know that how should i proceed to get the code, the documentation suggest either manually install and configure the development enviroment or use pre-configured ecosystem.
Here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code

You should do a manual install. I think the pre-configured system isn't working now.

Mozilla development happens in a mercurial repository, but you can use git locally with a tool called git-cinnabar. You can follow these instructions up to "With this setup, you can e.g. create new topic branches based on the remote branches:..." https://github.com/glandium/git-cinnabar/wiki/Mozilla:-A-git-workflow-for-Gecko-development

To submit code for review, you'll need to setup some accounts: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#quick-start

I installed mercurial and also downloaded the mozilla-central bundle. But should i need to configure mercurial for future progress in the issue?
Also what other tools do i need except mercurial?
Any help is appreciated, thanks in advance.

Great. I assume you're following https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial/Bundles for the bundle.

Once you have a clone of mozilla-central, as in the link above, run ./mach bootstrap -- it will take care of configuring your environment, including installing additional tools/dependencies. Essentially, follow the instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Do i also need to install visual studio, if i am on windows.

Greetings,

I would like to work on this issue, I am new to Open Source this is the first time.

I have done all the setup, following this https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites.
While doing setup I chosen Firefox for Desktop, and make build, and build was successful.
Then ran ./mach-run.

And did changes in these file as mentioned:
remote/domains/content/Page.jsm
137 setLifecycleEventsEnabled(options) {
remote/domains/parent/Input.jsm
36 async dispatchKeyEvent(options) {
remote/domains/parent/Network.jsm
83 setUserAgentOverride(options) {

After that please guide me, what to do next step or if anything I missed. Please help me.

Thank You.

Flags: needinfo?(mjzffr)

@(In reply to Abhishek Gupta from comment #12)

Greetings,

I would like to work on this issue, I am new to Open Source this is the first time.

I have done all the setup, following this https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites.
While doing setup I chosen Firefox for Desktop, and make build, and build was successful.
Then ran ./mach-run.

And did changes in these file as mentioned:
remote/domains/content/Page.jsm
137 setLifecycleEventsEnabled(options) {
remote/domains/parent/Input.jsm
36 async dispatchKeyEvent(options) {
remote/domains/parent/Network.jsm
83 setUserAgentOverride(options) {

After that please guide me, what to do next step or if anything I missed. Please help me.

Thank You.

I am currently working on this issue. Can you find something else?It is not good practice to ask such questions when someone else is working on a issue

i am getting this error on running "./mach bootstrap" :-
which: no python2.7 in (/c/Users/TECHIE MOHIT/bin:/mingw64/bin:/usr/local/bin:/usr/bin:/bin:/cmd:/c/Program Files/Java/jdk-12.0.2/bin:/c/Windows/system32:/c/Windows:/c/Windows/System32/Wbem:/c/Windows/System32/WindowsPowerShell/v1.0:/cmd:/c/Program Files/nodejs:/c/Program Files (x86)/NVIDIA Corporation/PhysX/Common:/c/Program Files (x86)/Brackets/command:/c/Users/Mohit/AppData/Local/atom/bin:/c/Program Files/PuTTY:/c/Users/TECHIE MOHIT/AppData/Local/Programs/Python/Python37/Scripts:/c/Users/TECHIE MOHIT/AppData/Local/Programs/Python/Python37:/c/Users/Mohit/AppData/Local/atom/bin:/c/Users/TECHIE MOHIT/AppData/Roaming/npm:/c/Program Files/MySQL/MySQL Server 8.0/bin:/c/Users/TECHIE MOHIT/AppData/Local/hyper/app-3.0.2/resources/bin:/c/Program Files/heroku/bin:/c/Users/TECHIE MOHIT/AppData/Local/Programs/Microsoft VS Code/bin:/c/GCC_7.2/MinGW/bin:/c/Program Files (x86)/Mercurial:/usr/bin/vendor_perl:/usr/bin/core_perl)
./mach: line 118: exec: python: cannot execute: Is a directory

(In reply to mohitsingh1930 from comment #13)

@(In reply to Abhishek Gupta from comment #12)

Greetings,

I would like to work on this issue, I am new to Open Source this is the first time.

I have done all the setup, following this https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites.
While doing setup I chosen Firefox for Desktop, and make build, and build was successful.
Then ran ./mach-run.

And did changes in these file as mentioned:
remote/domains/content/Page.jsm
137 setLifecycleEventsEnabled(options) {
remote/domains/parent/Input.jsm
36 async dispatchKeyEvent(options) {
remote/domains/parent/Network.jsm
83 setUserAgentOverride(options) {

After that please guide me, what to do next step or if anything I missed. Please help me.

Thank You.

I am currently working on this issue. Can you find something else?It is not good practice to ask such questions when someone else is working on a issue

I am sorry Mohit. I thought it was free as this issue is unassigned.

(In reply to Abhishek Gupta from comment #15)

(In reply to mohitsingh1930 from comment #13)

@(In reply to Abhishek Gupta from comment #12)

Greetings,

I would like to work on this issue, I am new to Open Source this is the first time.

I have done all the setup, following this https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites.
While doing setup I chosen Firefox for Desktop, and make build, and build was successful.
Then ran ./mach-run.

And did changes in these file as mentioned:
remote/domains/content/Page.jsm
137 setLifecycleEventsEnabled(options) {
remote/domains/parent/Input.jsm
36 async dispatchKeyEvent(options) {
remote/domains/parent/Network.jsm
83 setUserAgentOverride(options) {

After that please guide me, what to do next step or if anything I missed. Please help me.

Thank You.

I am currently working on this issue. Can you find something else?It is not good practice to ask such questions when someone else is working on a issue

I am sorry Mohit. I thought it was free as this issue is unassigned.

Yes it is unassigned but it may get assigned when patch gets accepted. I am already working on this issue, and i am sorry for that much delay in my work and also very thankful to you for understanding.
I am making my efforts to resolve is as soon as possible.

You don't need to install Visual Studio. Any plain-text editor will do.

(In reply to mohitsingh1930 from comment #14)

i am getting this error on running "./mach bootstrap" :-
...
./mach: line 118: exec: python: cannot execute: Is a directory

I assume you're following the instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites -- In particular, make sure you are using MozillaBuild. It ensures that you have the right version of Python available. For further troubleshooting with ./mach bootstrap, please ask questions on #introduction on Mozilla's IRC network.

Flags: needinfo?(mjzffr)
Priority: -- → P3

Sorry for the delay, i build the firefox with ./mach build, now what steps further i should do?

Great. Once you've made the changes described in Comment 0, please submit them for review in Phabricator. You can use moz-phab submit to do so. Resources:

i installed mozphab, created account and install certificate to mozphab. Now what next, can i talk to you on IRC?

All the information has already been provided to you above in previous comments, so please read carefully and try working on the bug. If you don't understand the description in Comment 0, or you get stuck elsewhere, then ask specific questions about it either here on on IRC #interop.

Assignee: nobody → mohitsingh1930
Status: NEW → ASSIGNED

I submit the patch with moz-phab submit command here:(https://phabricator.services.mozilla.com/D57745).

Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6f014beafb34 initialized option parameter present in 3 methods to default {} r=remote-protocol-reviewers,ato

Please note that the landed patch doesn't fix all the instances. There are still some remaining. Please see the link in comment 0.

Maybe you didn't update your tree to the latest changeset, and as such haven't seen it. Please run hg pull and hg rebase -d central and hg up central to be on the latest revision. As best wait until your former patch got merged to mozilla-central. A comment will be posted when this happened.

Keywords: leave-open

What does this means doesn't fix all instances? Can you talk on IRC network?

As said check: https://searchfox.org/mozilla-central/search?q=options%29+%7B&path=remote%2F**.jsm

There are 5 instances listed, and you only fixed 3 of them.

Also if you want to talk on IRC just join and ping us. No need to ask here for permission. Thanks.

Searchfox is great for searching the codebase. You can also use grep(1).

See here for a complete list of options without = {}:
https://searchfox.org/mozilla-central/search?q=(options)&case=false&regexp=false&path=remote%2Fdomains%2F**%2F*.js

When i repaired there was only 3, so what next should i do?

Also the patch get accepted, if this doesn't fix all instances then how did the patch get accepted?

Thanks for the patch.

(In reply to mohitsingh1930 from comment #30)

Also the patch get accepted, if this doesn't fix all instances then how did the patch get accepted?

The person who reviewed your patch probably didn't see that there were more lines to fix. Sometimes we accept a patch and then notice that more work is still needed; that's okay. As Henrik said, please pull to get the most up-to-date code and submit a second patch with more changes.

Ok, i will resubmit the patch after making changes.

When will i need to submit the patch again? As i am trying to submit but as the revision closed it refused to complete the process.
The following error popup: "ERR_CLOSED: This revision has already been closed."

You cannot amend your existing patch, you need to add a new one with just the new changes. Your first patch is already landed (integrated) so it can't be modified anymore, that's why it's closed.

How can i add new one, as the commit format i have to mention the bugid.

You can have many commits that refer to the same bug. Here's a possible commit message: "Bug 1601037 - Fix remaining options parameters to have a default"

(In reply to mohitsingh1930 from comment #33)

When will i need to submit the patch again? As i am trying to submit but as the revision closed it refused to complete the process.
The following error popup: "ERR_CLOSED: This revision has already been closed."

This is what i am getting when trying to submit a patch.

Can you help me in resolving this error?

I set the status to "Open" on Phabricator. Try again.

As I already set please use a new bookmark as created based from mozilla-central. The one you used so far (I hope you did) will not work anymore.

How to create a new bookmark? As previously i just add the comment with the mentioned bug and submitted the patch the revision was automatically created, now when again commit and trying to submit the patch it tried to submit on the old revision and refused to submit as it was closed

https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/bookmarks.html describes in how to use bookmarks. All work you do should basically happen on a bookmark and not directly on mozilla-central. Reason is that this can cause merge conflicts you aren't easily able to solve, and further it doesn't allow you to work on multiple patches in parallel. Best is to remove your own commit via hg strip, run a hg pull && hg update central, and start from fresh. Your former patch already landed on central, so only the remaining places have to be fixed.

Can i use hg pull then update and merge? After that retry to make patch submission?

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/13945362bd42 options parameter set default to {} in Page.jsm:printToPDF() and IO.jsm:close() r=remote-protocol-reviewers,whimboo

https://hg.mozilla.org/mozilla-central/rev/13945362bd42

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox73: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 73
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: