1045561 - Conditional exposure of Headers via codegen needs to work on workers, not just mainthread

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

The patch in bug 1029620 added a Headers idl which is exposed via a pref.  It uses a Pref annotation in the IDL, which it then ignores in workers in favor of a function that reads a pref value from the worker private.

Please switch this to a Func that does the right thing on both mainthread and workers, because the current setup is not compatible with codegenning the worker definition code based on Exposed annotations.  In particular, this bug now blocks landing Exposed.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Sorry, I'll need to chat with you a bit more to understand how this is supposed to work.  I inherited this particular bit of code from Nikhil.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Oh, I think I see.  You mean use the webidl [Func] annotation:

  https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Func

Comment 3

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #2)
> Oh, I think I see.  You mean use the webidl [Func] annotation:
> 
>   https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Func

Yeah.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Last I checked this is how the stubs for the Request/Response webidl were also exposed to workers.  CC Catalin here so he's aware of the necessary changes.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I don't see Request/Response in dom/webidl so far.  But yes, anything that is Exposed=Workers should not be using Pref annotations in the IDL.  I wonder whether I can make the IDL codegen enforce that...

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Use a WebIDL Func attribute to conditionally enable Fetch Headers.

I believe this patch does what was requested in comment 0.

Note, I had to manually call HeadersBinding::ConstructorEnabled() in RegisterBindings.cpp.  Not sure if that is expected.  In case, it seems that should be remove shortly with the Exposed support.

I used the test in bug 1045756 to verify that the pref was working.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Not sure if that is expected.

Yes, that's exactly right.  The Exposed codegen version will do exactly that.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

https://tbpl.mozilla.org/?tree=Try&rev=2eacb4650a03

Comment 9

[(no longer active)]

Comment on attachment 8464231 [details] [diff] [review]
Use a WebIDL Func attribute to conditionally enable Fetch Headers.

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

::: dom/fetch/moz.build
@@ +12,5 @@
>      'Headers.cpp',
>  ]
>  
> +LOCAL_INCLUDES += [
> +    '../workers',

I don't think you need this.

::: dom/webidl/Headers.webidl
@@ +19,5 @@
>  };
>  
>  [Constructor(optional HeadersInit init),
>   // FIXME: Exposed=Window,Worker,
> + Func="Headers::PrefEnabled"]

Nit: mozilla::dom::

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Use a WebIDL Func attribute to conditionally enable Fetch Headers. (v2)

I updated the Func name, but could not easily remove the LOCAL_INCLUDE.  I wrote a follow-up bug about the issues with the WorkerPrivate.h header export.  See bug 1045849.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8464273 [details] [diff] [review]
Use a WebIDL Func attribute to conditionally enable Fetch Headers. (v2)

r=me

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/858c7a6a1380

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/858c7a6a1380