Closed Bug 1282977 Opened 3 years ago Closed 3 years ago

Handle the "author" manifest property

Categories

(WebExtensions :: Untriaged, defect, P2)

defect

Tracking

(firefox52 verified)

VERIFIED FIXED
mozilla52
Tracking Status
firefox52 --- verified

People

(Reporter: mattw, Assigned: andy+bugzilla)

References

Details

(Keywords: dev-doc-complete, good-first-bug, Whiteboard: triaged)

Attachments

(1 file)

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
Whiteboard: [webNavigation] triaged → triaged
Whiteboard: triaged → triaged[good first bug]
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.
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.
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.
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.
Assignee: nobody → amckay
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": "webextension4@tests.mozilla.org"
> +    }
> +  }
> +}

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 = "webextension4@tests.mozilla.org";
> +
> +  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.
Attachment #8774022 - Flags: review?(kmaglione+bmo)
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 on attachment 8774022 [details]
bug 1282977 handle developer in manifest

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66662/diff/1-2/
Attachment #8774022 - Flags: review?(kmaglione+bmo)
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.
Attachment #8774022 - Flags: review?(kmaglione+bmo) → review+
Keywords: good-first-bug
Whiteboard: triaged[good first bug] → triaged
> 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.
Keywords: checkin-needed
Please mark the pending issues as resolved in MozReview so that this can be autolanded.
Keywords: checkin-needed
Done.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/23887d112341
handle developer in manifest r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/23887d112341
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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.
Status: RESOLVED → VERIFIED
Depends on: 1313567
Keywords: dev-doc-needed
-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/author

Let me know if this covers it.
Flags: needinfo?(amckay)
Looks great. Just a note that author key is actually required in Edge as a string.
Flags: needinfo?(amckay)
OK, I've done that. The sooner we can have proper compat data for manifest keys, the better.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.