Closed Bug 1179718 Opened 4 years ago Closed 4 years ago

Add a CheckAllPermissions extended attribute to WebIDL

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: kanru, Assigned: kanru)

References

Details

Attachments

(3 files)

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?
(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.
Bug 1179718 - Add a CheckAllPermissions extended attribute to WebIDL. r?bz
Attachment #8629260 - Flags: review?(bzbarsky)
Bug 1179718 - Convert BrowserElement.webidl to use CheckAllPermissions. r?bz
Attachment #8629261 - Flags: review?(bzbarsky)
Attachment #8629260 - 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+
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)
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+
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+
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)
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 #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+
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
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.