1025973 - webidl codegen creates inconsistent constructor signatures when JSContext needed

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

For the ServiceWorker Cache and Fetch APIs we need to define bindings for both worker and window contexts.  In particular the CacheStorage webidl has a constructor like:

  [Constructor(sequence<any> iterable)]

I have this in my Bindings.conf:

  'CacheStorage': [{
      'headerFile': 'mozilla/dom/CacheStorage.h',
      'nativeType': 'mozilla::dom::CacheStorage'
  }, {
      'headerFile': 'mozilla/dom/CacheStorage.h',
      'nativeType': 'mozilla::dom::CacheStorage',
      'workers': True,
  }],

So I am getting both window and workers.  The window based _constructor() calls:

  result = mozilla::dom::CacheStorage::Constructor(global, cx, Constify(arg0), rv);

While the worker based _constructor() calls:

  result = mozilla::dom::CacheStorage::Constructor(global, Constify(arg0), rv);

From talking with Boris, this inconsistency was introduced in bug 816088 because there was a separate WorkerGlobalObject type that contained a JSContext.  Later, this type was consolidated into just GlobalObject.

Boris suggests that we clean this up and remove the JSContext from the non-worker case since its not needed there any more.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Do not pass JSContext to static webidl methods in non-worker case.

I wrote this patch on an m-c from last Friday.  Rebuilding now to make sure nothing changed and broke it, but I think its good to go.

https://tbpl.mozilla.org/?tree=Try&rev=020477e46a4f

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Kyle suggests renaming GetContext() to just Context().  Should we do that here?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8440791 [details] [diff] [review]
Do not pass JSContext to static webidl methods in non-worker case.

Drop flag for now as IRC concluded "sure, why not?".

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8440791 [details] [diff] [review]
Do not pass JSContext to static webidl methods in non-worker case.

r=me; the rename should just be a separate diff.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Rename GlobalObject::GetContext() to Context()

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Do not pass JSContext to static webidl methods in non-worker case.

Carry r+ forward.  Mechanical update from GetContext() to Context() in the patch.

New try:

https://tbpl.mozilla.org/?tree=Try&rev=95b2f4360810

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8440886 [details] [diff] [review]
P1 Rename GlobalObject::GetContext() to Context()

r=me

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Rename GlobalObject::GetContext() to Context() (v2)

Try build caught a use of GetContext() in dom/nfc that dxr didn't seem to find for some reason.  This only fails on b2g builds.

Carry r+ forward.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

> that dxr didn't seem to find for some reason

Because dxr only knows about code it compiled.  :(

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc1f605dda9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/961dbfea27d3

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc1f605dda9d
https://hg.mozilla.org/mozilla-central/rev/961dbfea27d3

Comment 13

[Boris Zbarsky [:bzbarsky]]

Updated the docs to take note of this change.