Add support for [ChromeOnly] interface objects

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: bent.mozilla, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

unspecified
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Add support for [ChromeOnly] interface objects. I need this for ChromeWorker, for example.
What should happen with "content.ChromeWorker" in chrome code?  Should it just come back undefined?

That is, should we unwrap Xrays before doing the "is chrome?" check?
OK, so I have patches.  I have read the resulting codegen, but I have not tested them at runtime....  I assume bent will add some tests.
Bobby, can you look over the xpcpublic and classinfo changes wrt Xray stuff?
Attachment #759357 - Flags: review?(bugs)
Attachment #759357 - Flags: review?(bobbyholley+bmo)
Assignee: nobody → bzbarsky
Oh, and the point is that content.ChromeWorker should end up undefined, which is why we unwrap the Xray _before_ the enabled check.
Attachment #759563 - Flags: review?(bugs)
Attachment #759563 - Flags: review?(bobbyholley+bmo)
Attachment #759357 - Attachment is obsolete: true
Attachment #759357 - Flags: review?(bugs)
Attachment #759357 - Flags: review?(bobbyholley+bmo)
Posted patch Part 1 really building (obsolete) — Splinter Review
Attachment #759571 - Flags: review?(bugs)
Attachment #759571 - Flags: review?(bobbyholley+bmo)
Attachment #759563 - Attachment is obsolete: true
Attachment #759563 - Flags: review?(bugs)
Attachment #759563 - Flags: review?(bobbyholley+bmo)
Comment on attachment 759358 [details] [diff] [review]
part 2.  Make ordering of named constructors in codegen deterministic.   ran into this when looking at the diff in codegen from part 1.

Looks fine, but could you improve the comment saying why deterministic ordering is needed.
Attachment #759358 - Flags: review?(bugs) → review+
Comment on attachment 759571 [details] [diff] [review]
Part 1 really building

>   union {
>     mozilla::dom::DefineInterface mDefineDOMInterface; // for window
>     mozilla::dom::ConstructNavigatorProperty mConstructNavigatorProperty; // for navigator
>   };
>-  mozilla::dom::PrefEnabled mPrefEnabled; // May be null if not pref controlled
>+  // May be null if enabled unconditionally
>+  mozilla::dom::ConstructorEnabled mConstructorEnabled;
I'd prefer having * explicitly in this kind of member variable indicating that in can be null.
Same with parameters pointing to a function
Attachment #759571 - Flags: review?(bugs) → review+
> I'd prefer having * explicitly in this kind of member variable 

Wouldn't that make it a pointer to a function pointer?  That seems pretty odd...
Oh, why :: with ConstructorEnabled? Just because of that odd coding style of DOMCI?
> Oh, why :: with ConstructorEnabled?

Because there is a static function DOMCI called ConstructorEnabled and also the mozilla::dom::ConstructorEnabled typedef and the bareword reference is ambiguous.
Comment on attachment 759359 [details] [diff] [review]
part 3.  Add support for [ChromeOnly] on interface objects.

...looking still part 1 again, but if no need comments, it should be ok.
Attachment #759359 - Flags: review?(bugs) → review+
(In reply to Boris Zbarsky (:bz) from comment #13)

Can you explain what the difference is between the existing ConstructorEnabled stuff (which checks mChromeOnly) and the new stuff? Preferably in comments somewhere?

> Because there is a static function DOMCI called ConstructorEnabled and also
> the mozilla::dom::ConstructorEnabled typedef and the bareword reference is
> ambiguous.

Can we rename one of them?
> Can we rename one of them?

We can, but why?  Especially given that the DOMCI version is presumably on its way out with the rest of classinfo...

I will add comments about the mChromeOnly bits.
Talked to bholley and the static method will become OldBindingConstructorEnabled.
Attachment #759571 - Attachment is obsolete: true
Attachment #759571 - Flags: review?(bobbyholley+bmo)
Comment on attachment 759358 [details] [diff] [review]
part 2.  Make ordering of named constructors in codegen deterministic.   ran into this when looking at the diff in codegen from part 1.

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

::: dom/bindings/parser/WebIDL.py
@@ +508,5 @@
>          self._callback = False
>          self._finished = False
>          self.members = []
> +        # namedConstructors needs deterministic ordering
> +        self.namedConstructors = list()

Nit: []
Comment on attachment 760039 [details] [diff] [review]
part 1.  Change the "enabled" callback for WebIDL constructors to take a JSContext* and the object the constructor will be defined on.

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

r=bholley on the nsDOMClassInfo bits.

::: dom/base/nsDOMClassInfo.cpp
@@ +4075,5 @@
>      mozilla::dom::DefineInterface define =
>        name_struct->mDefineDOMInterface;
>      if (define) {
>        if (name_struct->mType == nsGlobalNameStruct::eTypeClassConstructor &&
> +          !OldBindingConstructorEnabled(name_struct, aWin)) {

{ on new line, I think

@@ +4096,5 @@
> +      if (name_struct->mConstructorEnabled &&
> +          !(*name_struct->mConstructorEnabled)(cx, global)) {
> +        return NS_OK;
> +      }
> +

Needs some commenting explaining how we don't want to define unenabled interfaces on Xrays even when they're enabled for the caller.

@@ +4975,5 @@
>        JSAutoCompartment ac(cx, naviObj);
> +
> +      if (name_struct->mConstructorEnabled &&
> +          !(*name_struct->mConstructorEnabled)(cx, naviObj)) {
> +        return NS_OK;

Commenting here as well.
Attachment #760039 - Flags: review?(bobbyholley+bmo) → review+
> { on new line, I think

We don't do that in this file....

> Needs some commenting explaining how we don't want to define unenabled interfaces on
> Xrays even when they're enabled for the caller.

Done, both places.

   https://hg.mozilla.org/integration/mozilla-inbound/rev/a96d87d45173
   https://hg.mozilla.org/integration/mozilla-inbound/rev/653f259d5d99
   https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf20eaca9ff

and documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#ChromeOnly
Flags: in-testsuite?
Target Milestone: --- → mozilla24
Blocks: 885177
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.