webidl codegen creates inconsistent constructor signatures when JSContext needed

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
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
Attachment #8440791 - Flags: review?(bzbarsky)
Kyle suggests renaming GetContext() to just Context().  Should we do that here?
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?".
Attachment #8440791 - Flags: review?(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.
Attachment #8440791 - Flags: review+
Carry r+ forward.  Mechanical update from GetContext() to Context() in the patch.

New try:

https://tbpl.mozilla.org/?tree=Try&rev=95b2f4360810
Attachment #8440791 - Attachment is obsolete: true
Attachment #8440888 - Flags: review+
Comment on attachment 8440886 [details] [diff] [review]
P1 Rename GlobalObject::GetContext() to Context()

r=me
Attachment #8440886 - Flags: review?(bzbarsky) → review+
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.
Attachment #8440886 - Attachment is obsolete: true
Attachment #8440933 - Flags: review+
> that dxr didn't seem to find for some reason

Because dxr only knows about code it compiled.  :(
https://hg.mozilla.org/mozilla-central/rev/fc1f605dda9d
https://hg.mozilla.org/mozilla-central/rev/961dbfea27d3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Updated the docs to take note of this change.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.