Convert to packed add-on #40
Conversation
| @@ -0,0 +1,7 @@ | |||
| { | |||
| "version": "0.12", | |||
There was a problem hiding this comment.
It would be nice if those versions were not hardcoded. I guess we can either:
- change unpack() update the files with current addon version when unpacking
- change the MAKEFILE to set the version at build time
Can be addressed in a follow up though. Will the addon version be bumped to 0.12 during the next release or should we do it right now? I think it's not technically necessary to have those version numbers aligned with the addon version, but I guess it would be easier to have them in sync.
There was a problem hiding this comment.
My main thinking here is that the manifest only has to change if these binaries change, and that's quite rare. (Most changes are in the JS layer of the add-on.) By leaving this number untouched until needed, we avoid extra unpacking for add-on upgrade where the binaries don't actually change.
But yes, that means there's an extra step to update these files when we do change the binaries. I'll file a bug to think more about it.
Yes, I am planning to bump the add-on to 0.12 after merging this, so that's why I used that number here. I filed a bug.
|
|
||
| const { Cu } = require("chrome"); | ||
| const { OS } = require("resource://gre/modules/osfile.jsm"); | ||
| const { Services } = Cu.import("resource://gre/modules/Services.jsm", {}); |
There was a problem hiding this comment.
Followup: we recently updated the DevTools codebase to get rid of Cu.import calls:
https://bugzilla.mozilla.org/show_bug.cgi?id=1434374
We should probably think about doing the same in adbhelper.
There was a problem hiding this comment.
Right! Unfortunately, we can't do that here yet, since this add-on might be loaded be older versions of Firefox, at least back to release (maybe ESR too...?). So, we'd need to wait for ChromeUtils to exist in all those places first. I filed a bug for this.
binary-manager.js
Outdated
| * | ||
| * @param {string} file | ||
| * The base name of the file, such as "adb". | ||
| * @param {boolean} exec |
There was a problem hiding this comment.
nit: the param is an object, which contains a boolean exec
There was a problem hiding this comment.
Ah, good call, I'll update it.
device.js
Outdated
| } | ||
| return false; |
There was a problem hiding this comment.
nit: weird indentation and extra blank line
There was a problem hiding this comment.
Hmm, curious! I need to watch out for eslint --fix more closely! 😅 Fixed.
| Services.io.newURI("jar:" + file.path + "!/", null, null); | ||
| let fileURI = Services.io.newFileURI(file); | ||
| if (!file.isDirectory()) { | ||
| fileURI = Services.io.newURI("jar:" + fileURI.spec + "!/"); |
There was a problem hiding this comment.
Just curious about this: is there a reason to prefer fileURI.spec rather than file.path here?
There was a problem hiding this comment.
Yes, we need the file:// prefix here, which only exists in fileURI.spec.
Firefox 59 no longer supports unpacked add-ons, which we relied on for launching the included binaries. Packed mode is now enabled and we manually unpack the binaries as needed to the local profile directory.
|
Thanks for the review! I have now tested across all 3 desktop platforms, and I think we're good to go. |
Firefox 59 and later require packed add-ons, so we convert to that mode here. (Some ESLint changes included as well in separate commits.)
Seems to work well on macOS. Still need to test the others.