Closed Bug 1302898 Opened 8 years ago Closed 8 years ago

Schema restrictions array should be non-nullable

Categories

(WebExtensions :: General, defect, P3)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: mattw, Assigned: mattw)

References

Details

(Whiteboard: triaged)

Attachments

(2 files)

Bug 1302898 - Make the schema restrictions array non-nullable.
58 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1302898 - Rename 'restrictions' to 'allowedContexts'
58 bytes, text/x-review-board-request
kmag
: review+
Details 
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 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.
Attachment #8791438 - Flags: review?(kmaglione+bmo)
Priority: -- → P3
Whiteboard: triaged
Blocks: 1291737
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.
Attachment #8791438 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8792724 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d1789329b81f
https://hg.mozilla.org/mozilla-central/rev/164ce4a8757a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: