880367 - Add support for [ChromeOnly] interface objects

Status: RESOLVED FIXED

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

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Add support for [ChromeOnly] interface objects. I need this for ChromeWorker, for example.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Change the "enabled" callback for WebIDL constructors to take a JSContext* and the object the constructor will be defined on.

Bobby, can you look over the xpcpublic and classinfo changes wrt Xray stuff?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make ordering of named constructors in codegen deterministic. ran into this when looking at the diff in codegen from part 1.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add support for [ChromeOnly] on interface objects.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, and the point is that content.ChromeWorker should end up undefined, which is why we unwrap the Xray _before_ the enabled check.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1, should actually compile

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 really building

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Oh, why :: with ConstructorEnabled? Just because of that odd coding style of DOMCI?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> 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 14

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 15

[Bobby Holley (:bholley)]

(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?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Talked to bholley and the static method will become OldBindingConstructorEnabled.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Change the "enabled" callback for WebIDL constructors to take a JSContext* and the object the constructor will be defined on.

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 20

[Bobby Holley (:bholley)]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> { 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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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