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

Add support for browser_specific_settings (as noted in docs) #1791

Open
wants to merge 6 commits into
base: master
from

Conversation

@andyrichardson
Copy link

andyrichardson commented Dec 17, 2019

About

The official documentation solely states that an addon can specify its id in the following manner.

"browser_specific_settings": {
  "gecko": {
    "id": "addon@example.com",
    "strict_min_version": "42.0"
  }
}

This ID attribute is not being considered by web-ext. This PR adds support for this feature.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 17, 2019

Coverage Status

Coverage decreased (-0.07%) to 99.928% when pulling 995ccf5 on andyrichardson:fix-browser-specific-support into 7ee3c44 on mozilla:master.

Copy link
Member

rpl left a comment

@andyrichardson Thanks for looking into this and opening a pull request to fix it!

It looks to me that we do need to make similar changes to getValidatedManifest too, see

if (manifestData.applications && !manifestData.applications.gecko) {
// Since the applications property only applies to gecko, make
// sure 'gecko' exists when 'applications' is defined. This should
// make introspection of gecko properties easier.
errors.push('missing "applications.gecko" property');
}
.

We should also cover this change with a couple of additional test cases added to test.manifest.js, which should also make coveralls happy again on this pull request.

Besides the additional changes described above, the travis job is currently failing because there is a single commit and it doesn't follow the conventions described at CONTRIBUTING.md#writing-commit-messages (if there are more than one commits in the PR, the pull request title has to follow those conventions, because it is what is going to be used as the commit message when squashing and merging the PR into the master branch).

manifestData.applications &&
manifestData.applications.gecko.id
Comment on lines 90 to 91

This comment has been minimized.

Copy link
@rpl

rpl Dec 18, 2019

Member

it seems reasonable to also check that the applications.gecko property actually exists here, as you are doing for the browser_specific_settings.gecko one below.

This comment has been minimized.

Copy link
@andyrichardson

andyrichardson Dec 18, 2019

Author

I initially added this but then realised the existing code didn't have this check look like for this reason

This comment has been minimized.

Copy link
@andyrichardson

andyrichardson Dec 18, 2019

Author

Updated the PR + types to cover this use case

@rpl

This comment has been minimized.

Copy link
Member

rpl commented Dec 18, 2019

As a side note, this was also failing on npm audit for reasons unrelated to this PR (I already merged #1787 to update the dependency that was triggerring the npm audit failure, and so you should also rebase this PR on top of a recent master to avoid that failure on this pull request)

@andyrichardson

This comment has been minimized.

Copy link
Author

andyrichardson commented Dec 18, 2019

Thanks for the feedback. Sounds like this is something that could be merged so I will add some tests around it 👍

…browser-specific-support
iorate added a commit to iorate/uBlacklist that referenced this pull request Dec 27, 2019
Upload to AMO does not work until this PR ( mozilla/web-ext#1791 ) is merged.
Copy link
Member

rpl left a comment

@andyrichardson I just took another look to the updated version of this patch and to the failures on travis, and it seems that this version is still triggering linting errors.

Besides that, I noticed that there are many changes in this new version that are likely applied automatically by prettier (I guess it may be due to your editor default configuration) and unrelated to the changes needed to fix the actual issue.

Would you mind to revert them?
(to keep the pull request clean and minimal, and also to keep the coding style conventions consistent to the rest of the sources).

@andyrichardson

This comment has been minimized.

Copy link
Author

andyrichardson commented Dec 30, 2019

@rpl Will run these through the linter when I get back to my laptop 👍

@Rob--W

This comment has been minimized.

Copy link
Member

Rob--W commented Feb 13, 2020

@andyrichardson Do you need help to finish this PR?

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.