Add a CheckAllPermissions extended attribute to WebIDL

RESOLVED FIXED in Firefox 42

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kanru, Assigned: kanru)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
The CheckPermissions extended attribute implemented in bug 952486 have a "or" relationship of the listed permissions; that is, any one of the listed permission is enough for the interface or interface member to be exposed.

I'd like to add a new CheckAllPermissions attribute that enforces "and" relationship of the listed permissions. So the content has to have all the listed permissions for the interface or interface member to be exposed. We already have some hierarchical permissions implemented but couldn't use WebIDL to express them instead have to check the sub-permission in code.
Sounds reasonable to me.  kanru, do you want to do this yourself?
(Assignee)

Comment 2

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #1)
> Sounds reasonable to me.  kanru, do you want to do this yourself?

Ah, I want to give it a try, it should be interesting.
(Assignee)

Comment 3

3 years ago
Created attachment 8629260 [details]
MozReview Request: Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz
Attachment #8629260 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

3 years ago
Created attachment 8629261 [details]
MozReview Request: Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r=bz

Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r?bz
Attachment #8629261 - Flags: review?(bzbarsky)
Comment on attachment 8629260 [details]
MozReview Request: Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

https://reviewboard.mozilla.org/r/12545/#review11153

::: dom/bindings/BindingUtils.h:3135
(Diff revision 1)
>  CheckPermissions(JSContext* aCx, JSObject* aObj, const char* const aPermissions[]);

I think we should rename the existing CheckPermissions to CheckAnyPermissions to make it clearer what's going on.  Followup patch is ok for this if you're prefer that.

::: dom/bindings/Codegen.py:11454
(Diff revision 1)
> +        if len(descriptor.allpermissions):

Seems like this could be factored out into a function that takes the "anypermissions" or "allpermissions" string and does the right thing...

::: dom/bindings/Configuration.py:569
(Diff revision 1)
> +            # Adds a allpermission list to this descriptor and returns the index to use.

This should _definitely_ be factored out some.

There's some code duplication in the parser too that might make sense to factor out; I can't tell for sure from these diffs.
Comment on attachment 8629261 [details]
MozReview Request: Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r=bz

https://reviewboard.mozilla.org/r/12547/#review11159

Ship It!
Attachment #8629261 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 7

3 years ago
Comment on attachment 8629260 [details]
MozReview Request: Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz
Attachment #8629260 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

3 years ago
Comment on attachment 8629261 [details]
MozReview Request: Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r=bz

Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r=bz
Attachment #8629261 - Attachment description: MozReview Request: Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r?bz → MozReview Request: Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r=bz
Attachment #8629261 - Flags: review+
(Assignee)

Comment 9

3 years ago
Created attachment 8633277 [details]
MozReview Request: Bug 1179718 - Rename CheckPermissions to CheckAnyPermissions. r=bz

Bug 1179718 - Rename CheckPermissions to CheckAnyPermissions. r?bz
Attachment #8633277 - Flags: review?(bzbarsky)
Comment on attachment 8629260 [details]
MozReview Request: Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

https://reviewboard.mozilla.org/r/12545/#review11867

::: dom/bindings/Codegen.py:1903
(Diff revision 2)
> +        assert checkAllPermissions is None  or isinstance(checkAllPermissions, int)

Extra space after "None" here.  Please remove it.

::: dom/bindings/Codegen.py:11451
(Diff revision 2)
> -            for (k, v) in sorted(descriptor.permissions.items()):
> +            permissions = getattr(descriptor, name, None)

I don't think you need to pass the None there.  The descriptor will always have the attr, no?

::: dom/bindings/Configuration.py:553
(Diff revision 2)
> -            def addPermissions(ifaceOrMember):
> +            def addPermissions(ifaceOrMember, checkAll=False):

You only have two callers, one of which passes true and the other passes false.  Just make checkAll a required argument.

Also, it's probably better to make it the "attribute" string and then just set "permissions" based on that.  Avoids the weird boolean argument thing where you can't tell what it means at the callsite.

::: dom/bindings/parser/WebIDL.py:1177
(Diff revision 2)
> +        if (self.getExtendedAttribute("CheckAllPermissions") and

This should really be changed to have a list of primary-global-only attributes (just two of them for now?) and walk it, raising exceptions as needed.

::: dom/bindings/parser/WebIDL.py:3381
(Diff revision 2)
> +        if (self.getExtendedAttribute("CheckAllPermissions") and

And here too, should just have a list of primary-global-only things.
Attachment #8629260 - Flags: review?(bzbarsky)
Comment on attachment 8633277 [details]
MozReview Request: Bug 1179718 - Rename CheckPermissions to CheckAnyPermissions. r=bz

https://reviewboard.mozilla.org/r/13195/#review11881

Ship It!
Attachment #8633277 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 12

3 years ago
Comment on attachment 8629260 [details]
MozReview Request: Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz
Attachment #8629260 - Flags: review?(bzbarsky)
(Assignee)

Comment 13

3 years ago
Comment on attachment 8629261 [details]
MozReview Request: Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r=bz

Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r=bz
(Assignee)

Updated

3 years ago
Attachment #8633277 - Attachment description: MozReview Request: Bug 1179718 - Rename CheckPermissions to CheckAnyPermissions. r?bz → MozReview Request: Bug 1179718 - Rename CheckPermissions to CheckAnyPermissions. r=bz
Attachment #8633277 - Flags: review+
(Assignee)

Comment 14

3 years ago
Comment on attachment 8633277 [details]
MozReview Request: Bug 1179718 - Rename CheckPermissions to CheckAnyPermissions. r=bz

Bug 1179718 - Rename CheckPermissions to CheckAnyPermissions. r=bz
Comment on attachment 8629260 [details]
MozReview Request: Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz

https://reviewboard.mozilla.org/r/12545/#review12093

Ship It!
Attachment #8629260 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/c7c07736ee7a
https://hg.mozilla.org/mozilla-central/rev/009f8711fd6c
https://hg.mozilla.org/mozilla-central/rev/4dd71c32a6cb
Assignee: nobody → kanru
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.