Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Removed default '--mute-audio' flag for chromium #1775

Merged
merged 4 commits into from
Dec 10, 2019

Conversation

jezhou
Copy link
Contributor

@jezhou jezhou commented Dec 6, 2019

Fixes #1774

@coveralls
Copy link

coveralls commented Dec 6, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling b77f064 on jezhou:jz/fix-mute-audio-chrome-flag into 23913db on mozilla:master.

@rpl rpl self-requested a review December 6, 2019 10:18
Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jezhou thanks for creating this pull request so quickly :-)
(I'm pretty sure that I marked the issue as a good first bug just yesterday :-P)

tests/unit/test-extension-runners/test.chromium.js Outdated Show resolved Hide resolved
tests/unit/test-extension-runners/test.chromium.js Outdated Show resolved Hide resolved
@jezhou
Copy link
Contributor Author

jezhou commented Dec 6, 2019

Added the test; I also slightly refactored so that there's only one source of truth for the expected default flags. I mentioned it briefly above, but I believe the flags we want until we update to 12.0 are in 11.2, which can be found here.

@jezhou jezhou requested a review from rpl December 6, 2019 20:10
@jezhou
Copy link
Contributor Author

jezhou commented Dec 8, 2019

Something I also want to ask about is that sometimes when I run the tests, this.wss.address() seems to return null. I'm working on a macOS, with node v13.2.0 and npm 6.13.1.

image

This is from an extension I'm playing around with:

image

I'm not sure if there's something I'm missing in the setup. If this is a separate issue I can file it if needed

@rpl
Copy link
Member

rpl commented Dec 9, 2019

Something I also want to ask about is that sometimes when I run the tests, this.wss.address() seems to return null. I'm working on a macOS, with node v13.2.0 and npm 6.13.1.
...
This is from an extension I'm playing around with:
...
I'm not sure if there's something I'm missing in the setup.

@jezhou does it also happen on other node version in your system, or just with nodejs v13?

If this is a separate issue I can file it if needed

yes, would you mind to file a separate issue, it looks unexpected and we should take a look into it (but I've briefly tried and I can't reproduce on my system by just running the tests on nodejs v13, I have to come back to it for further investigations)

Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just a small nit (related to making easier to maintain the flags to filter out over the time).

src/extension-runners/chromium.js Outdated Show resolved Hide resolved
@jezhou
Copy link
Contributor Author

jezhou commented Dec 9, 2019

@jezhou does it also happen on other node version in your system, or just with nodejs v13?

I also tried it on nodejs v10 with no success. I'll probably try a few combinations to see if I can get it to work, and file an issue sometime tonight 🙂

Edit: Hmmm... I restarted my computer and it now sometimes works with v13. Still doesn't consistently work with v10 though. In any case, I filed an issue 👉 #1779

@rpl rpl merged commit 7ee3c44 into mozilla:master Dec 10, 2019
@jezhou jezhou deleted the jz/fix-mute-audio-chrome-flag branch December 10, 2019 16:22
@caitmuenster
Copy link

Thanks for the patch, @jezhou! 🙌🏼 Your contribution has been added to our recognition wiki.

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

@jezhou
Copy link
Contributor Author

jezhou commented Dec 12, 2019

@caitmuenster Sure 🙂 I just made an account here https://mozillians.org/en-US/u/jezhou/

@caitmuenster
Copy link

Awesome, you're vouched. Welcome onboard! I look forward to seeing you around the project. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The Chromium launcher mutes audio by default
5 participants