1302898 - Schema restrictions array should be non-nullable

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[Matthew Wein [:mattw]]

Since the `restrictions`[1] array gets set to to null when no restrictions are provided, `shouldInject`[2] has to always check that the array is not null before checking if the correct restrictions are provided.  We should make the `restrictions` array non-nullable so these checks can be removed.

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Schemas.jsm#477
[2]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#704

Comment 1

[Matthew Wein [:mattw]]

Attachment: Bug 1302898 - Make the schema restrictions array non-nullable.

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

Comment 2

[Matthew Wein [:mattw]]

Comment on attachment 8791438 [details]
Bug 1302898 - Make the schema restrictions array non-nullable.

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

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8791438 [details]
Bug 1302898 - Make the schema restrictions array non-nullable.

https://reviewboard.mozilla.org/r/78838/#review78098

This change looks good to me, but while we're at it, can we please rename this to something other than `restrictions`? The value doesn't add restrictions, it removes them. `contexts` might be better.

Comment 4

[Matthew Wein [:mattw]]

Attachment: Bug 1302898 - Rename 'restrictions' to 'allowedContexts'

All tests pass locally.

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

Comment 5

[Matthew Wein [:mattw]]

Comment on attachment 8791438 [details]
Bug 1302898 - Make the schema restrictions array non-nullable.

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

Comment 6

[Matthew Wein [:mattw]]

Comment on attachment 8792724 [details]
Bug 1302898 - Rename 'restrictions' to 'allowedContexts'

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

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8791438 [details]
Bug 1302898 - Make the schema restrictions array non-nullable.

https://reviewboard.mozilla.org/r/78838/#review78760

::: toolkit/components/extensions/Schemas.jsm:475
(Diff revision 3)
>      this.preprocessor = schema.preprocess || null;
>  
>      /**
> -     * @property {Array<string>} [restrictions]
> -     * A list of restrictions to consider before generating the API.
> +     * @property {Array<string>} restrictions A list of restrictions to
> +     * consider before generating the API.
> +     * @NotNull

`@NotNull` is not a thing. The type alone, and the fact that the property is not optional, is enough.

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8792724 [details]
Bug 1302898 - Rename 'restrictions' to 'allowedContexts'

https://reviewboard.mozilla.org/r/79636/#review78762

Thanks!

::: toolkit/components/extensions/Schemas.jsm:344
(Diff revision 2)
>     * @abstract
>     * @param {string} namespace The namespace of the API. This may contain dots,
>     *     e.g. in the case of "devtools.inspectedWindow".
>     * @param {string} [name] The name of the property in the namespace.
>     *     `null` if we are checking whether the namespace should be injected.
> -   * @param {Array} restrictions An arbitrary list of restrictions as declared
> +   * @param {Array} allowedContexts An arbitrary list of contexts the script is allowed

The type should be `Array<string>`. And I'm not sure what "arbitrary" means here, or "the script is allowed to run in". I'd go with something like `A list of additional, restricted contexts in which this API should be available. May include any of:` ... followed by a list of context types we recognize and their descriptions.

::: toolkit/components/extensions/Schemas.jsm:1429
(Diff revision 2)
> -      if (context.shouldInject(namespace, fun.name, fun.restrictions || ns.defaultRestrictions)) {
> +      let contexts = fun.allowedContexts.length ? fun.allowedContexts : ns.defaultContexts;
> +      if (context.shouldInject(namespace, fun.name, contexts)) {

It seems like this change would be required by the previous patch, rather than this one.

::: toolkit/components/extensions/Schemas.jsm:1660
(Diff revision 2)
> -      ns.restrictions = null;
> -      ns.defeaultRestrictions = null;
> +      ns.allowedContexts = [];
> +      ns.defaultContexts = [];

Same. The default values should probably be changed in the other patch, rather than this one.

::: toolkit/components/extensions/Schemas.jsm:1856
(Diff revision 2)
> -      ns.defaultRestrictions = namespace.defaultRestrictions || null;
> +      ns.allowedContexts = namespace.allowedContexts || [];
> +      ns.defaultContexts = namespace.defaultContexts || [];

Same again.

::: toolkit/components/extensions/Schemas.jsm:1931
(Diff revision 2)
> -        if (context.shouldInject(namespace, name, entry.restrictions || ns.defaultRestrictions)) {
> +        let contexts = entry.allowedContexts.length ? entry.allowedContexts : ns.defaultContexts;
> +        if (context.shouldInject(namespace, name, contexts)) {

And again.

Comment 9

[Matthew Wein [:mattw]]

Comment on attachment 8791438 [details]
Bug 1302898 - Make the schema restrictions array non-nullable.

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

Comment 10

[Matthew Wein [:mattw]]

Comment on attachment 8792724 [details]
Bug 1302898 - Rename 'restrictions' to 'allowedContexts'

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1789329b81f
Make the schema restrictions array non-nullable. r=kmag
https://hg.mozilla.org/integration/autoland/rev/164ce4a8757a
Rename 'restrictions' to 'allowedContexts' r=kmag

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d1789329b81f
https://hg.mozilla.org/mozilla-central/rev/164ce4a8757a