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

Enable support for browser_specific_settings in Firefox #6787

Merged

Conversation

SebastienGllmt
Copy link
Contributor

@SebastienGllmt SebastienGllmt commented Dec 25, 2018

Firefox supports browser_specific_settings as an alternative to applications because Chrome rejects addons with applications inside the manifest but supports the browser_specific_settings field.

This was originally introduced in here
And you can see it inside the Firefox codebase here
And supported by the Addons-linter here

Both Firefox and the addons-linter give warnings if both applications and browser_specific_settings are present and give priority to browser_specific_settings
Similarly, my change gives priority to browser_specific_settings but I did not add a warning.


This change is Reviewable

@SebastienGllmt
Copy link
Contributor Author

Any chance this can be merged? If there is a reason it hasn't been merged yet let me know and I can fix it.


let {browser_specific_settings} =
/** @type {{browser_specific_settings:{gecko:{id:string}}}} */(
JSON.parse(buf.toString('utf8')));
Copy link
Contributor

Choose a reason for hiding this comment

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

Save the JSON.parse result to a variable so you don't have to re-parse on line 185 below.

That, or perhaps change this to:

let {applications, browser_specific_settings} = JSON.parse(buf.toString('utf8'))

@SebastienGllmt
Copy link
Contributor Author

@jleyba the code was like that to match the original Firefox code but it seems they've cleaned this up themselves. I pushed a new commit based on the Mozilla linter logic: https://github.com/mozilla/addons-linter/blob/master/src/parsers/manifestjson.js#L277

@SebastienGllmt
Copy link
Contributor Author

Just another ping on this issue 🙏

@jleyba jleyba merged commit 34aedfe into SeleniumHQ:master Jun 19, 2019
@SebastienGllmt SebastienGllmt deleted the feature-accept-browser-specific branch June 25, 2019 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants