Closed Bug 958667 Opened 9 years ago Closed 9 years ago

Add a WebIDL annotation for the scope a method/attribute/interface is exposed to

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: ehsan.akhgari, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 7 obsolete files)

10.10 KB, patch
peterv
: review+
Details | Diff | Splinter Review
3.11 KB, patch
khuey
: review+
Details | Diff | Splinter Review
3.07 KB, patch
peterv
: review+
Details | Diff | Splinter Review
7.71 KB, patch
peterv
: review+
Details | Diff | Splinter Review
11.28 KB, patch
peterv
: review+
Details | Diff | Splinter Review
It would be nice if we could express the scope where an interface/method/attribute is visible in through a WebIDL annotation.  Strawman proposal: [Scope=window,packaged-app,certified-app].
Isn't "window" the default, basically?  If not, what does it mean?

I assume "packaged-app" implies availability in certified apps?

Also, how do you feel about calling it [AvailableIn]?

Do we need to add some sort of built-in hook for permissions checks as well?  That can get a bit complicated, since in general we need to deal with various boolean operations on permissions...
(In reply to comment #1)
> Isn't "window" the default, basically?  If not, what does it mean?

It is...  And speaking of which, we probably don't need that.  There is very little use in an API that is hidden from window.  ;-)

> I assume "packaged-app" implies availability in certified apps?

Correct.

> Also, how do you feel about calling it [AvailableIn]?

That you're clearly better at naming things than I am.  :-)

> Do we need to add some sort of built-in hook for permissions checks as well? 
> That can get a bit complicated, since in general we need to deal with various
> boolean operations on permissions...

Yeah, I thought about that, but I don't have a good proposal of what that should look like yet, for this reason...  Anyways, we can do this for now while we figure out what to do with the permissions.  I can triage the existing usages of permissions to see what kinds of constructs we would need to express to at least be able to move the existing checks into a WebIDL annotation.
Oh, I also think that this condition should be anded with whatever other existing conditions such as [Pref] and [Func] exist on the names.
Yeah, I was thinking I should just go ahead and revamp the handling of all these condition bits so they stop being mutually exclusive and get anded together.  If I do it right, it should be pretty simple.

So I need a good way to detect apps and certified apps.  On main thread, if I have a window, I figure I can just get its principal and GetAppStatus() on that.

For workers, I assume I need to stash the app status of the workerprivate, just like we stash whether it's system
Assignee: nobody → bzbarsky
(In reply to comment #4)
> Yeah, I was thinking I should just go ahead and revamp the handling of all
> these condition bits so they stop being mutually exclusive and get anded
> together.  If I do it right, it should be pretty simple.

\o/

> So I need a good way to detect apps and certified apps.  On main thread, if I
> have a window, I figure I can just get its principal and GetAppStatus() on
> that.

Yeah I believe that's the "canonical" way of doing that.

> For workers, I assume I need to stash the app status of the workerprivate, just
> like we stash whether it's system

That sounds sane...
So doing this for interfaces is not too bad.

Doing this for individual properties is a bit of a pain because we currently have 3 totally separate mechanisms for handling conditional properties (separate non-chrome/chrome arrays, boolean pref flag in Prefable, and function pointers to the function specified via Func) none of which use a codegenned function.  But the desired end state is clearly to be able to use a codegenned function here.
Ah, but in practice there is no use case for using Func with the annotations here, since you can just call this stuff just as well from inside your custom function if you're doing one anyway.  So maybe I can just assume for now that Func won't be used together with AvailableIn; that will help.
As in, I'll add an easy way for custom functions to check these conditions.
So more precisely, I'll allow both Func and AvailableIn on interface objects, just not on interface members...
Though actually, nevermind.  I can just add another pointer to Prefable for this special case.
This is the one patch I'm not too happy with, but I can't see a better simple way to do this.  Also, happy to bikeshed the naming more (e.g. we could have two separate extended attributes [PackagedAppOnly] and [CertifiedAppOnly] or something, instead of [AvailableIn=PackagedApps] and [AvailableIn=CertifiedApps].  And also also, I have no idea how to test whether the app-related annotations actually work.  Especially the worker part...  Even manual testing would be good; right now there's just been none.
Attachment #8358749 - Flags: review?(peterv)
Keywords: dev-doc-needed
Whiteboard: [need review]
Attachment #8358747 - Attachment is obsolete: true
Attachment #8358747 - Flags: review?(peterv)
Attachment #8358748 - Attachment is obsolete: true
Attachment #8358748 - Flags: review?(peterv)
(In reply to Boris Zbarsky [:bz] from comment #15)
> [AvailableIn=PackagedApps] and [AvailableIn=CertifiedApps]

Should this mean "privileged" instead of "packaged"? I thought that packaged and hosted apps should have access to exactly the same APIs and the differentiator was only if they're privileged or certified (which in the wild only packaged apps can be due to signing requirements).
> Should this mean "privileged" instead of "packaged"?

I don't know.  What I implemented based on comment 0 uses "status != APP_STATUS_NOT_INSTALLED", so includes installed, privileged, and certified apps.  It would be pretty simple to restrict to only privileged and certified, but then I agree we should rename the annotation to PrivilegedApps.
Yeah I think privileged is the correct name.
Erm, I mean, when I said "packaged" in comment 0, I meant "privileged".  My fingers apologize.  :-)
OK.  So we want to name it Privileged and also have the app type check check for "privileged or certified" for that setting, right?
Flags: needinfo?(ehsan)
Attachment #8358746 - Attachment is obsolete: true
Attachment #8358746 - Flags: review?(khuey)
Attachment #8358750 - Attachment is obsolete: true
Attachment #8358750 - Flags: review?(peterv)
Attachment #8359565 - Attachment is obsolete: true
Attachment #8359565 - Flags: review?(peterv)
Attachment #8358751 - Attachment is obsolete: true
Attachment #8358751 - Flags: review?(peterv)
Attachment #8358749 - Attachment is obsolete: true
Attachment #8358749 - Flags: review?(peterv)
(In reply to Boris Zbarsky [:bz] from comment #22)
> OK.  So we want to name it Privileged and also have the app type check check
> for "privileged or certified" for that setting, right?

Yes.  Basically we're operating under this assumption that we never want an API which is only exposed to privileged apps but not certified apps.
Flags: needinfo?(ehsan)
Blocks: 874364
Blocks: 951976
Comment on attachment 8358745 [details] [diff] [review]
part 1.  Make it possible to use multiple methods of disabling the visibility of a constructor at once, disabling it if any of them says it should be disabled.

Review of attachment 8358745 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +2096,5 @@
> +        func = iface.getExtendedAttribute("Func")
> +        if func:
> +            assert isinstance(func, list) and len(func) == 1
> +            conditions.append("%s(aCx, aObj)" % func[0])
> +        if iface.getExtendedAttribute("PrefControlled"):

I wish we could replace these with Func instead :-/.

@@ +2102,5 @@
> +        # We should really have some conditions
> +        assert len(conditions)
> +        body = CGList((CGIfWrapper(CGGeneric("return false;"), "!%s" % cond) for
> +                       cond in conditions), "\n\n")
> +        body.append(CGGeneric("return true;"))

Or you could do something like:

    body.append(CGWrapper(CGList(conditions, " &&\n"), pre="return ",
                          reindent=True))
Attachment #8358745 - Flags: review?(peterv) → review+
Attachment #8359566 - Flags: review?(peterv) → review+
It would be nice to have this information exposed in a way that allows for queries of the type "does this window have access to this interface?". That would allow us to have WebIDL as the single source of truth for that information, for example to be used by a navigator.hasFeature() API.
Hmm, it looks like that's already possible by querying nsScriptNameSpaceManager and calling mConstructorEnabled on the result with the global you want? Nice.
> it looks like that's already possible by querying nsScriptNameSpaceManager and calling
> mConstructorEnabled on the result with the global you want?

Yep.  That's how the window resolve hook goes about answering the question.
> I wish we could replace these with Func instead :-/.

We can, sure.  Just haven't gotten around to it.  I considered doing it in this bug, but there's a ton of uses for webaudio stuff and whatnot.  Then again, maybe they call call the same C++ method; I seem to recall ehsan setting it up that way...

> Or you could do something like:

Yeah, fair.  I did this to make it actually work:

        body = CGWrapper(CGList((CGGeneric(cond) for cond in conditions),
                                " &&\n"),
                         pre="return ", post=";", reindent=True)
Attachment #8359567 - Flags: review?(peterv) → review+
Attachment #8359568 - Flags: review?(peterv) → review+
(In reply to Boris Zbarsky [:bz] from comment #33)
> > I wish we could replace these with Func instead :-/.
> 
> We can, sure.  Just haven't gotten around to it.  I considered doing it in
> this bug, but there's a ton of uses for webaudio stuff and whatnot.  Then
> again, maybe they call call the same C++ method; I seem to recall ehsan
> setting it up that way...

Yes, but I'm removing all of that crap in bug 968479.  :-)
I filed bug 968643 on nixing PrefControlled.
Blocks: 971766
So I'm trying to use [AvailableIn] on XMLHttpRequest.mozSystem, which is exposed to workers. The problem I'm running into is that IsInPrivilegedApp is always called before WorkerPrivate::SetPrincipal, so it's always false when we need it.
(In reply to comment #39)
> So I'm trying to use [AvailableIn] on XMLHttpRequest.mozSystem, which is
> exposed to workers. The problem I'm running into is that IsInPrivilegedApp is
> always called before WorkerPrivate::SetPrincipal, so it's always false when we
> need it.

Hmm, I thought [AvailableIn] was tested to work in workers... :/
Depends on: 1040333
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.