Closed Bug 1045561 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: bzbarsky, Assigned: bkelly)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Oh, I think I see.  You mean use the webidl [Func] annotation:

  https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Func
(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.
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.
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...
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.
Attachment #8464231 - Flags: review?(ehsan)
> Not sure if that is expected.

Yes, that's exactly right.  The Exposed codegen version will do exactly that.
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::
Attachment #8464231 - Flags: review?(ehsan) → review+
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.
Attachment #8464231 - Attachment is obsolete: true
Attachment #8464273 - Flags: review+
Comment on attachment 8464273 [details] [diff] [review]
Use a WebIDL Func attribute to conditionally enable Fetch Headers. (v2)

r=me
Attachment #8464273 - Flags: review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/858c7a6a1380
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.