feat: Add support for browser_specific_settings #1944
Conversation
…browser-specific-support
| assert.equal(install.asProxy, true); | ||
| assert.equal(install.manifestData.applications.gecko.id, | ||
| manifestData.applications && | ||
| manifestData.applications.gecko && |
There was a problem hiding this comment.
This seems to be the pattern to silence flow here, but it feels wrong. Should we maybe just test for the exact string here rather than optional chaining these properties? (same for the test.sign.js version)
There was a problem hiding this comment.
I see this more as "making flow happy / respect flow type checking in tests", the other option is actually silence it using a $FLOW_IGNORE suppress comment (e.g. like used in some other tests here)
As long as the assertion is going to fail when it is supposed to, I'm fine with this.
|
@rpl Should be green now (it hangs on npm ci for the windows machine). |
Lusito
changed the title
Lusito
changed the title
There was a problem hiding this comment.
Thanks for reviving that PR @Lusito!
This version looks pretty clean, I like it.
Follows just a couple of review comments (one related to make the code to retrieve the addon id to match a bit more closely what is being done on the Firefox side, the other is just related to an additional test case it may be nice to add).
| assert.equal(install.asProxy, true); | ||
| assert.equal(install.manifestData.applications.gecko.id, | ||
| manifestData.applications && | ||
| manifestData.applications.gecko && |
There was a problem hiding this comment.
I see this more as "making flow happy / respect flow type checking in tests", the other option is actually silence it using a $FLOW_IGNORE suppress comment (e.g. like used in some other tests here)
As long as the assertion is going to fail when it is supposed to, I'm fine with this.
| describe('without applications and browser_specific_settings', () => { | ||
|
|
||
| it('returns undefined when ID is not specified', () => { | ||
| assert.strictEqual(getManifestId(manifestWithoutApps), undefined); | ||
| }); | ||
|
|
There was a problem hiding this comment.
On the Firefox side "browser_specific_settings" has an higher priority than "applications" and in this patch it looks that web-ext is going to do the same, it would be nice to add an explicit test case, would you mind add one small test case to cover this in the automated tests?
| const manifestApps = [ | ||
| manifestData.browser_specific_settings, | ||
| manifestData.applications, | ||
| ]; | ||
| for (const apps of manifestApps) { | ||
| if (apps && apps.gecko && apps.gecko.id) { | ||
| return apps.gecko.id; | ||
| } | ||
| } | ||
|
|
||
| return undefined; |
There was a problem hiding this comment.
Thanks! I actually like this version.
I took another look to what is being done on the Firefox side and it looks that we are doing a slightly different check here:
https://searchfox.org/mozilla-central/rev/5a4aaccb28665807a6fd49cf48367d47fbb5a19a/toolkit/mozapps/extensions/internal/XPIInstall.jsm#472-479
in particular, on the Firefox side, it looks that if browser_specific_settings.gecko is defined we only try to retrieve the id from that object and we don't look to applications.gecko.id anymore.
It may be reasonable to match that here in web-ext too, how that sounds to you?
|
Weird, I did not get notified about your code review comments, but I got notified by the mention. Thanks for the merge! |
So, as request, I've made a cleaned up version of #1791. It fixes #1901 and resolves #1791.
I've kept the history so that the work of @andyrichardson can still be credited.
Flow and chromium tests don't seem to run on windows, so I'll wait for travis to check these. lint and unit-tests are green.
What I've done: