Closed Bug 1268399 Opened 8 years ago Closed 8 years ago

Add runtime.getBrowserInfo() method with AppInfo data

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox49 wontfix, firefox50 wontfix, firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed

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.
Whiteboard: [design-decision-needed]triaged
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]
Whiteboard: [design-decision-approved]triaged[good-first-bug] → [design-decision-approved]triaged[good first bug]
Mentor: aswan
Keywords: good-first-bug
Whiteboard: [design-decision-approved]triaged[good first bug] → [design-decision-approved]triaged
Assignee: nobody → tomica
Status: NEW → ASSIGNED
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.
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).
Attachment #8788000 - Flags: review?(aswan)
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)
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 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.
Summary: Add more information for "chrome.PlatformInfo" to fill placeholders of "app.update.url" → Add runtime.getBrowserInfo() method with AppInfo data
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+
No longer blocks: fx-enterprise
Keywords: dev-doc-needed
Attachment #8788000 - Flags: review?(aswan)
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)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/5dea4ee471a3
add runtime.getBrowserInfo() method with AppInfo data r=kmag
https://hg.mozilla.org/mozilla-central/rev/5dea4ee471a3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
it looks like it got landed after all.
Flags: needinfo?(tomica)
Wontfix for 49, could still take this in 50 if it is necessary. But if it can ride with 51, great.
I don't think this needs to be in 50.
New page for getBrowserInfo:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/getBrowserInfo

Does this cover it, :zombie?
Flags: needinfo?(tomica)
Yes, thanks Will.
Flags: needinfo?(tomica)
Depends on: 1389751
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.