Closed
Bug 1268399
Opened 9 years ago
Closed 8 years ago
Add runtime.getBrowserInfo() method with AppInfo data
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox49 wontfix, firefox50 wontfix, firefox51 fixed)
RESOLVED
FIXED
mozilla51
People
(Reporter: yuki, Assigned: zombie, Mentored)
References
(Depends on 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved]triaged)
Attachments
(1 file)
When I tried to migrate my addon "Only Minor Update" to WebExtensions, I realized that there are too few information under "chrome.PlatformInfo".
https://addons.mozilla.org/firefox/addon/only-minor-update/
The addon downloads Firefox's update information internally and blocks it if it includes a major update (like Firefox 38 to 45), to allow Firefox to apply only security fixes. The addon tries to complete the URL from "app.update.url" like "https://aus5.mozilla.org/update/3/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml".
In XUL-based addons we can use nsIURLFormatter ( http://mxr.mozilla.org/mozilla-central/source/toolkit/components/urlformatter/nsURLFormatter.js ) but currently there is no alternative in WebExtensions.
Otherwise, I simply need a new option of Firefox itself to ignore major update and apply only security fixes.
Updated•9 years ago
|
Whiteboard: [design-decision-needed]triaged
Reporter | ||
Updated•9 years ago
|
Comment 1•9 years ago
|
||
Adding more information to chrome.PlatformInfo seemed ok to most people in the meeting, this also seems like a reasonably good first bug.
Whiteboard: [design-decision-needed]triaged → [design-decision-approved]triaged[good-first-bug]
Updated•9 years ago
|
Whiteboard: [design-decision-approved]triaged[good-first-bug] → [design-decision-approved]triaged[good first bug]
Updated•9 years ago
|
Mentor: aswan
Updated•9 years ago
|
Keywords: good-first-bug
Whiteboard: [design-decision-approved]triaged[good first bug] → [design-decision-approved]triaged
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 3•8 years ago
|
||
It's probably worth noting that WebExtensions don't have the ability to block app updates, regardless of whether they have access to this information.
Assignee | ||
Comment 4•8 years ago
|
||
I was unsure about exactly what fields to include, and about the naming, so I played it safe, and went with what Jetpack exposed from AppInfo service, and with the "app" prefix for the properties (since it's exported through *Platform*Info).
Assignee | ||
Updated•8 years ago
|
Attachment #8788000 -
Flags: review?(aswan)
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8788000 [details]
bug 1268399 - add runtime.getBrowserInfo() method with AppInfo data
https://reviewboard.mozilla.org/r/76530/#review75164
The code itself looks good here, but I think it would make more sense to add a new runtime.getAppInfo() method rather than putting this into PlatformInfo.
That means modifying the api schema for runtime, the actual code should be straightforward.
Attachment #8788000 -
Flags: review?(aswan)
Assignee | ||
Comment 6•8 years ago
|
||
I thought about that, but remembered that Chrome extension docs are intertwined with mentions of "Chrome web apps" (which use the same manifest and a lot of the same APIs), so a method named getAppInfo() might be even more confusing.
I understand "Web Extensions" != "Chrome Extensions", but that's still rather unfortunate..
Perhaps an AppInfo key under PlatformInfo makes it clearer what "app" it is referring to? Or something else?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8788000 [details]
bug 1268399 - add runtime.getBrowserInfo() method with AppInfo data
https://reviewboard.mozilla.org/r/76530/#review75324
Looks good, but it would be nice if the test were more explicit.
::: toolkit/components/extensions/ext-runtime.js:82
(Diff revision 4)
> },
>
> + getBrowserInfo: function() {
> + const {name, vendor, version, appBuildID} = Services.appinfo;
> + const info = {name, vendor, version, buildID: appBuildID};
> + return Promise.resolve(Object.freeze(info));
No need to `Object.freeze` this.
::: toolkit/components/extensions/schemas/runtime.json:95
(Diff revision 4)
> }
> },
> {
> + "id": "BrowserInfo",
> + "type": "object",
> + "restrictions": ["content"],
This shouldn't be necessary.
::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_getBrowserInfo.js:5
(Diff revision 4)
> +add_task(function* mock() {
> + const {updateAppInfo} = Cu.import("resource://testing-common/AppInfo.jsm", {});
> + updateAppInfo();
> +});
We should set these to specific values, and check those actual values in the test. It might be best to move the AppInfo setup in `ExtensionXPCShellUtils.jsm` from `startAddonManager` to its own method, and then just test against those values.
::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_getBrowserInfo.js:10
(Diff revision 4)
> +add_task(function* mock() {
> + const {updateAppInfo} = Cu.import("resource://testing-common/AppInfo.jsm", {});
> + updateAppInfo();
> +});
> +
> +function background() {
Please move this to the task.
::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_getBrowserInfo.js:24
(Diff revision 4)
> +
> + browser.test.notifyPass("runtime.getBrowserInfo");
> + });
> +}
> +
> +add_task(function* test_contentscript() {
The test function name doesn't seem very relevant.
Assignee | ||
Updated•8 years ago
|
Summary: Add more information for "chrome.PlatformInfo" to fill placeholders of "app.update.url" → Add runtime.getBrowserInfo() method with AppInfo data
Comment hidden (mozreview-request) |
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8788000 [details]
bug 1268399 - add runtime.getBrowserInfo() method with AppInfo data
https://reviewboard.mozilla.org/r/76530/#review75522
Looks good. Thanks!
Attachment #8788000 -
Flags: review+
Updated•8 years ago
|
No longer blocks: fx-enterprise
Keywords: dev-doc-needed
Updated•8 years ago
|
Attachment #8788000 -
Flags: review?(aswan)
Assignee | ||
Updated•8 years ago
|
Keywords: good-first-bug → checkin-needed
Comment 13•8 years ago
|
||
has problems to apply:
applying 6a4a3bd26229
patching file toolkit/components/extensions/test/xpcshell/xpcshell.ini
Hunk #1 FAILED at 37
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/extensions/test/xpcshell/xpcshell.ini.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue
Flags: needinfo?(tomica)
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/5dea4ee471a3
add runtime.getBrowserInfo() method with AppInfo data r=kmag
Comment 15•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment 17•8 years ago
|
||
Wontfix for 49, could still take this in 50 if it is necessary. But if it can ride with 51, great.
status-firefox50:
--- → ?
Comment 18•8 years ago
|
||
I don't think this needs to be in 50.
Comment 19•8 years ago
|
||
New page for getBrowserInfo:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/getBrowserInfo
Does this cover it, :zombie?
Flags: needinfo?(tomica)
Updated•8 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•7 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•