1274159 - Add [HTMLConstructor] to support custom element feature

Status: RESOLVED FIXED

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

Description

[Edgar Chen [:edgar]]

According to session 4.13.1.1 and 4.13.1.2 in https://html.spec.whatwg.org/multipage/scripting.html#custom-elements. Custom element supports using constructor to create a autonomous custom element

  class FlagIcon extends HTMLElement {
    constructor() {
      super();

      ...
    }
   
    ...
  }

  customElements.define("flag-icon", FlagIcon);

  const flagIcon = new FlagIcon();

or a customized built-in element.

  class PlasticButton extends HTMLButtonElement {
    constructor() {
      super();

      ...
    }

    ...
  }

  customElements.define("plastic-button", PlasticButton, { extends: "button" });

  const plasticButton = new PlasticButton();

So we need to follow the spec to implement the HTMLElement constructor, https://html.spec.whatwg.org/multipage/dom.html#dom-htmlelement.

Comment 1

[Edgar Chen [:edgar]]

There is a spec hole for customized built-in element, see https://github.com/whatwg/html/issues/1289.
So I am focusing on autonomous custom element in this bug first.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

btpp-active per comment 1. Edgar, feel free to modify the btpp-* to reflect the status if I am wrong.

Comment 3

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #1)
> There is a spec hole for customized built-in element, see
> https://github.com/whatwg/html/issues/1289.

Spec issue is fixed: A new extend attribute, [HTMLConstructor], is introduced for HTMLElement and all its subclass to support custom element feature.

Comment 4

[Edgar Chen [:edgar]]

Attachment: WIP patch, v1

Comment 5

[Edgar Chen [:edgar]]

Attachment: Part 1: Support looking up definitions by using constructor as a key, v1

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 2: Support HTMLConstructor WebIDL extended attribute for custom elements, v1

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 3-1: Add HTMLConstructor for HTMLElement, v1

Comment 8

[Edgar Chen [:edgar]]

Attachment: Part 3-2: Add HTMLConstructor for HTMLImageElement, v1

Comment 9

[Edgar Chen [:edgar]]

Attachment: Part 4: Add test cases for HTMLConstructor, v1

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 8793229 [details] [diff] [review]
Part 2: Support HTMLConstructor WebIDL extended attribute for custom elements, v1

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

Hi William and Boris,

I try to implement HTMLConstructor [1] in this bug for custom element, and I would like to have your feedback to see if I am on the right direction.

Some notes about these patches,
1. [HTMLConstructor] is added only on HTMLElemenet and HTMLImageElement for experimenting.
2. I plan to add support of construction stack in bug 1287348, so we always run "construction stack is empty" case for now.
3. Since element's local name should be set to definition's local name (See step 8.1), so I handle NodeInfo in binding code.

BTW, do you know is there any existing function or data structure that is useful for implementing step 5 of HTMLConstructor?
And it will be appreciated if you could provide some suggestion how to implement it?

Thank you.

[1] https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8793229 [details] [diff] [review]
Part 2: Support HTMLConstructor WebIDL extended attribute for custom elements, v1

This generally looks like a sane approach.

One thing that bothers me, looking at the shape of this patch: does this allow creating an arbitrary HTML element type with an arbitrary name (e.g. an HTMLImageElement with local name "x-foo")?  I thought the whole point was to not allow that....

>+GetCustomElementNodeInfo(JSContext* aCx, const JS::CallArgs& aCallArgs,
>+  if (!definition->IsCustomedBuiltIn()) {

That's an odd function name.  Should it be "IsCustomBuiltIn" or something?  Or even "IsAutonomousCustomElement"?

>+    JS::Rooted<JSObject*> callee(aCx, &aCallArgs.callee());

Don't you already have that in "obj"?  Might want to rename "obj" to "callee".

>+  if (!window || !(doc = window->GetExtantDoc())) {

You already null-checked "window".

I guess this needs to live in bindings code because it directly examines newtarget and the callee function?  That should probably be clearly documented somewhere.  It took me a bit to figure out why we were adding codegen complexity here.

I would prefer it if we didn't create the GlobalObject twice; it's not totally free to do that.  The problem is that we need to do this new stuff (and throw any exceptions it throws) before we get the desired proto, right?  Can we hoist the GlobalObject bit above the GetDesiredProto bit and insert the new thing in between the two?  Probably the simplest thing to do is to move the GlobalObject creation into CGClassConstructor and not do it in CGPerSignatureCall if isConstructor; that would allow simplifying the logic there too.  And add web platform tests that would detect the ordering difference, of course.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8797115 [details] [diff] [review]
Part 4: Add test cases for HTMLConstructor, v1

The various test assertion descriptions here should actually say what's being tested instead of all being copy/pastes of the same thing.

Comment 13

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #11)
> Comment on attachment 8793229 [details] [diff] [review]
> Part 2: Support HTMLConstructor WebIDL extended attribute for custom
> elements, v1
> 
> This generally looks like a sane approach.
> 
> One thing that bothers me, looking at the shape of this patch: does this
> allow creating an arbitrary HTML element type with an arbitrary name (e.g.
> an HTMLImageElement with local name "x-foo")?  I thought the whole point was
> to not allow that....

We should not allow creating an HTMLImageElement with local name "x-foo"...
But this patch allows doing so, because it doesn't implement 
step 5 of https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor yet. And I am still finding a way to implement it.

> 
> >+GetCustomElementNodeInfo(JSContext* aCx, const JS::CallArgs& aCallArgs,
> >+  if (!definition->IsCustomedBuiltIn()) {
> 
> That's an odd function name.  Should it be "IsCustomBuiltIn" or something? 
> Or even "IsAutonomousCustomElement"?

Okay, will do.

> 
> >+    JS::Rooted<JSObject*> callee(aCx, &aCallArgs.callee());
> 
> Don't you already have that in "obj"?  Might want to rename "obj" to
> "callee".

Nice catch, will do.

> 
> >+  if (!window || !(doc = window->GetExtantDoc())) {
> 
> You already null-checked "window".
> 
> I guess this needs to live in bindings code because it directly examines
> newtarget and the callee function?  That should probably be clearly
> documented somewhere.  It took me a bit to figure out why we were adding
> codegen complexity here.

Yes, will add documentation somewhere.

> 
> I would prefer it if we didn't create the GlobalObject twice; it's not
> totally free to do that.  The problem is that we need to do this new stuff
> (and throw any exceptions it throws) before we get the desired proto, right?
> Can we hoist the GlobalObject bit above the GetDesiredProto bit and insert
> the new thing in between the two?  Probably the simplest thing to do is to
> move the GlobalObject creation into CGClassConstructor and not do it in
> CGPerSignatureCall if isConstructor; that would allow simplifying the logic
> there too.  And add web platform tests that would detect the ordering
> difference, of course.

Okay, I will try your suggestion. Thank you.

And sorry for late response.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> But this patch allows doing so

OK, that shouldn't land, clearly.  ;)

> And I am still finding a way to implement it.

Hmm.  Right now this information is basically scattered all over in the form of virtual WrapObject methods, right?

We should consider putting it into something like nsHTMLTagList.h/SVGTagList.h or something...

Comment 15

[Edgar Chen [:edgar]]

Comment on attachment 8788828 [details] [diff] [review]
Part 1: Support looking up definitions by using constructor as a key, v1

I got a crash when running http://w3c-test.org/custom-elements/CustomElementRegistry.html test with applying this part1 patch.

crash at:
> * thread #1: tid = 0x2750fe9, 0x000000010312a608 XUL`js::gc::IsInsideNursery(cell=0x0000820a08000000) + 72 at HeapAPI.h:338, > queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
>     frame #0: 0x000000010312a608 XUL`js::gc::IsInsideNursery(cell=0x0000820a08000000) + 72 at HeapAPI.h:338
>    335      uintptr_t addr = uintptr_t(cell);
>    336      addr &= ~js::gc::ChunkMask;
>    337      addr |= js::gc::ChunkLocationOffset;
> -> 338      auto location = *reinterpret_cast<ChunkLocation*>(addr);
>    339      MOZ_ASSERT(location == ChunkLocation::Nursery || location == ChunkLocation::TenuredHeap);
>    340      return location == ChunkLocation::Nursery;
>    341  }

backtrace:
> * thread #1: tid = 0x2750fe9, 0x000000010312a608 XUL`js::gc::IsInsideNursery(cell=0x0000820a08000000) + 72 at HeapAPI.h:338, > queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
>   * frame #0: 0x000000010312a608 XUL`js::gc::IsInsideNursery(cell=0x0000820a08000000) + 72 at HeapAPI.h:338
>     frame #1: 0x000000010ad01d95 XUL`js::gc::Cell::isTenured(this=0x0000820a08000000) const + 21 at Heap.h:251
>     frame #2: 0x000000010a994dc9 XUL`js::gc::Cell::getTraceKind(this=0x0000820a08000000) const + 25 at Heap.h:1178
>     frame #3: 0x000000010b0a860e XUL`js::gc::StoreBuffer::CellPtrEdge::trace(this=0x00000001276fd128, mover=0x00007fff5d02a6c8) const + 62 at Marking.cpp:2374
>     frame #4: 0x000000010b0d5476 XUL`js::gc::StoreBuffer::MonoTypeBuffer<js::gc::StoreBuffer::CellPtrEdge>::trace(this=0x000000011d172de0, owner=0x000000011d172d88, mover=0x00007fff5d02a6c8) + 342 at Marking.cpp:2258
>     frame #5: 0x000000010b0f9124 XUL`js::gc::StoreBuffer::traceCells(this=0x000000011d172d88, mover=0x00007fff5d02a6c8) + 36 at StoreBuffer.h:426
>     frame #6: 0x000000010b0e61db XUL`js::Nursery::doCollection(this=0x000000011d1729e8, rt=0x000000011d172208, reason=EVICT_NURSERY, tenureCounts=0x00007fff5d02a898) + 347 at Nursery.cpp:685
>     frame #7: 0x000000010b0e5b1e XUL`js::Nursery::collect(this=0x000000011d1729e8, rt=0x000000011d172208, reason=EVICT_NURSERY) + 686 at Nursery.cpp:584
>     frame #8: 0x000000010aa4508c XUL`js::gc::GCRuntime::minorGC(this=0x000000011d172990, reason=EVICT_NURSERY, phase=PHASE_EVICT_NURSERY) + 300 at jsgc.cpp:6553
>     frame #9: 0x000000010aa0b9a0 XUL`js::gc::GCRuntime::evictNursery(this=0x000000011d172990, reason=EVICT_NURSERY) + 32 at GCRuntime.h:624
>     frame #10: 0x000000010ab5179c XUL`js::NukeCrossCompartmentWrappers(cx=0x000000011d172000, sourceFilter=0x00007fff5d02ac90, targetFilter=0x00007fff5d02ac80, nukeReferencesToWindow=DontNukeWindowReferences) + 92 at CrossCompartmentWrapper.cpp:502
>     frame #11: 0x00000001053cb4f2 XUL`WindowDestroyedEvent::Run(this=0x000000011da88e00) + 994 at nsGlobalWindow.cpp:8971
>     frame #12: 0x00000001032207a9 XUL`nsThread::ProcessNextEvent(this=0x000000011960df20, aMayWait=false, aResult=0x00007fff5d02af53) + 1289 at nsThread.cpp:1082
>     frame #13: 0x00000001032ac99c XUL`NS_ProcessPendingEvents(aThread=0x000000011960df20, aTimeout=10) + 140 at nsThreadUtils.cpp:232
>     frame #14: 0x0000000107b98d3e XUL`nsBaseAppShell::NativeEventCallback(this=0x000000011d83c790) + 190 at nsBaseAppShell.cpp:97
>     frame #15: 0x0000000107c31012 XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x000000011d83c790) + 498 at nsAppShell.mm:386
>     frame #16: 0x00007fff88354881 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
>     frame #17: 0x00007fff88333fbc CoreFoundation`__CFRunLoopDoSources0 + 556
>     frame #18: 0x00007fff883334df CoreFoundation`__CFRunLoopRun + 927
>     frame #19: 0x00007fff88332ed8 CoreFoundation`CFRunLoopRunSpecific + 296
>     frame #20: 0x00007fff9043e935 HIToolbox`RunCurrentEventLoopInMode + 235
>     frame #21: 0x00007fff9043e76f HIToolbox`ReceiveNextEventCommon + 432
>     frame #22: 0x00007fff9043e5af HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 71
>     frame #23: 0x00007fff8ce2ddf6 AppKit`_DPSNextEvent + 1067
>     frame #24: 0x00007fff8ce2d226 AppKit`-[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 454
>     frame #25: 0x0000000107c2fba4 XUL`-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](self=0x000000011d6878f0, _cmd="nextEventMatchingMask:untilDate:inMode:dequeue:", mask=18446744073709551615, expiration=4001-01-01 00:00:00 UTC, mode="kCFRunLoopDefaultMode", flag=YES) + 116 at nsAppShell.mm:121
>     frame #26: 0x00007fff8ce21d80 AppKit`-[NSApplication run] + 682
>     frame #27: 0x0000000107c319bc XUL`nsAppShell::Run(this=0x000000011d83c790) + 172 at nsAppShell.mm:660
>     frame #28: 0x0000000109172965 XUL`::XRE_RunAppShell() + 325 at nsEmbedFunctions.cpp:869
>     frame #29: 0x0000000103c1f973 XUL`mozilla::ipc::MessagePumpForChildProcess::Run(this=0x0000000119648e20, aDelegate=0x00007fff5d02cd00) + 179 at MessagePump.cpp:269
>     frame #30: 0x0000000103b25035 XUL`MessageLoop::RunInternal(this=0x00007fff5d02cd00) + 117 at message_loop.cc:232
>     frame #31: 0x0000000103b24f95 XUL`MessageLoop::RunHandler(this=0x00007fff5d02cd00) + 21 at message_loop.cc:225
>     frame #32: 0x0000000103b24f3d XUL`MessageLoop::Run(this=0x00007fff5d02cd00) + 45 at message_loop.cc:205
>     frame #33: 0x00000001091721f8 XUL`::XRE_InitChildProcess(aArgc=5, aArgv=0x00007fff5d02cf98, aChildData=0x00007fff5d02cf38) + 4184 at nsEmbedFunctions.cpp:701
>     frame #34: 0x0000000102bd44ea plugin-container`content_process_main(argc=8, argv=0x00007fff5d02cf98) + 186 at plugin-container.cpp:197
>     frame #35: 0x0000000102bd45d2 plugin-container`main(argc=9, argv=0x00007fff5d02cf98) + 34 at MozillaRuntimeMain.cpp:18
>     frame #36: 0x0000000102bd2d74 plugin-container`start + 52

Comment 16

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #15)
> Comment on attachment 8788828 [details] [diff] [review]
> Part 1: Support looking up definitions by using constructor as a key, v1
> 
> I got a crash when running
> http://w3c-test.org/custom-elements/CustomElementRegistry.html test with
> applying this part1 patch.
> 
> crash at:
> > * thread #1: tid = 0x2750fe9, 0x000000010312a608 XUL`js::gc::IsInsideNursery(cell=0x0000820a08000000) + 72 at HeapAPI.h:338, > queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
> >     frame #0: 0x000000010312a608 XUL`js::gc::IsInsideNursery(cell=0x0000820a08000000) + 72 at HeapAPI.h:338
> >    335      uintptr_t addr = uintptr_t(cell);
> >    336      addr &= ~js::gc::ChunkMask;
> >    337      addr |= js::gc::ChunkLocationOffset;
> > -> 338      auto location = *reinterpret_cast<ChunkLocation*>(addr);
> >    339      MOZ_ASSERT(location == ChunkLocation::Nursery || location == ChunkLocation::TenuredHeap);
> >    340      return location == ChunkLocation::Nursery;
> >    341  }
> 
> backtrace:
> > * thread #1: tid = 0x2750fe9, 0x000000010312a608 XUL`js::gc::IsInsideNursery(cell=0x0000820a08000000) + 72 at HeapAPI.h:338, > queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
> >   * frame #0: 0x000000010312a608 XUL`js::gc::IsInsideNursery(cell=0x0000820a08000000) + 72 at HeapAPI.h:338
> >     frame #1: 0x000000010ad01d95 XUL`js::gc::Cell::isTenured(this=0x0000820a08000000) const + 21 at Heap.h:251
> >     frame #2: 0x000000010a994dc9 XUL`js::gc::Cell::getTraceKind(this=0x0000820a08000000) const + 25 at Heap.h:1178
> >     frame #3: 0x000000010b0a860e XUL`js::gc::StoreBuffer::CellPtrEdge::trace(this=0x00000001276fd128, mover=0x00007fff5d02a6c8) const + 62 at Marking.cpp:2374
> >     frame #4: 0x000000010b0d5476 XUL`js::gc::StoreBuffer::MonoTypeBuffer<js::gc::StoreBuffer::CellPtrEdge>::trace(this=0x000000011d172de0, owner=0x000000011d172d88, mover=0x00007fff5d02a6c8) + 342 at Marking.cpp:2258
> >     frame #5: 0x000000010b0f9124 XUL`js::gc::StoreBuffer::traceCells(this=0x000000011d172d88, mover=0x00007fff5d02a6c8) + 36 at StoreBuffer.h:426
> >     frame #6: 0x000000010b0e61db XUL`js::Nursery::doCollection(this=0x000000011d1729e8, rt=0x000000011d172208, reason=EVICT_NURSERY, tenureCounts=0x00007fff5d02a898) + 347 at Nursery.cpp:685
> >     frame #7: 0x000000010b0e5b1e XUL`js::Nursery::collect(this=0x000000011d1729e8, rt=0x000000011d172208, reason=EVICT_NURSERY) + 686 at Nursery.cpp:584
> >     frame #8: 0x000000010aa4508c XUL`js::gc::GCRuntime::minorGC(this=0x000000011d172990, reason=EVICT_NURSERY, phase=PHASE_EVICT_NURSERY) + 300 at jsgc.cpp:6553
> >     frame #9: 0x000000010aa0b9a0 XUL`js::gc::GCRuntime::evictNursery(this=0x000000011d172990, reason=EVICT_NURSERY) + 32 at GCRuntime.h:624
> >     frame #10: 0x000000010ab5179c XUL`js::NukeCrossCompartmentWrappers(cx=0x000000011d172000, sourceFilter=0x00007fff5d02ac90, targetFilter=0x00007fff5d02ac80, nukeReferencesToWindow=DontNukeWindowReferences) + 92 at CrossCompartmentWrapper.cpp:502
> >     frame #11: 0x00000001053cb4f2 XUL`WindowDestroyedEvent::Run(this=0x000000011da88e00) + 994 at nsGlobalWindow.cpp:8971
> >     frame #12: 0x00000001032207a9 XUL`nsThread::ProcessNextEvent(this=0x000000011960df20, aMayWait=false, aResult=0x00007fff5d02af53) + 1289 at nsThread.cpp:1082
> >     frame #13: 0x00000001032ac99c XUL`NS_ProcessPendingEvents(aThread=0x000000011960df20, aTimeout=10) + 140 at nsThreadUtils.cpp:232
> >     frame #14: 0x0000000107b98d3e XUL`nsBaseAppShell::NativeEventCallback(this=0x000000011d83c790) + 190 at nsBaseAppShell.cpp:97
> >     frame #15: 0x0000000107c31012 XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x000000011d83c790) + 498 at nsAppShell.mm:386
> >     frame #16: 0x00007fff88354881 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
> >     frame #17: 0x00007fff88333fbc CoreFoundation`__CFRunLoopDoSources0 + 556
> >     frame #18: 0x00007fff883334df CoreFoundation`__CFRunLoopRun + 927
> >     frame #19: 0x00007fff88332ed8 CoreFoundation`CFRunLoopRunSpecific + 296
> >     frame #20: 0x00007fff9043e935 HIToolbox`RunCurrentEventLoopInMode + 235
> >     frame #21: 0x00007fff9043e76f HIToolbox`ReceiveNextEventCommon + 432
> >     frame #22: 0x00007fff9043e5af HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 71
> >     frame #23: 0x00007fff8ce2ddf6 AppKit`_DPSNextEvent + 1067
> >     frame #24: 0x00007fff8ce2d226 AppKit`-[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 454
> >     frame #25: 0x0000000107c2fba4 XUL`-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](self=0x000000011d6878f0, _cmd="nextEventMatchingMask:untilDate:inMode:dequeue:", mask=18446744073709551615, expiration=4001-01-01 00:00:00 UTC, mode="kCFRunLoopDefaultMode", flag=YES) + 116 at nsAppShell.mm:121
> >     frame #26: 0x00007fff8ce21d80 AppKit`-[NSApplication run] + 682
> >     frame #27: 0x0000000107c319bc XUL`nsAppShell::Run(this=0x000000011d83c790) + 172 at nsAppShell.mm:660
> >     frame #28: 0x0000000109172965 XUL`::XRE_RunAppShell() + 325 at nsEmbedFunctions.cpp:869
> >     frame #29: 0x0000000103c1f973 XUL`mozilla::ipc::MessagePumpForChildProcess::Run(this=0x0000000119648e20, aDelegate=0x00007fff5d02cd00) + 179 at MessagePump.cpp:269
> >     frame #30: 0x0000000103b25035 XUL`MessageLoop::RunInternal(this=0x00007fff5d02cd00) + 117 at message_loop.cc:232
> >     frame #31: 0x0000000103b24f95 XUL`MessageLoop::RunHandler(this=0x00007fff5d02cd00) + 21 at message_loop.cc:225
> >     frame #32: 0x0000000103b24f3d XUL`MessageLoop::Run(this=0x00007fff5d02cd00) + 45 at message_loop.cc:205
> >     frame #33: 0x00000001091721f8 XUL`::XRE_InitChildProcess(aArgc=5, aArgv=0x00007fff5d02cf98, aChildData=0x00007fff5d02cf38) + 4184 at nsEmbedFunctions.cpp:701
> >     frame #34: 0x0000000102bd44ea plugin-container`content_process_main(argc=8, argv=0x00007fff5d02cf98) + 186 at plugin-container.cpp:197
> >     frame #35: 0x0000000102bd45d2 plugin-container`main(argc=9, argv=0x00007fff5d02cf98) + 34 at MozillaRuntimeMain.cpp:18
> >     frame #36: 0x0000000102bd2d74 plugin-container`start + 52

Okay, setting ALLOW_MEMMOVE to false seems prevent this crash.

Comment 17

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #14)
> 
> Hmm.  Right now this information is basically scattered all over in the form
> of virtual WrapObject methods, right?
> 
> We should consider putting it into something like
> nsHTMLTagList.h/SVGTagList.h or something...

Actually, the information we need is HTMLXXXElementBinding::GetConstructorObject method.
I plan to reuse nsHTMLTagList.h and do something similar with https://dxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLContentSink.cpp#99-109 in BindingUtils. I already have a WIP patch, will attach it soon and ask for feedback then. Thank you.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> the information we need is HTMLXXXElementBinding::GetConstructorObject

But the mapping from tag names to binding ctors is not 1-1... Anyway, let's see the patch and then we can talk specifics.  ;)

Comment 19

[Edgar Chen [:edgar]]

Attachment: Part 1: Support looking up definitions by using constructor as a key, v2

Comment 20

[Edgar Chen [:edgar]]

Attachment: Part 2: Support HTMLConstructor WebIDL extended attribute for custom elements, v2

Addressed review comment #11:
- s/IsCustomedBuiltIn/IsCustomBuiltIn/.
- Remove duplicated "callee".
- Remove redundant null-checking for window.
- Move the GlobalObject creation into CGClassConstructor if isConstructor.
- Add some documentations.

And this version also implements the step 5 of https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor. I define functions to get corresponding constructor object for html elements and reuse the nsHTMLTagList.h. As mentioned in comment #12, the mapping of tag names and binding ctors is not 1-1, so we have to do additional mapping for the shared implementation, e.g. HTMLSharedElement and HTMLSharedListElement.

BTW, I also add prototype fallback, step 7 of https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor, in this patch.

Suggestions and comments are always welcome. :)

Comment 21

[Edgar Chen [:edgar]]

Attachment: Part 3-3: Add HTMLConstructor for HTMLParagraphElement, v1

Add one more experimenting element, HTMLParagraphElement, in order to pass http://w3c-test.org/custom-elements/htmlconstructor/newtarget.html test.

Comment 22

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #20)
> 
> reuse the nsHTMLTagList.h. As mentioned in comment #12, the mapping of tag
                                             ^^^^^^^^^^^
                                        Err, comment #18

Comment 23

[Edgar Chen [:edgar]]

Attachment: Part 4: Add test cases for HTMLConstructor, v2

Addressed review comment #12 and add a test case for constructing built-in custom element with incorrect "extends" in custom definition.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Part 2 looks to me like it needs splitting up into multiple pieces.  There's no reason all the "get the right constructor for this element name" stuff should be in the same changeset as all the rest of the changes there, and splitting it out will make reviewing both much simpler.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Apart from that....

I am not entirely sure why we need these various methods scattered throughout the code, or the by-tag discrimination inside those methods.  And I'm not super-happy with the duplication between the WrapObject methods and the new bits.

The way I was thinking about this is that we change nsHTMLTagList.h to include the name of the relevant interface in the macros is uses, just like it currently includes the C++ implementation class name.  Then instead of having lots of methods scattered about, you would have a single place where you include the tag list to produce an array mapping integer tag ids to relevant constructor getter functions.  Whatever file has this will need to include all the relevant binding header files, of course, but you should be able to auto-generate _that_ out of the tag list too, right?

As a followup, we could then move most if not all wrapobject methods to be based on the taglist too, instead of hardcoding things; would need to think about it a bit.

The idea would be to have a single place (nsHTMLTagList.h) that stores the mapping from tag name to corresponding interface.

Comment 26

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #24)
> Part 2 looks to me like it needs splitting up into multiple pieces.  There's
> no reason all the "get the right constructor for this element name" stuff
> should be in the same changeset as all the rest of the changes there, and
> splitting it out will make reviewing both much simpler.

Will split part2 up into multiple patch in the next version.

Comment 27

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #25)
> Apart from that....
> 
> I am not entirely sure why we need these various methods scattered
> throughout the code, or the by-tag discrimination inside those methods.  And
> I'm not super-happy with the duplication between the WrapObject methods and
> the new bits.
> 
> The way I was thinking about this is that we change nsHTMLTagList.h to
> include the name of the relevant interface in the macros is uses, just like
> it currently includes the C++ implementation class name.  Then instead of
> having lots of methods scattered about, you would have a single place where
> you include the tag list to produce an array mapping integer tag ids to
> relevant constructor getter functions.  Whatever file has this will need to
> include all the relevant binding header files, of course, but you should be
> able to auto-generate _that_ out of the tag list too, right?
> 
> As a followup, we could then move most if not all wrapobject methods to be
> based on the taglist too, instead of hardcoding things; would need to think
> about it a bit.
> 
> The idea would be to have a single place (nsHTMLTagList.h) that stores the
> mapping from tag name to corresponding interface.

I am working on this idea. Thanks for the feedback.

Comment 28

[Edgar Chen [:edgar]]

Attachment: Part 2-1: Implment a mapping of html tag and interface object, v3

In this version:

* I change nsHTMLTagList.h to include the name of the relevant names for the binding codes. And I introduce a new macro for <summary> tag which is a special case: it's implementation class is HTMLSummaryElement, but its interface is actually a HTMLElement [1].

* I still put the array mapping tag ids to relevant constructor getter functions in BindingUtils.cpp since it is the only place using this mapping for now. And #include statements is not allowed using inside a macro, so I use forward declaration here.

Hi Boris, may I have your feedback, thank you.

[1] http://searchfox.org/mozilla-central/source/dom/html/HTMLSummaryElement.cpp

Comment 29

[Edgar Chen [:edgar]]

Attachment: Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements, v3

Splitting up [HTMLConstructor] part out from attachment #8806279 [details] [diff] [review]

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8808109 [details] [diff] [review]
Part 2-1: Implment a mapping of html tag and interface object, v3

I don't think there's a need to introduce a new HTML_HTMLELEMENT_TAG_WITH_CLASS_NAME macro.  You should be able to just have:

  HTML_TAG(summary, Summary, )

In fact, looking at the places that include this stuff, I think you should be able to do this in HTMLTagList.h:

  #define HTML_HTMLELEMENT_TAG(_tag) HTML_TAG(_tag, , )

and #undef it after the big long list, and then all the consumers should be able to just define HTML_TAG and nothing else.  This should work because pasting empty string into HTML##interfaceName##Element or HTML##className##Element should produce the right thing: HTMLElement.

The BindingUtils.cpp changes here should be part of the next patch, I would think.  Why do those need the leading nullptr in the sConstructorGetterCallback array, though?  Or will that be clear in the next patch?

Comment 31

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #30)
> Comment on attachment 8808109 [details] [diff] [review]
> Part 2-1: Implment a mapping of html tag and interface object, v3
> 
> I don't think there's a need to introduce a new
> HTML_HTMLELEMENT_TAG_WITH_CLASS_NAME macro.  You should be able to just have:
> 
>   HTML_TAG(summary, Summary, )
> 
> In fact, looking at the places that include this stuff, I think you should
> be able to do this in HTMLTagList.h:
> 
>   #define HTML_HTMLELEMENT_TAG(_tag) HTML_TAG(_tag, , )
> 
> and #undef it after the big long list, and then all the consumers should be
> able to just define HTML_TAG and nothing else.  This should work because
> pasting empty string into HTML##interfaceName##Element or
> HTML##className##Element should produce the right thing: HTMLElement.

I thought we intent to avoid calling macro with empty argument to suppress compiler warning (bug 574078).
But I find we still pass empty argument in other macro [1], so I guess it might not be a problem.
Will do, thank you.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#4322

> The BindingUtils.cpp changes here should be part of the next patch, I would
> think.

Got it, I will split changes in BindingUtils.cpp into another part.

> Why do those need the leading nullptr in the sConstructorGetterCallback array, though?  Or will that be clear in the next patch?

It's a mistake, they should be mapping to HTMLUnkownElement. Thanks for catching this.

Comment 32

[Boris Zbarsky [:bzbarsky]]

> I thought we intent to avoid calling macro with empty argument to suppress compiler warning (bug 574078).

Ah.  waldo, is that still an issue?  It's annoying to have to uglify tons of code to avoid that in this case...

Comment 33

[Boris Zbarsky [:bzbarsky]]

Ah.  According to http://stackoverflow.com/questions/7666344/are-empty-macro-arguments-legal-in-c11 empty macro arguments were not allowed in older C++ versions, but as of C++11 they are perfectly fine.

Comment 34

[Edgar Chen [:edgar]]

Attachment: Part 1: Support looking up definitions by using constructor as a key, v3

Rebase.

Comment 35

[Edgar Chen [:edgar]]

Attachment: Part 2-1: Include the name of relevant interface in nsHTMLTagList.h, v4

Addressed review comment #31:
- #define HTML_HTMLELEMENT_TAG(_tag) HTML_TAG(_tag, , ).
- Use HTML_TAG(summary, Summary, ) for summary.
- Move changes in BindingUtils.cpp to part2-2.
- Replace nullptr in sConstructorGetterCallback with HTMLUnkownElementBinding::GetConstructorObject.

Comment 36

[Edgar Chen [:edgar]]

Attachment: Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements, v4

Comment 37

[Edgar Chen [:edgar]]

Attachment: Part 3: Add HTMLConstructor to HTMLElement and its subclass, v2

Follow spec to add [HTMLConstructor] to HTMLElement and its subclass.

Here is the list of HTML*Element that does NOT have [HTMLConstructor]:
- HTMLAppletElement: https://html.spec.whatwg.org/multipage/obsolete.html#htmlappletelement
- HTMLUnknownElement: https://html.spec.whatwg.org/multipage/dom.html#htmlunknownelement
- HTMLFrameElement: https://html.spec.whatwg.org/#htmlframeelement
- HTMLFrameSetElement: https://html.spec.whatwg.org/#htmlframesetelement
- HTMLHeadElement: https://html.spec.whatwg.org/multipage/semantics.html#htmlheadelement
- HTMLShadowElement: didn't find in spec.
- HTMLContentElement: didn't find in spec.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> Here is the list of HTML*Element that does NOT have [HTMLConstructor]:

At first glance, all of those are spec bugs; the spec explicitly says that all HTML element interfaces have [HTMLConstructor]...  I filed https://github.com/whatwg/html/issues/2028

>- HTMLShadowElement: didn't find in spec.
>- HTMLContentElement: didn't find in spec.

Those used to be defined in the shadow DOM spec, but I think they're gone now.

Comment 39

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #38)
> > Here is the list of HTML*Element that does NOT have [HTMLConstructor]:
> 
> At first glance, all of those are spec bugs; the spec explicitly says that
> all HTML element interfaces have [HTMLConstructor]...  I filed
> https://github.com/whatwg/html/issues/2028

Thanks for following up this.

> 
> >- HTMLShadowElement: didn't find in spec.
> >- HTMLContentElement: didn't find in spec.
> 
> Those used to be defined in the shadow DOM spec, but I think they're gone
> now.

So should we add [HTMLConstructor] on them?

Comment 40

[Boris Zbarsky [:bzbarsky]]

> So should we add [HTMLConstructor] on them?

I was going to say it doesn't matter because we don't enable them by default, but it looks like we _are_ shipping them by default?  I filed bug 1316347.  In the meantime, I still think it doesn't matter, because I don't think we create instances of those interfaces unless webcomponents is enabled.

Comment 41

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #40)
> > So should we add [HTMLConstructor] on them?
> 
> I was going to say it doesn't matter because we don't enable them by
> default, but it looks like we _are_ shipping them by default?  I filed bug
> 1316347.  In the meantime, I still think it doesn't matter, because I don't
> think we create instances of those interfaces unless webcomponents is
> enabled.

Got it. So I am not going to add [HTMLConstructor] on them in this bug, since they are removed from spec.
We could add later anytime if we finger out we should support [HTMLConstructor] on them.

Comment 42

[Boris Zbarsky [:bzbarsky]]

> So I am not going to add [HTMLConstructor] on them in this bug

You should do whatever is simplest for you to implement, basically.

Note that per the spec issue all the other interfaces from comment 37 except HTMLUnknownElement should have [HTMLConstructor].

Comment 43

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #42)
> Note that per the spec issue all the other interfaces from comment 37 except
> HTMLUnknownElement should have [HTMLConstructor].

Yeah, I will provide a new version which includes those interfaces, 
- HTMLAppletElement
- HTMLFrameElement
- HTMLFrameSetElement
- HTMLHeadElement

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8808964 [details] [diff] [review]
Part 2-1: Include the name of relevant interface in nsHTMLTagList.h, v4

>+++ b/parser/htmlparser/nsHTMLTagList.h

The comment about "designed to be used as inline input to nsHTMLTags.cpp and nsHTMLContentSink *only*" should probably be adjusted to reflect the new reality.  Maybe "designed to be used as input to various places that will define the HTML_TAG macro in useful ways"?

> The first argument to HTML_TAG is both the enum identifier of the
> property and the string value.

No, the first argument is the tag name.

>+  The thrid argument is the namespace of webidl binding codes in the form
>+  HTML$INTERFACENAMEElementBinding.

This should say something more like:

  The third argument is the interface name specified for this element
  in the HTML specification.  It can be empty if the relevant interface name
  is "HTMLElement".

>+HTML_TAG(summary, Summary, )

So this won't work right if dom.details_element.enabled is toggled false.  Can you please file a followup bug to remove that preference?

If we decide to keep the preference for some reason, there are ways we could make it work, but let's not worry about it for now.

>\ No newline at end of file

Please fix that.

r=me with those fixed.

Comment 45

[Boris Zbarsky [:bzbarsky]]

I'm assuming the hashtable in part 1 actually works correctly in terms of dealing with things moving on gc.  Please make sure a JS hacker who would catch it if it didn't reviews it.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8808967 [details] [diff] [review]
Part 3: Add HTMLConstructor to HTMLElement and its subclass, v2

Instead of adding all these Constructor functions that duplicate existing NS_NewWhatever functions, can we not simply reuse the tagname-to-factory thing the content sink does?

Comment 47

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8808965 [details] [diff] [review]
Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements, v4

>+++ b/dom/base/CustomElementRegistry.cpp
>+CustomElementRegistry::LookupCustomElementDefinition(JSContext* aCx,
>+                                                      JSObject *aConstructor) const

Please fix the indentation.

>+++ b/dom/bindings/BindingUtils.cpp

>+static const constructorGetterCallback sConstructorGetterCallback[] = {

Please document why the first and last entries here are HTMLUnknownElementBinding::GetConstructorObject.  I guess it's so that you can use nsHTMLTag to index into the array, but that should be very clearly written down.  And a comment should be added to where nsHTMLTag is defined so that a change there will also be applied here.

>+GetCustomElementNodeInfo(JSContext* aCx, const JS::CallArgs& aCallArgs,
>+    if (callee != HTMLElementBinding::GetConstructorObject(aCx)) {

This doesn't look like it will do the right thing over Xrays, at first glance...  Please add some Xray tests?

Also, the right hand side there can return null, in which case an exception has been thrown on aCx and you need to stop everything and unwind.

>+    if (tag == eHTMLTag_userdefined) {
>+      return nullptr;

Given that, do you need the dummy entry at the end of sConstructorGetterCallback?

>+    NS_ASSERTION(tag <= NS_HTML_TAG_MAX, "tag is out of bounds");

MOZ_ASSERT, please.

>+    constructorGetterCallback cb = sConstructorGetterCallback[tag];
>+    if (!cb || callee != cb(aCx)) {

Again, cb(aCx) can return null and throw on aCx; if that happens you need to propagate the exception out.

>+  RefPtr<mozilla::dom::NodeInfo> nodeInfo =

Why do you need this temporary?  Why not just "return doc->NodeInfoManager()->GetNodeInfo(....);"

>+++ b/dom/bindings/BindingUtils.h
>+// Get the desired nodeInfo for a custom element. Null is returned if the
>+// corresponding custom definition in custom element registry is not valid.

Something here should say thatthis is expected to be called from the constructor function for an HTML element interface, that the callargs/cx need to be whatever was passed to that constructor function, and that aWindow should be the window corresponding to that function.

>+++ b/dom/bindings/Codegen.py
>@@ -1712,33 +1712,87 @@ class CGClassConstructor(CGAbstractStati
>+        htmlConstructorOnlyCheck = ""
>+        htmlConstructorOnlyFallback = ""

These should probably be in the "else" clause.

Also, probably should be called "htmlConstructorSanityCheck" and "htmlConstructorFallback".

>+                if (newTarget == GetConstructorObject(cx)) {

Again, it's not clear to me that this works correctly over Xrays.  I'm pretty sure it does not.  When doing |new contentWindow.HTMLPreElement()| newTarget will be an Xray for contentWindow.HTMLPreElement, while GetConstructorObject(cx) will return the chrome window's HTMLPreElement.  I mean, maybe we'll still get back a null nodeinfo, but....

And again, GetConstructorObject can return null, in which case you need to propagate the exception.

>+            # If we are unable to get desired prototype from newTarget, then we
>+            # fallback to the interface prototype object from newTarget's realm.

s/fallback/fall back/.

>+                  JS::Rooted<JSObject*> newTargetUnwrapped(cx, js::CheckedUnwrap(newTarget));
>+                  { // enter newTarget's compartment
>+                    JSAutoCompartment ac(cx, newTargetUnwrapped);
>+                    desiredProto = GetProtoObjectHandle(cx);

This doesn't actually implement what the spec says to do.  The spec says to use GetFunctionRealm(), which is not the same thing as what you have here (e.g. in the case of scripted callable proxies whose target is not same-compartment with the proxy, or bound functions, etc).  At the very least, please file a followup for fixing this, probably blocked on a bug on adding something to do GetFunctionRealm to JSAPI.

You should probably throw if newTargetUnwrapped is null, by the way.

Also, GetProtoObjectHandle() can return null and throw...  If it does, you need to propagate the exception.

>@@ -7297,36 +7353,33 @@ class CGPerSignatureCall(CGThing):
>+            if not isConstructor:
>+                cgThings.append(CGGeneric(dedent(
>+                    """
...
>+                """)))

Please fix the indentation on that closing-string-and-parens line.

>+        if isConstructor and isHTMLConstructor(idlNode):

Given that we know there are no args, etc, can't we just codegen a call to a thing that takes nodeinfo and returns an element and not have to implement Constructor on all the HTML elements?

>+++ b/dom/bindings/parser/WebIDL.py
>@@ -4939,17 +4951,18 @@ class IDLMethod(IDLInterfaceMember, IDLS
>+              identifier == "HTMLConstructor"):

I'm not a big fan of allowing [HTMLConstructor] on random methods where it won't do anything useful.  Please use some other way to tell when the constructor of an interface is an HTMLConstructor.  For example, you could have a property on IDLMethod instances that indicates that the method is a constructor and what type of constructor... or just a boolean for "is htmlconstructor" or something.

Comment 48

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #44)
> Comment on attachment 8808964 [details] [diff] [review]
> Part 2-1: Include the name of relevant interface in nsHTMLTagList.h, v4
> 
> >+++ b/parser/htmlparser/nsHTMLTagList.h
> 
> The comment about "designed to be used as inline input to nsHTMLTags.cpp and
> nsHTMLContentSink *only*" should probably be adjusted to reflect the new
> reality.  Maybe "designed to be used as input to various places that will
> define the HTML_TAG macro in useful ways"?

Will do.

> 
> > The first argument to HTML_TAG is both the enum identifier of the
> > property and the string value.
> 
> No, the first argument is the tag name.

Will do.

> 
> >+  The thrid argument is the namespace of webidl binding codes in the form
> >+  HTML$INTERFACENAMEElementBinding.
> 
> This should say something more like:
> 
>   The third argument is the interface name specified for this element
>   in the HTML specification.  It can be empty if the relevant interface name
>   is "HTMLElement".

Much better, will do.

> 
> >+HTML_TAG(summary, Summary, )
> 
> So this won't work right if dom.details_element.enabled is toggled false. 
> Can you please file a followup bug to remove that preference?

There is an existing bug for removing dom.details_element.enabled preference, see bug 1271549.

> 
> If we decide to keep the preference for some reason, there are ways we could
> make it work, but let's not worry about it for now.
> 
> >\ No newline at end of file
> 
> Please fix that.

Okay.

> 
> r=me with those fixed.

Comment 49

[Edgar Chen [:edgar]]

Comment on attachment 8808963 [details] [diff] [review]
Part 1: Support looking up definitions by using constructor as a key, v3

>diff --git a/dom/base/CustomElementRegistry.cpp b/dom/base/CustomElementRegistry.cpp
> 
>@@ -611,19 +618,24 @@ CustomElementRegistry::Define(const nsAS
>     aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
>     return;
>   }
> 
>   /**
>    * 4. If this CustomElementRegistry contains an entry with constructor constructor,
>    *    then throw a "NotSupportedError" DOMException and abort these steps.
>    */
>-  // TODO: Step 3 of HTMLConstructor also needs a way to look up definition by
>-  // using constructor. So I plans to figure out a solution to support both of
>-  // them in bug 1274159.
>+  nsCOMPtr<nsIAtom> duplicatedAtom;
>+  ConstructorHashKey finder(constructorUnwrapped);

>@@ -771,16 +783,22 @@ CustomElementRegistry::Define(const nsAS
>                                 callbacks,
>                                 0 /* TODO dependent on HTML imports. Bug 877072 */);
> 
>   /**
>    * 12. Add definition to this CustomElementRegistry.
>    */
>   mCustomDefinitions.Put(nameAtom, definition);
> 
>+  ConstructorHashKey key(constructorUnwrapped);

finder and key need to be rooted.

Comment 50

[Edgar Chen [:edgar]]

Attachment: Part 1: Support looking up definitions by using constructor as a key, v4

- Mark the constructor of ConstructorHashKey which is callable with one argument as explicit
- Fix comment #49

Comment 51

[Edgar Chen [:edgar]]

Comment on attachment 8810284 [details] [diff] [review]
Part 1: Support looking up definitions by using constructor as a key, v4

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

Hi Jon,
We use JSObject as the key of a hashtable, and I use js::MovableCellHasher<> to generate hash value.
Could you help to review JS hashing stuff? Thank you.

Hi William,
Could you please help to review CustomElementRegistry? This patch also fixes some wpt tests. Thank you.

Comment 52

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #46)
> Comment on attachment 8808967 [details] [diff] [review]
> Part 3: Add HTMLConstructor to HTMLElement and its subclass, v2
> 
> Instead of adding all these Constructor functions that duplicate existing
> NS_NewWhatever functions, can we not simply reuse the tagname-to-factory
> thing the content sink does?

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #47)
> Comment on attachment 8808965 [details] [diff] [review]
> Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom
> elements, v4
> 
> >+        if isConstructor and isHTMLConstructor(idlNode):
> 
> Given that we know there are no args, etc, can't we just codegen a call to a
> thing that takes nodeinfo and returns an element and not have to implement
> Constructor on all the HTML elements?

Ah, yeah, we can reuse NS_NewWhatever functions and it makes changes simpler. Will do, thank you.

Comment 53

[Edgar Chen [:edgar]]

Attachment: Part 2-1: Include the name of relevant interface in nsHTMLTagList.h, v5, r=bz

Addressed review comment #44.

Comment 54

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #47)
> Comment on attachment 8808965 [details] [diff] [review]
> Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom
> elements, v4
> 
> >+++ b/dom/base/CustomElementRegistry.cpp
> >+GetCustomElementNodeInfo(JSContext* aCx, const JS::CallArgs& aCallArgs,
> >+    if (callee != HTMLElementBinding::GetConstructorObject(aCx)) {
> 
> This doesn't look like it will do the right thing over Xrays, at first
> glance...  Please add some Xray tests?
> 
> >+                if (newTarget == GetConstructorObject(cx)) {
> 
> Again, it's not clear to me that this works correctly over Xrays.  I'm
> pretty sure it does not.  When doing |new contentWindow.HTMLPreElement()|
> newTarget will be an Xray for contentWindow.HTMLPreElement, while
> GetConstructorObject(cx) will return the chrome window's HTMLPreElement.  I
> mean, maybe we'll still get back a null nodeinfo, but....

I did not test over Xrays. Will do it and add relevant test cases. Thank you for catching this.

Comment 55

[Jon Coppeard (:jonco)]

Comment on attachment 8810284 [details] [diff] [review]
Part 1: Support looking up definitions by using constructor as a key, v4

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

I think what you've got here will work, but it would be easier if you used a JS::GCHashMap for this since they're designed to handle this stuff for you.  I'll post a patch to show you what I mean.

::: dom/base/CustomElementRegistry.h
@@ +239,5 @@
>    // Custom prototypes are stored in the compartment where
>    // registerElement was called.
>    DefinitionMap mCustomDefinitions;
>  
> +  // Hashtabke for looking up definitoins by using constructor as key.

typos in 'hashtabke' and 'definitoins'

Comment 56

[Jon Coppeard (:jonco)]

Attachment: use-gchashmap

Patch on top of "Part 1" to use GCHashMap instead of PLDHashTable for this.

Comment 57

[Edgar Chen [:edgar]]

(In reply to Jon Coppeard (:jonco) from comment #55)
> Comment on attachment 8810284 [details] [diff] [review]
> Part 1: Support looking up definitions by using constructor as a key, v4
> 
> Review of attachment 8810284 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think what you've got here will work, but it would be easier if you used a
> JS::GCHashMap for this since they're designed to handle this stuff for you. 
> I'll post a patch to show you what I mean.
> 
> ::: dom/base/CustomElementRegistry.h
> @@ +239,5 @@
> >    // Custom prototypes are stored in the compartment where
> >    // registerElement was called.
> >    DefinitionMap mCustomDefinitions;
> >  
> > +  // Hashtabke for looking up definitoins by using constructor as key.
> 
> typos in 'hashtabke' and 'definitoins'

(In reply to Jon Coppeard (:jonco) from comment #56)
> Created attachment 8810470 [details] [diff] [review]
> use-gchashmap
> 
> Patch on top of "Part 1" to use GCHashMap instead of PLDHashTable for this.

Thanks for your patch. Will use GCHashMap instead.

Comment 58

[Edgar Chen [:edgar]]

Comment on attachment 8810284 [details] [diff] [review]
Part 1: Support looking up definitions by using constructor as a key, v4

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

Need to address comment #55 first.

Comment 59

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #47)
> Comment on attachment 8808965 [details] [diff] [review]
> Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom
> elements, v4
> 
> >+++ b/dom/bindings/Codegen.py
> >@@ -1712,33 +1712,87 @@ class CGClassConstructor(CGAbstractStati
> >+                  JS::Rooted<JSObject*> newTargetUnwrapped(cx, js::CheckedUnwrap(newTarget));
> >+                  { // enter newTarget's compartment
> >+                    JSAutoCompartment ac(cx, newTargetUnwrapped);
> >+                    desiredProto = GetProtoObjectHandle(cx);
> 
> This doesn't actually implement what the spec says to do.  The spec says to
> use GetFunctionRealm(), which is not the same thing as what you have here
> (e.g. in the case of scripted callable proxies whose target is not
> same-compartment with the proxy, or bound functions, etc).  At the very
> least, please file a followup for fixing this, probably blocked on a bug on
> adding something to do GetFunctionRealm to JSAPI.

Filed bug 1317658.

Comment 60

[Edgar Chen [:edgar]]

Attachment: Part 1: Support looking up definitions by using constructor as a key, v5

Use GCHashMap instead of PLDHashTable.

Comment 61

[Edgar Chen [:edgar]]

Attachment: Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements, v5

Addressed review comment #47:
- s/NS_ASSERTION/MOZ_ASSERT/
- s/htmlConstructorOnlyCheck/htmlConstructorSanityCheck/
- s/htmlConstructorOnlyFallback/htmlConstructorFallback/
- Add some documentation.
- Do correct check on target/callee in Xray case.
- GetConstructorObject()/GetProtoObjectHandle()/js::CheckedUnwrap() might return null, handle such case.
- Add isHTMLConstructor() on IDLMethod to indicate the method is a html constructor, instead of allowing [HTMLConstructor] on random methods.
- Reuse factory function in nsHTMLContentSink, instead of implementing Constructor on all the HTML elements.

Comment 62

[Edgar Chen [:edgar]]

Attachment: Part 3-1: Add HTMLConstructor to HTMLElement and its subclass, v3

- Intentionally not add [HTMLConstructor] on HTMLShadowElement/HTMLContentElement per comment #41.
- Intentionally not add [HTMLConstructor] on HTMLAppletElement/HTMLUnknownElement per https://github.com/whatwg/html/issues/2028.

Comment 63

[Edgar Chen [:edgar]]

Attachment: Part 3-2: Update web-platform-test expected result, v1

We can pass some web-platform-test after we support [HTMLConstructor] on HTMLElements.

Comment 64

[Edgar Chen [:edgar]]

Attachment: Part 4: Add test cases for HTMLConstructor, v3

Add mochitest-chrome test for Xrays.

Comment 65

[Edgar Chen [:edgar]]

Could you help to review these patches again? Thank you.

Comment 66

[Edgar Chen [:edgar]]

Comment on attachment 8811784 [details] [diff] [review]
Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements, v5

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

Hi William, Could you also help to take a look at CreateHTMLElement() in BindingUtils which implements most of https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor? Thank you.

Comment 67

[Jon Coppeard (:jonco)]

Comment on attachment 8811779 [details] [diff] [review]
Part 1: Support looking up definitions by using constructor as a key, v5

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

r=me for the GCHashMap bits.

Comment 68

[Boris Zbarsky [:bzbarsky]]

Which specific ones do you want reviewed?

Comment 69

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #68)
> Which specific ones do you want reviewed?

part2-2, part3-1 and part4, thank you. :)

Comment 70

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8811784 [details] [diff] [review]
Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements, v5

OK.  I'm not reviewing the details of the custom elements registry because I assume wchen will do that....  Please let me know if this assumption is incorrect.

>+CustomElementRegistry::LookupCustomElementDefinition(JSContext* aCx,
>+  auto ptr = mConstructors.lookup(constructor);

Not "auto &"?   Why not?

>+CreateHTMLElement(const GlobalObject& aGlobal, const JS::CallArgs& aCallArgs,

>+  JS::Rooted<JSObject*> callee(cx, js::CheckedUnwrap(&aCallArgs.callee()));
>+  if (!callee) {
>+    return nullptr;

You need to throw something on aRv.  Otherwise this will crash.

>+  JSAutoCompartment ac(cx, callee);

Please document clearly why this line is needed.  

>+    JS::Rooted<JSObject*> constructor(cx, HTMLElementBinding::GetConstructorObject(cx));
>+    if (!constructor) {
>+      return nullptr;

You need to indicate on aRv that we're throwing, by calling aRv.NoteJSContextException() before returning.

>+    JS::Rooted<JSObject*> constructor(cx, cb(cx));
>+    if (!constructor) {
>+      return nullptr;

Needs aRv.NoteJSContextException().

>+++ b/dom/bindings/BindingUtils.h
>+// HTML element interface, the global/callargs need to be whatever was passed to

';', not ','.

>+++ b/dom/bindings/Codegen.py
>@@ -1712,39 +1712,108 @@ class CGClassConstructor(CGAbstractStati

>+                if (!newTarget) {
>+                  return false;
>+                }

Do you really mean to throw an uncatchable exception, or should you be throwing an actual exception here?  CheckenUnwrap does _not_ throw when it returns null.

>+                { // enter newTarget's compartment

Please document the _why_ not the _what_.  The "what" is clear here, but why you're doing it is not obvious to a casual reader.

This applies to both newTarget compartment bits in this function.

>+                } // leave newTarget's compartment

This is in the htmlConstructorFallback block.  The problem is that desiredProto is still in the compartment of newTarget, which will not match the compartment of the object we'll actually create.  I expect that to trigger fatal assertions.  Have you tested this codepath in the case when newTarget != &args.newTarget().toObject()?  I'm guessing not; please add a test.

>+            # Given that HTMLConstructor takes no args, so we can just codegen

s/so we/we/

>+            # a call to CreateHTMLElement() in BindingUtils which resues the

"reuses"

>@@ -7297,36 +7366,33 @@ class CGPerSignatureCall(CGThing):
>+            # If we're a constructor, global object will be created in

No, the "global object" isn't getting created anywhere.  Maybe more like:

  # If we're a constructor, the GlobalObject struct will be created in

>+++ b/dom/bindings/parser/WebIDL.py
>@@ -4532,16 +4542,20 @@ class IDLMethod(IDLInterfaceMember, IDLS

>+        # The identifier of a HTMLConstructor must be 'constrcutor'.

must be 'constructor'.

r=me with those fixed, but I'd like to see an updated patch before you land it.

Comment 71

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8811788 [details] [diff] [review]
Part 3-1: Add HTMLConstructor to HTMLElement and its subclass, v3

r=me but note that I have low confidence that I would have caught you missing an interface here.  ;)

Comment 72

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8811799 [details] [diff] [review]
Part 4: Add test cases for HTMLConstructor, v3

+++ b/dom/tests/mochitest/webcomponents/html_constructor_tests.js
>+// Test construcing a HTMLELement.

"constructing"

>+  }, 'calling an autonomous custom element consturctor should throw a TypeError');

"constructor"

>+  SimpleTest.ok(element instanceof testWindow.HTMLElement,

That's good, but a pretty weak test given that any HTML element is instaceof HTMLElement.

Please test that the actual proto chain of "element" is what one would expect.

>+  SimpleTest.ok(element instanceof testWindow.HTMLImageElement,

Same here.

The non-Xray test could really use being a web platform test, but I understand the desire to share code between the two...

r=me, but please do add those tests that would exercise having a newTarget that is not same-compartment with the HTML constructor (which you have in your tests) and doesn't return a useful object as its prototype.

Also, it would be good to have a test that goes through the list of all HTML tag names and ensures that we correctly have [HTMLConstructor] on the relevant interfaces.  Unless there are existing web platform tests that test this?

Comment 73

[William Chen [:wchen]]

Comment on attachment 8811784 [details] [diff] [review]
Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements, v5

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

::: dom/bindings/BindingUtils.cpp
@@ +3408,5 @@
> +    return nullptr;
> +  }
> +
> +  JSContext* cx = aGlobal.Context();
> +  JS::Rooted<JSObject*> newTarget(cx, &aCallArgs.newTarget().toObject());

The spec seems to indicate that the NewTarget may be undefined during this algorithm. We either need to handle that case here or make sure that it can't happen. I get a type error when trying to call a class constructor so it might be that some other spec already covers this case and the custom elements spec has some redundant language. If this case is covered by another spec, we also need to make sure those checks happen before we call CreateHTMLElement.

@@ +3425,5 @@
> +  JSAutoCompartment ac(cx, callee);
> +  int32_t tag = eHTMLTag_userdefined;
> +  if (!definition->IsCustomBuiltIn()) {
> +    // If the definition is for an autonomous custom element, the active
> +    // function should be HTMLElement.

It looks like we are missing step 2 where we check whether NewTarget is the same as the active function. We need to either handle it here (and in customized built in case) or make sure it can't happen. I get a type error when I try to directly construct an element using its interface so maybe we're already handling this case somewhere else. If we are, we may need to need to change that behavior otherwise when we create a subclass of an element (e.g. Foo extends HTMLElement {}), directly constructing the subclass (e.g. new Foo()) will throw.

::: dom/bindings/parser/tests/test_constructor.py
@@ +254,5 @@
> +        results = parser.finish()
> +    except:
> +        threw = True
> +
> +    harness.ok(threw, "Can't have both a HTMLConstructor and a ChromeConstructor")

The spec forbids [HTMLConstructor] on a callback interface, so we can add a test for that too.

::: parser/htmlparser/nsHTMLTags.h
@@ +18,5 @@
>     To change the list of tags, see nsHTMLTagList.h
>  
> +   These enum values are used as the index of array in various places.
> +   Don't forget to update dom/bindings/BindingUtils.cpp and
> +   dom/html/nsHTMLContentSink.cpp as well.

What needs to be updated in BindingUtils.cpp and nsHTMLContentSink.cpp when you make a change to the list? If you don't actually have to update those files, it should be enough to say that structures in BindingUtils.cpp and nsHTMLContentSink.cpp are affected by changes to the list of tags.

Comment 74

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #70)
> Comment on attachment 8811784 [details] [diff] [review]
> Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom
> elements, v5
> 
> >+CustomElementRegistry::LookupCustomElementDefinition(JSContext* aCx,
> >+  auto ptr = mConstructors.lookup(constructor);
> 
> Not "auto &"?   Why not?
Will use "const auto&".

> 
> >+CreateHTMLElement(const GlobalObject& aGlobal, const JS::CallArgs& aCallArgs,
> 
> >+  JSAutoCompartment ac(cx, callee);
> 
> Please document clearly why this line is needed.  
Will do.

> 
> >+    JS::Rooted<JSObject*> constructor(cx, HTMLElementBinding::GetConstructorObject(cx));
> >+    if (!constructor) {
> >+      return nullptr;
> 
> You need to indicate on aRv that we're throwing, by calling
> aRv.NoteJSContextException() before returning.
Will do.

> 
> >+++ b/dom/bindings/BindingUtils.h
> >+// HTML element interface, the global/callargs need to be whatever was passed to
> 
> ';', not ','.
Will do.

> 
> >+++ b/dom/bindings/Codegen.py
> >@@ -1712,39 +1712,108 @@ class CGClassConstructor(CGAbstractStati
> 
> >+                if (!newTarget) {
> >+                  return false;
> >+                }
> 
> Do you really mean to throw an uncatchable exception, or should you be
> throwing an actual exception here?  CheckenUnwrap does _not_ throw when it
> returns null.
I missed throwing exception for the case that CheckenUnwrap returns null. Will do. Thanks for catching this.

> 
> >+                { // enter newTarget's compartment
> 
> Please document the _why_ not the _what_.  The "what" is clear here, but why
> you're doing it is not obvious to a casual reader.
> 
> This applies to both newTarget compartment bits in this function.
Will do.

> 
> >+                } // leave newTarget's compartment
> 
> This is in the htmlConstructorFallback block.  The problem is that
> desiredProto is still in the compartment of newTarget, which will not match
> the compartment of the object we'll actually create.  I expect that to
> trigger fatal assertions.  Have you tested this codepath in the case when
> newTarget != &args.newTarget().toObject()?  I'm guessing not; please add a
> test.
Will fix it and add relevant test.

Comment 75

[Boris Zbarsky [:bzbarsky]]

> The spec seems to indicate that the NewTarget may be undefined during this algorithm.

It indicates that it may be undefined during the algorithm HTMLConstructor runs.  dom::CreateHTMLElement is only _part_ of that algorithm.  The full algorithm is the thing output by CGClassConstructor and one of the first things it does is:

             if (!args.isConstructing()) {
               return ThrowConstructorWithoutNew(cx, "${ctorName}");
             }

where isConstructing() is equivalent to checking that newTarget is not undefined.

> It looks like we are missing step 2 where we check whether NewTarget is the same as the active function.

It's in the code CGClassConstructor outputs.  See the "newTarget == constructor" bit in htmlConstructorSanityCheck.  Maybe this patch should actually label its code with the relevant algorithm step numbers like the JS folks have been doing...  Especially if it references a spec snapshot in the process.

> What needs to be updated in BindingUtils.cpp and nsHTMLContentSink.cpp when you make a change to the list?

Good catch, this needs better documentation.

What needs to be updated is that if you change the structure of the enum by adding entries to it or removing entries from it _directly_, not via nsHTMLTagList.h, then you need to adjust the other places accordingly, right?

Comment 76

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #72)
> Comment on attachment 8811799 [details] [diff] [review]
> Part 4: Add test cases for HTMLConstructor, v3
> 
> +++ b/dom/tests/mochitest/webcomponents/html_constructor_tests.js
> 
> >+  SimpleTest.ok(element instanceof testWindow.HTMLElement,
> 
> That's good, but a pretty weak test given that any HTML element is instaceof
> HTMLElement.
> 
> Please test that the actual proto chain of "element" is what one would
> expect.

Will do.

> 
> The non-Xray test could really use being a web platform test, but I
> understand the desire to share code between the two...

There is a web platform test doing similar tests, https://github.com/w3c/web-platform-tests/blob/master/custom-elements/HTMLElement-constructor.html, but only for autonomous custom element. We can contribute our tests, will do it in a separated bug or on github. 

> 
> r=me, but please do add those tests that would exercise having a newTarget
> that is not same-compartment with the HTML constructor (which you have in
> your tests) and doesn't return a useful object as its prototype.

There is a web platform test doing such tests, https://github.com/w3c/web-platform-tests/blob/master/custom-elements/htmlconstructor/newtarget.html (Note that newtarget.html wpt in gecko is not up-to-date yet, so we can not pass it due to the test itself is incorrect. Once gecko merge the latest version from upstream, we can pass it).

> 
> Also, it would be good to have a test that goes through the list of all HTML
> tag names and ensures that we correctly have [HTMLConstructor] on the
> relevant interfaces.  Unless there are existing web platform tests that test
> this?

Will add mochitest test for all HTML tag names.
Per https://github.com/whatwg/html/pull/2032, someone is working on the web platform tests.

Comment 77

[Edgar Chen [:edgar]]

(In reply to William Chen [:wchen] from comment #73)
> Comment on attachment 8811784 [details] [diff] [review]
> Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom
> elements, v5
> 
> Review of attachment 8811784 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/parser/tests/test_constructor.py
> @@ +254,5 @@
> > +        results = parser.finish()
> > +    except:
> > +        threw = True
> > +
> > +    harness.ok(threw, "Can't have both a HTMLConstructor and a ChromeConstructor")
> 
> The spec forbids [HTMLConstructor] on a callback interface, so we can add a
> test for that too.

Will do.

Comment 78

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #75)
> Maybe this patch should actually label its code with the relevant algorithm step numbers like the JS
> folks have been doing...  Especially if it references a spec snapshot in the
> process.

Labeling code with step numbers sound good.

> > What needs to be updated in BindingUtils.cpp and nsHTMLContentSink.cpp when you make a change to the list?
> 
> this needs better documentation.

Will do.

Comment 79

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #76)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #72)
> > 
> > r=me, but please do add those tests that would exercise having a newTarget
> > that is not same-compartment with the HTML constructor (which you have in
> > your tests) and doesn't return a useful object as its prototype.
> 
> There is a web platform test doing such tests,
> https://github.com/w3c/web-platform-tests/blob/master/custom-elements/
> htmlconstructor/newtarget.html (Note that newtarget.html wpt in gecko is not
> up-to-date yet, so we can not pass it due to the test itself is incorrect.
> Once gecko merge the latest version from upstream, we can pass it).

However, we need similar tests for Xray, will add.

Comment 80

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #76)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #72)
> > Comment on attachment 8811799 [details] [diff] [review]
> > Part 4: Add test cases for HTMLConstructor, v3
> > 
> > +++ b/dom/tests/mochitest/webcomponents/html_constructor_tests.js
> > 
> > >+  SimpleTest.ok(element instanceof testWindow.HTMLElement,
> > 
> > That's good, but a pretty weak test given that any HTML element is instaceof
> > HTMLElement.
> > 
> > Please test that the actual proto chain of "element" is what one would
> > expect.
> 
> Will do.

For the test case

  class X extends contentWindow.HTMLElement {}
  contentWindow.define("x-foo", X);
  var element = new X();

I test the proto chain of element by

  SimpleTest.is(Object.getPrototypeOf(element), X.prototype, "...");

But it doesn't get passed in Xray case.

And actually we have same behavior in normal constructor, for example

  class FooError extends contentWindow.DOMError {
    constructor() {
      super("foo");
    }
  }
  var error = new FooError();

  SimpleTest.is(Object.getPrototypeOf(error), FooError.prototype, "..."); // Failed ...
  SimpleTest.is(Object.getPrototypeOf(error), contentWindow.DOMError.prototype, "..."); // Passed ...

It seems because the desiredProto is in chrome compartement and we wrap it into content compartment [1] then something wrong when setting
it as the prototype to JS reflector of a DOM object (probably we don't allow doing this?). I am not sure what's exactly happen ...
Is this behavior what we expect?

(BTW, I have a hacky patch to set desiredProto as the prototype to the wrapper, not JS reflector. And we can pass FooError.prototype checks, but again I am not sure if it is what we expect) 

[1] http://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/DOMErrorBinding.cpp#182
[2] http://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/DOMErrorBinding.cpp#356

Comment 81

[Boris Zbarsky [:bzbarsky]]

> Is this behavior what we expect?

What actually happens in your DOMError testcase is that the object that gets constructed is a content-side DOMError whose proto is a cross-compartment wrapper for FooError.prototype.  Then what's returned and stored in "error" is an Xray to that object.

When you do Object.getPrototypeOf() on an Xray it completely ignores the actual prototype of the target object, because script can mutate that via __proto__ or Object.setPrototypeOf and we don't want such mutations to affect chrome code.  Instead what it does is look up the default proto for objects of that sort, and returns an Xray to that.  So your Object.getPrototypeOf(error) call returns an Xray for contentWindow.DOMError.prototype.

Note that this would also happen if FooError were defined in content as a subclass of the content DOMError.  In other words, if content has:

  class FooError extends DOMError {
    constructor() {
      super("foo");
    }
  }
  var error = new FooError();
  passErrorToChromeCode(error); // creates xrayForError on the chrome side

and then chrome JS implementing passErrorToChromeCode examines Object.getPrototypeOf(xrayForError), it would get an Xray for the content-side DOMError.prototype, not FooError.prototype.

Going back to your original test, what you probably want to test instead is:

   SimpleTest.is(Object.getPrototypeOf(Cu.waiveXrays(element)), X.prototype, "...");

Comment 82

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #81)
>    SimpleTest.is(Object.getPrototypeOf(Cu.waiveXrays(element)), X.prototype,
> "...");

Using Cu.waiveXrays() works. Thanks for the explanation.

Comment 83

[Edgar Chen [:edgar]]

Attachment: Part 1: Support looking up definitions by using constructor as a key, v6, r=wchen,jonco

Rebase.

Comment 84

[Edgar Chen [:edgar]]

Attachment: Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements, v6

Addressed review comment #70 and comment #73:
- auto& ptr
- throw something on aRv when CheckenUnwrap returns nullptr
- call aRv.NoteJSContextException() before returning
- document why we need to unwrap newTarget/callee and enter into it's compartment
- fix typo
- add parser test for [HTMLConstructor] on a callback interface
- labeling code with step numbers in CreateHTMLElement

Comment 85

[Edgar Chen [:edgar]]

Attachment: Part 3-1: Add HTMLConstructor to HTMLElement and its subclass, v4, r=bz

Rebase.

Comment 86

[Edgar Chen [:edgar]]

Attachment: Part 3-2: Update web-platform-test expected result, v2, r=wchen

Rebase.

Comment 87

[Edgar Chen [:edgar]]

Attachment: Part 4: Add test cases for HTMLConstructor, v4

Address review comment #72:
- fix typo
- test proto chain by using is(Object.getPrototypeof(x), ...)
- add tests for constructor doesn't return a useful object as its prototype
- add tests for all HTML tag

Comment 88

[Edgar Chen [:edgar]]

Comment on attachment 8816407 [details] [diff] [review]
Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements, v6

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

Hi William, could you review this new version (see comment #84) again? Thank you.

Comment 89

[Edgar Chen [:edgar]]

Comment on attachment 8816410 [details] [diff] [review]
Part 4: Add test cases for HTMLConstructor, v4

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

Hi William, I add more mochitest tests (see comment #87), could you review this new version again? Thank you.

Comment 90

[Edgar Chen [:edgar]]

Hi Boris, could you help to review part 2-2 (comment #84) and part 4 (comment #87) again? Thank you.

Comment 91

[Edgar Chen [:edgar]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a98d373be55a6273a9a331be6cd536111e4e4d3&group_state=expanded&filter-tier=1

Comment 92

[Boris Zbarsky [:bzbarsky]]

Can you please post interdiffs from the last bits that I reviewed?

Comment 93

[Edgar Chen [:edgar]]

Attachment: interdiff_part2-2_v5_v6

interdiff_part2-2_v5_v6

Comment 94

[Edgar Chen [:edgar]]

Attachment: interdiff_part4_v3_v4

Comment 95

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #92)
> Can you please post interdiffs from the last bits that I reviewed?

Done.
The interdiff of part 4 might not have too much help because I made a lot of changes in part 4 in order to add tests for all html tags.

Comment 96

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8816407 [details] [diff] [review]
Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements, v6

>+++ b/dom/bindings/BindingUtils.cpp
>+  // And the actual callee might be in different compartment, so enter into it's
>+  // compartment to get correct constructor object for checking.

"its compartment".

Also, the end should perhaps be:

  so enter its compartment before getting the standard constructor
  object to compare to, so we get it from the same global as callee itself.

>+++ b/dom/bindings/Codegen.py
>@@ -1712,39 +1712,115 @@ class CGClassConstructor(CGAbstractStati
>+                // The newTarge might be an Xray wrapper or cross-compartment wrapper.
>+                // Unwrap it to get actual newTarget.

How about:

  // The newTarget might be a cross-compartment wrapper. Get the underlying object
  // so we can do the spec's object-identity checks.

>+                // And the actual newTarget might be in different compartment, so enter into it's
>+                // compartment to get correct constructor object for checking.

How about:

  // Enter the compartment of our underlying newTarget object, so we end
  // up comparing to the constructor object for our interface from that global.

or so.

>+                  // desiredProto is in the compartment of newTarget and it might not match
>+                  // the compartment of the object we'll actually create. But it is okay
>+                  // because we will wrap it in Wrap().

Uh, no.  Wrap()'s invariant is that the incoming thing is in the context compartment.  It's not asserting that, which is a bug; I filed bug 1321835 on it.

In any case, please do what I asked you to do in my review comments: desiredProto needs to be wrapped into the context compartment once you exit newTarget's compartment.

>+++ b/parser/htmlparser/nsHTMLTags.h
>+   If we change the structure of the enum by adding entries to it or removing
>+   entries from it _directly_, not via nsHTMLTagList.h. Don't forget to update

After "nsHTMLTagList.h", you want a comma, not a period.

Comment 97

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8816410 [details] [diff] [review]
Part 4: Add test cases for HTMLConstructor, v4

r=me

Comment 98

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #96)
> Comment on attachment 8816407 [details] [diff] [review]
> Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom
> elements, v6
> 
> "its compartment".
> 
> Also, the end should perhaps be:
> 
>   so enter its compartment before getting the standard constructor
>   object to compare to, so we get it from the same global as callee itself.

Done.

> How about:
> 
>   // The newTarget might be a cross-compartment wrapper. Get the underlying
> object
>   // so we can do the spec's object-identity checks.

Done.

> How about:
> 
>   // Enter the compartment of our underlying newTarget object, so we end
>   // up comparing to the constructor object for our interface from that
> global.

Done.

> desiredProto needs to be wrapped into the context compartment once you exit
> newTarget's compartment.

Done.

> After "nsHTMLTagList.h", you want a comma, not a period.

Done.

Thanks for the review.

Comment 99

[Edgar Chen [:edgar]]

Attachment: Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements, v7

Addressed review comment #96.

Comment 100

[Edgar Chen [:edgar]]

Attachment: interdiff_part2-2_v6_v7

Comment 101

[Boris Zbarsky [:bzbarsky]]

Thank you for the interdiff, though the ANSI color escapes really confused the browser's encoding detection... ;)

One last tweak I would like to see: for the "JSAutoCompartment ac(cx, newTarget)" right before "desiredProto = GetProtoObjectHandle(cx)", please document why we want to be in that compartment.  Maybe this part, and the part in htmlConstructorSanityCheck, should also cite steps in the spec algorithm....

Comment 102

[Edgar Chen [:edgar]]

Attachment: interdiff_part2-2_v6_v7

interdiff without the ANSI color escapes

Comment 103

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #101)
> One last tweak I would like to see: for the "JSAutoCompartment ac(cx,
> newTarget)" right before "desiredProto = GetProtoObjectHandle(cx)", please
> document why we want to be in that compartment.  Maybe this part, and the
> part in htmlConstructorSanityCheck, should also cite steps in the spec
> algorithm....

Done.

Comment 104

[Edgar Chen [:edgar]]

Attachment: Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements, v8, r=wchen

Addressed review comment #101.

Comment 105

[Edgar Chen [:edgar]]

Attachment: interdiff_part2-2_v7_v8

Comment 106

[Edgar Chen [:edgar]]

Comment on attachment 8817107 [details] [diff] [review]
Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements, v8, r=wchen

Could you help to take a look at this part again? I have addressed comment #101 and the interdiff is attachment #8817108 [details] [diff] [review]. Thank you.

Comment 107

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8817108 [details] [diff] [review]
interdiff_part2-2_v7_v8

r=me

Comment 108

[Edgar Chen [:edgar]]

Attachment: Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements, v8, r=wchen

Ah, I just realized I attached a wrong patch. Sorry about that. Here is the correct one.

Comment 109

[Edgar Chen [:edgar]]

Try result with recent m-c: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43cfbf58473d541bb99e3431e033277122007ab9&group_state=expanded&filter-tier=1

Comment 110

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33c69deecb7a
Part 1: Support looking up definitions by using constructor as a key; r=wchen,jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3a6908b84e
Part 2-1: Include the name of relevant interface in nsHTMLTagList.h; r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2a5fc3e8c4e
Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements; r=bz,wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/32b1721d1125
Part 3-1: Add HTMLConstructor to HTMLElement and its subclass; r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/bde709210e59
Part 3-2: Update web-platform-test expected result; r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/86d70a2e93e1
Part 4: Add test cases for HTMLConstructor; r=bz,wchen

Comment 111

[Edgar Chen [:edgar]]

Boris, William and Jon,
Thanks for the review effort.

Comment 112

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for frequently failing test_custom_element_htmlconstructor.html on Android 4.3 debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd390860c6c90d477063f641c9181b9d6ab6dd64
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bef3e41a9c4041270a66ee33666525098d9023e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d423f3a721a7165eaa18db503d384256ba7f12b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d423f3a721a7165eaa18db503d384256ba7f12b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/650b23385ab00daa4b2ed1bfd32b2a88ae9b6bec
https://hg.mozilla.org/integration/mozilla-inbound/rev/bac27f774b129f3254affad3121e630b14f93fc2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=86d70a2e93e1c1fa3b500e3ab648d1e61db7a438
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40563723&repo=mozilla-inbound

[task 2016-12-14T07:09:13.909251Z] 07:09:13     INFO -  267 INFO TEST-PASS | dom/tests/mochitest/webcomponents/test_custom_element_htmlconstructor.html | constructing a customized HTMLPreElement; the element should be owned by the registry's associated document
[task 2016-12-14T07:09:13.909627Z] 07:09:13     INFO -  268 INFO TEST-PASS | dom/tests/mochitest/webcomponents/test_custom_element_htmlconstructor.html | constructing a customized HTMLPreElement; the constructor should have been invoked once
[task 2016-12-14T07:09:13.910085Z] 07:09:13     INFO -  269 INFO TEST-PASS | dom/tests/mochitest/webcomponents/test_custom_element_htmlconstructor.html | constructing a customized HTMLPreElement; if prototype is not object, fallback from NewTarget's realm
[task 2016-12-14T07:09:13.910449Z] 07:09:13     INFO -  Buffered messages finished
[task 2016-12-14T07:09:13.910800Z] 07:09:13     INFO -  270 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/webcomponents/test_custom_element_htmlconstructor.html | Test timed out.
[task 2016-12-14T07:09:13.910975Z] 07:09:13     INFO -      reportError@SimpleTest/TestRunner.js:114:7
[task 2016-12-14T07:09:13.911171Z] 07:09:13     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:135:7
[task 2016-12-14T07:09:13.911366Z] 07:09:13     INFO -  271 ERROR [SimpleTest.finish()] this test already called finish!
[task 2016-12-14T07:09:13.911606Z] 07:09:13     INFO -  272 INFO TEST-UNEXPECTED-ERROR | dom/tests/mochitest/webcomponents/test_custom_element_htmlconstructor.html | called finish() multiple times

Comment 113

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39947ae430cc
Backed out changeset 32b1721d1125

Comment 114

[Edgar Chen [:edgar]]

Attachment: Part 4: Add test cases for HTMLConstructor, v5, r=wchen,bz

Android tests are running on emulator which is quite slow, so I disable it on android first and filed bug 1323645 to follow up.

Try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f02b05975921cef60f54348be2b59a8e26749ff&group_state=expanded&filter-tier=1

Comment 115

[Edgar Chen [:edgar]]

Attachment: Part 1: Support looking up definitions by using constructor as a key, v7, r=wchen,jonco

Rebase.

Comment 116

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec9360f803c8
Part 1: Support looking up definitions by using constructor as a key; r=wchen,jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/20fce80b97eb
Part 2-1: Include the name of relevant interface in nsHTMLTagList.h; r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cde2ad83e60
Part 2-2: Support HTMLConstructor WebIDL extended attribute for custom elements; r=bz,wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/fab12859e2b0
Part 3-1: Add HTMLConstructor to HTMLElement and its subclass; r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0305d9bb203
Part 3-2: Update web-platform-test expected result; r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed19f769c77a
Part 4: Add test cases for HTMLConstructor; r=bz,wchen

Comment 117

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ec9360f803c8
https://hg.mozilla.org/mozilla-central/rev/20fce80b97eb
https://hg.mozilla.org/mozilla-central/rev/8cde2ad83e60
https://hg.mozilla.org/mozilla-central/rev/fab12859e2b0
https://hg.mozilla.org/mozilla-central/rev/d0305d9bb203
https://hg.mozilla.org/mozilla-central/rev/ed19f769c77a