1282977 - Handle the "author" manifest property

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Matthew Wein [:mattw]]

This property is optional and from what I can tell it accepts a string (I was unable to find any documentation further than what can be found here: https://developer.chrome.com/extensions/manifest

Comment 1

[drhen123]

Well from searching online to the author tag in the manifest there doesn't seem to be much mention other than the chrome documentation page you linked above. 

This link
http://stackoverflow.com/questions/9391567/how-to-specify-author-of-extension-in-manifest-file

Seems to think that the author tag may no longer be supported in the v2 chrome manifest and it was only supported v1.

Comment 2

[drhen123]

Further digging into the internet has given me this webpage.

https://developer.microsoft.com/en-us/microsoft-edge/platform/documentation/extensions/api-support/supported-manifest-keys/

This specifies that the author tag is not supported in chrome.

Comment 3

[Andy McKay]

In the W3C proposal here: https://lists.w3.org/Archives/Public/public-browserext/2016May/0000.html it was proposed to be:

  "developer": {
       "name": "Your Name or Company",
       "url": "Company Website"
  }

Chrome has an author property (even if its not used) and apparently people have also been using creator.

I think our first priority should be to try and aim for supporting the W3C standard, it doesn't seem unreasonable. And if developer > name and developer > url are not specified fall back to author and homepage_url.

Since creator doesn't seem to be documented, perhaps we should just ignore it.

Comment 4

[Matthew Wein [:mattw]]

I don't really see the point in supporting this property if we don't use it or have any plan to use it in the future.  In either case, however, I agree with Andy that if we are to support this property, we should aim for supporting what the W3C proposes.

Comment 5

[Andy McKay]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e169fc2a6451

Comment 6

[Andy McKay]

Attachment: bug 1282977 handle developer in manifest

Review commit: https://reviewboard.mozilla.org/r/66662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66662/

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8774022 [details]
bug 1282977 handle developer in manifest

https://reviewboard.mozilla.org/r/66662/#review64280

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:957
(Diff revision 1)
>    function getLocale(aLocale) {
>      // Use the raw manifest, here, since we need values with their
>      // localization placeholders still in place.
>      let rawManifest = extension.rawManifest;
>  
> +    let creator = rawManifest.creator;

I think that we should probably use "author" as a fallback, since that's what Chrome uses, and drop support for "creator".

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:961
(Diff revision 1)
>  
> +    let creator = rawManifest.creator;
> +    let homepageURL = rawManifest.homepage_url;
> +
> +    // Allow developer to override creator and homepage_url.
> +    if ('developer' in rawManifest) {

Strings use double quotes. But we need to check `if (rawManifest.developer)` here, since optional properties may be null. Same for name and URL.

::: toolkit/mozapps/extensions/test/addons/webextension_4/manifest.json:16
(Diff revision 1)
> +  "applications": {
> +    "gecko": {
> +      "id": "[e-mail]"
> +    }
> +  }
> +}

We don't really use pre-built XPIs in new tests. Please use `createTempWebExtensionFile` instead.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension.js:303
(Diff revision 1)
> +
> +add_task(function* developerShouldOverride() {
> +  const ID = "[e-mail]";
> +
> +  yield promiseInstallAllFiles([do_get_addon("webextension_4")], true);
> +  yield promiseAddonStartup();

We don't actually need the add-on to start-up here. It would be better to mark it as incompatible so that it's installed but never runs.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension.js:309
(Diff revision 1)
> +
> +  let addon = yield promiseAddonByID(ID);
> +  addon.userDisabled = true;
> +
> +  // Test that it overrode.
> +  do_check_eq(addon.creator, "en name");

Please use `equal` rather than `do_check_eq` in new code.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension.js:316
(Diff revision 1)
> +
> +  Services.prefs.setCharPref(PREF_SELECTED_LOCALE, "fr-FR");
> +  yield promiseRestartManager();
> +  addon = yield promiseAddonByID(ID);
> +
> +  // Test that it overrode and localised.

s/localised/localized/ (yes, yes, I know, American hegemony...)

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension.js:329
(Diff revision 1)
> +  writeWebManifestForExtension({
> +    name: "Web Extension Name",
> +    version: "1.0",
> +    manifest_version: 2,
> +    creator: "Some creator",
> +    developer: {},

We should test with `null` and `{name: null, url: null}` too.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension.js:334
(Diff revision 1)
> +    developer: {},
> +    applications: {
> +      gecko: {
> +        id: ID
> +      }
> +    }

Please add a trailing comma after every property value.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension.js:335
(Diff revision 1)
> +    applications: {
> +      gecko: {
> +        id: ID
> +      }
> +    }
> +

Please remove extra line.

Comment 8

[Andy McKay]

Thanks, Kris that's simpler. I was worried about how many used creator and a search in dxr returned zero results, which seems suspicious actually...

https://dxr.mozilla.org/addons/search?q=creator+path%3Amanifest.json&redirect=true

Comment 9

[Andy McKay]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3635ec127d48

Comment 10

[Andy McKay]

Comment on attachment 8774022 [details]
bug 1282977 handle developer in manifest

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66662/diff/1-2/

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8774022 [details]
bug 1282977 handle developer in manifest

https://reviewboard.mozilla.org/r/66662/#review65122

::: toolkit/components/extensions/schemas/manifest.json:56
(Diff revision 2)
>              "type": "string",
>              "optional": true,
>              "preprocess": "localize"
>            },
>  
>            "creator": {

This needs to be chagned to "author"

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:957
(Diff revision 2)
> +    let creator = rawManifest.author;
> +    let homepageURL = rawManifest.homepage_url;
> +
> +    // Allow developer to override creator and homepage_url.
> +    if (rawManifest.developer) {
> +      if (rawManifest.developer.name) {
> +        creator = rawManifest.developer.name;
> +      }
> +      if (rawManifest.developer.url) {
> +        homepageURL = rawManifest.developer.url;
> +      }
> +    }

We should actually probably move this outside the `getLocale` function, since the only thing that will change between locales is the substitution values.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension.js:329
(Diff revision 2)
> +});
> +
> +add_task(function* developerEmpty() {
> +  for (let developer of [{}, null, {name: null, url: null}]) {
> +    let addon = yield promiseInstallWebExtension({
> +      manifest: {

Please add an incompatible `strict_min_version` or `strict_max_version` to both of these manifests so they don't try to startup after we install them. We wind up with test failures for unchecked promise rejections if extensions don't finish starting up before we uninstall them.

Comment 12

[Andy McKay]

> Please add an incompatible `strict_min_version` or `strict_max_version` to
> both of these manifests so they don't try to startup after we install them.
> We wind up with test failures for unchecked promise rejections if extensions
> don't finish starting up before we uninstall them.

Hmm that doesn't seem like a good thing to do, we'd be relying on the consequence of an error to stop something else happens. It wouldn't seem obvious to future developers and potentially fragile if that behaviour of checking versions ever changes. I can see the need though, wonder if there's a better way.

Comment 13

[Andy McKay]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70b5363863c9

Comment 14

[Andy McKay]

Comment on attachment 8774022 [details]
bug 1282977 handle developer in manifest

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66662/diff/2-3/

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Please mark the pending issues as resolved in MozReview so that this can be autolanded.

Comment 16

[Andy McKay]

Done.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23887d112341
handle developer in manifest r=kmag

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23887d112341

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Added a line break at the end of test_webextension.js. Eslint complained with delay:
https://hg.mozilla.org/integration/autoland/rev/23887d112341#l3.85

https://hg.mozilla.org/mozilla-central/rev/49fe455cac957808ed4a5d1685c3a1938dac1d31

Comment 20

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 51.0a2 (2016-10-12) under Windows 10 64-bit.

Verified as fixed on Firefox 52.0a1 (2016-10-12) under Windows 10 64-bit and Ubuntu 14.04 64-bit. "Author" and “Developer” properties are supported and the second one successfully overrides the “author” value, if any.

Comment 21

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/author

Let me know if this covers it.

Comment 22

[Andy McKay]

Looks great. Just a note that author key is actually required in Edge as a string.

Comment 23

[Will Bamberg [:wbamberg]]

OK, I've done that. The sooner we can have proper compat data for manifest keys, the better.