1309147 - Add [CEReactions] to support custom element reactions

Status: RESOLVED FIXED

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

Description

[John Dai[:jdai]]

According to spec [1], we introduce the [CEReactions] IDL extended attribute. It indicates that the relevant algorithm is to be supplemented with additional steps in order to appropriately track and invoke custom element reactions.

[CEReactions] IDL extended attribute is introduced by following specs:
https://dom.spec.whatwg.org/
https://html.spec.whatwg.org/
http://w3c.github.io/selection-api/
https://w3c.github.io/DOM-Parsing/

[1] https://html.spec.whatwg.org/multipage/scripting.html#cereactions

Comment 1

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 1: Add CEReactions for CustomElementRegistry. v1

Firstly, I support CEReactions for CustomElementRegistry, and I support Codegen and WebIDL parser for operation, attribute, setter, and deleter. I would like to have your feedback to see if I am on the right direction.

Comment 2

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 2: Test CEReactions for CustomElementRegistry. v1

Comment 3

[John Dai[:jdai]]

Comment on attachment 8829340 [details] [diff] [review]
Bug 1309147 - Part 1: Add CEReactions for CustomElementRegistry. v1

Per offline discussion, I'll split patches into small parts and add more parser tests. Cancel feedback first.

Comment 4

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL Bindings. v1

Comment 5

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v1

Comment 6

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 3: Add CEReactions for CustomElementRegistry. v1

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

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 8833220 [details] [diff] [review]
Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL Bindings. v1

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

::: dom/bindings/parser/WebIDL.py
@@ +4277,5 @@
>              if self.isStatic():
>                  raise WebIDLError("[Unscopable] is only allowed on non-static "
>                                    "attributes and operations",
>                                    [attr.location, self.location])
> +        elif identifier == "CEReactions":

noArguments check here as well and add tests.

@@ +4278,5 @@
>                  raise WebIDLError("[Unscopable] is only allowed on non-static "
>                                    "attributes and operations",
>                                    [attr.location, self.location])
> +        elif identifier == "CEReactions":
> +            if self.readonly and not self.getExtendedAttribute("PutForwards"):

This works correct only if [PutForwards] come before [CEReactions], but [PutForwards] might come after [CEReactions], e.g.

  [CEReactions, PutForwards=A] readonly attribute I b;

in such case, it will throw error, but it should not. Maybe putting the checks in validate() is more proper? And please add tests.

@@ +5022,5 @@
> +                                  [attr.location])
> +
> +            if self.isSpecial() and not self.isSetter() and not self.isDeleter():
> +                raise WebIDLError("[CEReactions] is only allowed on setter or deleter "
> +                                  "attributes and operations",

"[CEReactions] is only allowed on operation, attribute, setter, and deleter"

::: dom/bindings/parser/tests/test_cereactions.py
@@ +77,5 @@
> +    threw = False
> +    try:
> +        parser.parse("""
> +          interface Foo {
> +            [CEReactions] stringifier DOMString ();

Add one more test case for jsonifier, too?

Comment 8

[Edgar Chen [:edgar]]

Comment on attachment 8833221 [details] [diff] [review]
Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v1

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

::: dom/bindings/Codegen.py
@@ +7877,1 @@
>          if len(signatures) == 1:

It is not clear to me why handle [CEReactions] only on one signature method? What about the method that has more than one signature? e.g.

  [CEReactions] void foo();
  [CEReactions] void foo(DOMString bar);

Comment 9

[Edgar Chen [:edgar]]

Comment on attachment 8833221 [details] [diff] [review]
Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v1

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

Could you explain a bit why touching CGDOMJSProxyHandler_defineProperty, CGDeleteNamedProperty, CGDOMJSProxyHandler_delete and CGDOMJSProxyHandler_setCustom?
And please make sure each of changes has relevant test.

::: dom/bindings/test/TestExampleGen.webidl
@@ +784,5 @@
>    [NeedsSubjectPrincipal] void needsSubjectPrincipalMethod();
>    [NeedsSubjectPrincipal] attribute boolean needsSubjectPrincipalAttr;
>    [NeedsCallerType] void needsCallerTypeMethod();
>    [NeedsCallerType] attribute boolean needsCallerTypeAttr;
> +  [CEReactions] void cEReactionsMethod();

s/cEReactions/ceReactions/, here and elsewhere.

@@ +788,5 @@
> +  [CEReactions] void cEReactionsMethod();
> +  [CEReactions] attribute boolean cEReactionsAttr;
> +  [CEReactions] setter void (DOMString name, DOMString value);
> +  [CEReactions] deleter void (DOMString name);
> +  [PutForwards=writableByte, CEReactions] readonly attribute TestInterface putForwardsCEReactionsAttr;

s/cEReactions/ceReactions/, here and elsewhere.

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 8833222 [details] [diff] [review]
Bug 1309147 - Part 3: Add CEReactions for CustomElementRegistry. v1

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

::: dom/base/CustomElementRegistry.h
@@ +373,5 @@
>    public:
>      explicit AutoCEReaction(CustomElementRegistry* aRegistry)
>        : mRegistry(aRegistry) {
> +      if (mRegistry) {
> +        mRegistry->CreateAndPushElementQueue();

For no registry case (global object is not a window), I think we should not create AutoCEReaction at all, instead of creating an AutoCEReaction that do nothing. You can use Maybe<> in binding code, e.g.

  Maybe<AutoCEReaction> ceReaction;
  if (registry) {
    ceReaction.emplace(registry);
  }

Comment 11

[Edgar Chen [:edgar]]

Comment on attachment 8833221 [details] [diff] [review]
Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v1

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

::: dom/bindings/test/TestCodeGen.webidl
@@ +957,5 @@
> +  [CEReactions] void cEReactionsMethod();
> +  [CEReactions] attribute boolean cEReactionsAttr;
> +  [CEReactions] setter void (DOMString name, DOMString value);
> +  [CEReactions] deleter void (DOMString name);
> +  [PutForwards=writableByte, CEReactions] readonly attribute TestInterface putForwardsCEReactionsAttr;

Add same tests to TestJSImpGen per http://searchfox.org/mozilla-central/rev/848c29538ab007fb95dc6cff194f0e3d3809613d/dom/bindings/test/TestCodeGen.webidl#971?

Comment 12

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL Bindings. v2

Addressed comment #7.

Comment 13

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v2

Addressed comment #8, comment #9, and comment #11.

> Could you explain a bit why touching CGDOMJSProxyHandler_defineProperty, CGDeleteNamedProperty, CGDOMJSProxyHandler_delete and CGDOMJSProxyHandler_setCustom?
> And please make sure each of changes has relevant test.

After I tested on my local machine, CGDOMJSProxyHandler_delete and CGDOMJSProxyHandler_setCustom will be called. If I misunderstanding anything, please point out.

Comment 14

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 3: Add CEReactions for CustomElementRegistry. v2

Addressed comment #10.

Comment 15

[Edgar Chen [:edgar]]

Comment on attachment 8835931 [details] [diff] [review]
Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL Bindings. v2

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

> # HG changeset patch
> # User John Dai <[e-mail]>
> Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL Bindings.
Could you revise commit message to "Implement the support for CEReactions in WebIDL parser" which match what this patch does more?

::: dom/bindings/parser/tests/test_cereactions.py
@@ +3,5 @@
> +    try:
> +        parser.parse("""
> +            interface Foo {
> +              [CEReactions(DOMString a)] void foo(boolean arg2);
> +              [CEReactions(DOMString b)] readonly attribute boolean bar;

This doesn't actually test the attribute case since we will hit error on foo() first. Split them to two tests.

@@ +25,5 @@
> +            };
> +            interface Person {
> +              [CEReactions, PutForwards=full] readonly attribute Name A;
> +              [PutForwards=full, CEReactions] readonly attribute Name B;
> +              attribute unsigned short age;

Nit: remove unecessary attribute?

@@ +34,5 @@
> +    except Exception as inst:
> +        print inst
> +        threw = True
> +
> +    harness.ok(not threw, "Shouldn't have thrown for [CEReactions] used on PutForwards")

Nit: What about harness.ok(False, ..) when got exception? e.g. 

  parser = parser.reset();  
  try:
      parser.parse("""
          ...
      """)
  except Exception, e:
      harness.ok(False, "... %s" % e)

which probably can make error message clear

@@ +116,5 @@
> +    threw = False
> +    try:
> +        parser.parse("""
> +            interface Foo {
> +                            readonly attribute DOMString deviceId;

Nit: alignment or remove this unecessary attribute.

Comment 16

[Edgar Chen [:edgar]]

Comment on attachment 8835931 [details] [diff] [review]
Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL Bindings. v2

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

[CEReaction] isn't allowed on a interface, please also add tests for that.

i.e.
 [CEReaction]
 interface Foo {
  ...
 }

Comment 17

[Edgar Chen [:edgar]]

Comment on attachment 8835934 [details] [diff] [review]
Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v2

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

You need to handle [CEReactions] in CGDOMJSProxyHandler_defineProperty which also calls NamedSetter or IndexedSetter.

::: dom/bindings/Codegen.py
@@ +7882,5 @@
>              self.cgRoot = CGList([getPerSignatureCall(signature)])
>              requiredArgs = requiredArgCount(signature)
>  
> +            if method.getExtendedAttribute('CEReactions') is not None:
> +                self.cgRoot.prepend(CGGeneric(dedent(

If we move CEReaction stuff outside the if statement, we could share it and don't need to have a duplicated one in overload case.

@@ +9159,5 @@
>                                                          self.attr)
> +        ceReaction = ""
> +        if self.attr.getExtendedAttribute("CEReactions"):
> +            ceReaction = dedent(
> +            """

Indentation,

ceReaction = dedent(
    """
    CustomElementRegistry* registr ...
    """)

@@ +9214,5 @@
>          forwardToAttrName = self.attr.getExtendedAttribute("PutForwards")[0]
> +        ceReaction = ""
> +        if self.attr.getExtendedAttribute("CEReactions"):
> +            ceReaction = dedent(
> +            """

Indentation.

@@ +11716,5 @@
>  
>      def getBody(self):
> +        def getCEReactionsBody():
> +            return dedent(
> +            """

Indentation.

@@ +11734,5 @@
> +        namedDeleter = self.descriptor.operations['NamedDeleter']
> +        indexedDeleter = self.descriptor.operations['IndexedDeleter']
> +        if ((namedDeleter is not None and namedDeleter.getExtendedAttribute("CEReactions")) or
> +            (indexedDeleter is not None and indexedDeleter.getExtendedAttribute("CEReactions"))):
> +            delete += getCEReactionsBody()

Could we handle [CEReactions] in CGProxySpecialOperation which is a base class for indexed or named special operation? Then CEReactions stuff don't need to be scattered in CGDOMJSProxyHandler_*. (This approach also fix the missing in CGDOMJSProxyHandler_defineProperty)

But we need to fingure out does CGDeleteNamedProperty need this changes since CGDeleteNamedProperty will use CGProxyNamedDeleter.

@@ +12064,5 @@
>  
>      def getBody(self):
> +        def getCEReactionsBody():
> +            return dedent(
> +            """

Indentation.

@@ -12254,5 @@
>          if descriptor.interface.getExtendedAttribute('OverrideBuiltins'):
>              parentClass = 'ShadowingDOMProxyHandler'
>          else:
>              parentClass = 'mozilla::dom::DOMProxyHandler'
> -

Remove this unnecessary change.

::: dom/bindings/test/TestCodeGen.webidl
@@ +1287,5 @@
> +  [CEReactions] setter creator void (DOMString name, DOMString item);
> +  [CEReactions] deleter void (DOMString name);
> +  getter long item(unsigned long index);
> +  getter DOMString (DOMString name);
> +  readonly attribute unsigned long length;

Remove this unnecessary attribute?

Comment 18

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL Bindings. v3

Addressed comment #15.

Comment 19

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v3

Addressed comment #17.

> +            };
> +            interface Person {
> +              [CEReactions, PutForwards=full] readonly attribute Name A;
> +              [PutForwards=full, CEReactions] readonly attribute Name B;
> +              attribute unsigned short age;
> Nit: remove unecessary attribute?

We can't remove length attribute, otherwise codegen won't generate corresponding binding code. In webidl spec, it said "If an interface has a setter of a given variety, then it MUST also have a getter of that variety. If it has a named property deleter, then it MUST also have a named property getter."[1] If I misunderstanding anything, please point out.

[1] https://www.w3.org/TR/WebIDL-1/#idl-special-operations

Comment 20

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #19)
> Created attachment 8837500 [details] [diff] [review]
> Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v3
> 
> Addressed comment #17.
> 
> > +            };
> > +            interface Person {
> > +              [CEReactions, PutForwards=full] readonly attribute Name A;
> > +              [PutForwards=full, CEReactions] readonly attribute Name B;
> > +              attribute unsigned short age;
> > Nit: remove unecessary attribute?
> 
> We can't remove length attribute, otherwise codegen won't generate
> corresponding binding code. In webidl spec, it said "If an interface has a
> setter of a given variety, then it MUST also have a getter of that variety.
> If it has a named property deleter, then it MUST also have a named property
> getter."[1] If I misunderstanding anything, please point out.
> 
> [1] https://www.w3.org/TR/WebIDL-1/#idl-special-operations

Oops, I pasted wrong piece of comment. It should be the right one.

> +  [CEReactions] setter creator void (DOMString name, DOMString item);
> +  [CEReactions] deleter void (DOMString name);
> +  getter long item(unsigned long index);
> +  getter DOMString (DOMString name);
> +  readonly attribute unsigned long length;
> Remove this unnecessary attribute?

We can't remove those attributes, otherwise codegen won't generate corresponding binding code. In webidl spec, it said "If an interface has a setter of a given variety, then it MUST also have a getter of that variety. If it has a named property deleter, then it MUST also have a named property getter."[1] If I misunderstanding anything, please point out.

[1] https://www.w3.org/TR/WebIDL-1/#idl-special-operations

Comment 21

[Edgar Chen [:edgar]]

(In reply to John Dai[:jdai] from comment #20)
> > +  readonly attribute unsigned long length;
> > Remove this unnecessary attribute?
> 
> We can't remove those attributes, otherwise codegen won't generate
> corresponding binding code.

Yeah, indexed getter needs 'length' attribute [1].

[1] http://searchfox.org/mozilla-central/rev/ac8a72f2d5204ced2004c93a9c193430c63a6a71/dom/bindings/parser/WebIDL.py#1351-1354

Comment 22

[Edgar Chen [:edgar]]

Comment on attachment 8837495 [details] [diff] [review]
Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL Bindings. v3

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

::: dom/bindings/parser/WebIDL.py
@@ +4099,1 @@
>      def handleExtendedAttribute(self, attr):

Nit: add a blank line before "def handleExtendedAttribute(self, attr):".

::: dom/bindings/parser/tests/test_cereactions.py
@@ +47,5 @@
> +    except Exception, e:
> +        harness.ok(False, "Shouldn't have thrown for [CEReactions] used on PutForwards. %s" % e)
> +        threw = True
> +
> +    harness.ok(True, "[CEReactions] used on PutForwards without error")

This check is always passed, do we need it?

Comment 23

[Edgar Chen [:edgar]]

Comment on attachment 8837500 [details] [diff] [review]
Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v3

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

::: dom/bindings/Codegen.py
@@ +7874,5 @@
>                                        method,
>                                        argConversionStartsAt=argConversionStartsAt,
>                                        isConstructor=isConstructor,
>                                        useCounterName=useCounterName)
> +        def getCEReactionsBody():

Nit: blank line around function, i.e.

  (blank line)
  def getCEReactionsBody():
      return ...
  (blank line)

@@ +11127,5 @@
>          # CGPerSignatureCall won't do any argument conversion of its own.
>          CGPerSignatureCall.__init__(self, returnType, arguments, nativeName,
>                                      False, descriptor, operation,
>                                      len(arguments), resultVar=resultVar)
> +        if ((operation.isSetter() or operation.isDeleter()) and

Nit: one blank line before if.

@@ -12013,5 @@
>  
>      def getBody(self):
>          assertion = ("MOZ_ASSERT(!xpc::WrapperFactory::IsXrayWrapper(proxy),\n"
>                       '           "Should not have a XrayWrapper here");\n')
> -

revert this change.

@@ +13922,5 @@
>          def descriptorDeprecated(desc):
>              iface = desc.interface
>              return any(m.getExtendedAttribute("Deprecated") for m in iface.members + [iface])
>  
> +        def descriptorCEReactions(desc):

s/descriptorCEReactions/descriptorHasCEReactions/

::: dom/bindings/test/TestCodeGen.webidl
@@ +1287,5 @@
> +  [CEReactions] setter creator void (DOMString name, DOMString item);
> +  [CEReactions] deleter void (DOMString name);
> +   getter long item(unsigned long index);
> +   getter DOMString (DOMString name);
> +   readonly attribute unsigned long length;

Nit: aligment

  [CEReactions] ....
  [CEReactions] deleter void (DOMString name);
  getter long item(unsigned long index);
  getter DOMString (DOMString name);
  readonly attribute unsigned long length;

Comment 24

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL Bindings. v4

Comment 25

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v4

Comment 26

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 3: Add CEReactions for CustomElementRegistry. v3

Hi Boris and William,
In these patches, I support [CEReactions] in WebIDL and Codegen. Currently, [CEReactions] is added in CustomElementRegistry, others will be added in bug 1340027. Could you help me review these patches? Thank you.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8837922 [details] [diff] [review]
Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL Bindings. v4

>+                raise WebIDLError("[CEReactions] only allowed on "
>+                                  "readonly attributes is also "
>+                                  "annotated with [PutForwards]",

s/is also/which are also/

>+    harness.ok(threw, "Should have thrown for [CEReactions] takes arguments")

"Should have thrown for [CEReactions] with an argument".  Same in the other place where you have this.

>+            [CEReactions]
>+            interface Foo {
>+             }

Take out the extra space before the '}'.

>+               "Should have thrown for [CEReactions] used on a named legacycaller")

It's just a legacycaller, not a named legacycaller.

>+               "Should have thrown for [CEReactions] used on a named stringifier")

s/named //

>+    harness.ok(threw, "Should have thrown for [CEReactions] used on a named jsonifier")

s/named //

I don't see tests for writable attributes and regular operations, on which [CEReactions] should be allowed.  Please add those.

r=me with that.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8837924 [details] [diff] [review]
Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v4

>+GetCustomElementRegistry(JSContext* aCx, JS::Handle<JSObject*> aObj)

Why is the relevant custom element registry the one for aObj's underlying object, not the current compartment?  Or does it not really matter?  Per spec, we should be pushing a queue that applies across an entire "unit of related similar-origin browsing contexts", right?

In any case, using GlobalObject is the wrong tool here.  You want xpc::WindowOrNull; the only question is what you pass to it.

>+            if method.getExtendedAttribute('CEReactions') is not None:
>+                self.cgRoot.prepend(getCEReactionsBody())

Should this be going up at the top before argument conversions like this?  Or should it happen after argument conversions?  Or does it not matter?  I filed https://github.com/whatwg/html/issues/2362 to clarify this in the spec too.

>@@ -9263,36 +9288,48 @@ class CGSpecializedForwardingSetter(CGSpecializedSetter):

It seems a bit unlikely that we want the AutoCEReaction to go on the stack _after_ the JS_GetProperty but before the JS_SetProperty here....

I'm not sure you need a new interface for testing [CEReactions], except for the setter/deleter bits.  Why can't you just test the rest on TestInterface, and similarly on TestJSImplInterface/TestExampleInterface?

Comment 29

[Boris Zbarsky [:bzbarsky]]

Also, can you attach the resulting codegen, once we get the above pieces sorted out?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8837927 [details] [diff] [review]
Bug 1309147 - Part 3: Add CEReactions for CustomElementRegistry. v3

r=me

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8837924 [details] [diff] [review]
Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v4

r- pending answers to my questions.

Comment 32

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #28)
> Comment on attachment 8837924 [details] [diff] [review]
> Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v4
> 
> >+GetCustomElementRegistry(JSContext* aCx, JS::Handle<JSObject*> aObj)
> 
> Why is the relevant custom element registry the one for aObj's underlying
> object, not the current compartment?  Or does it not really matter?  Per
> spec, we should be pushing a queue that applies across an entire "unit of
> related similar-origin browsing contexts", right?
> 

If I understand correctly, the purpose of [CEReactions] is to make sure that we will synchronize doing the following steps. [1]
Therefore, we need to push an element queue (when doing CEReactions ctor[2]) to the right custom element reactions stack in the correct global object which has correct custom element registry.

[1] https://html.spec.whatwg.org/multipage/scripting.html#enqueue-an-element-on-the-appropriate-element-queue
[2] https://html.spec.whatwg.org/multipage/scripting.html#cereactions

> In any case, using GlobalObject is the wrong tool here.  You want
> xpc::WindowOrNull; the only question is what you pass to it.
> 
> >+            if method.getExtendedAttribute('CEReactions') is not None:
> >+                self.cgRoot.prepend(getCEReactionsBody())
> 
> Should this be going up at the top before argument conversions like this? 
> Or should it happen after argument conversions?  Or does it not matter?  I
> filed https://github.com/whatwg/html/issues/2362 to clarify this in the spec
> too.
> 
> >@@ -9263,36 +9288,48 @@ class CGSpecializedForwardingSetter(CGSpecializedSetter):
> 
> It seems a bit unlikely that we want the AutoCEReaction to go on the stack
> _after_ the JS_GetProperty but before the JS_SetProperty here....
> 
> I'm not sure you need a new interface for testing [CEReactions], except for
> the setter/deleter bits.  Why can't you just test the rest on TestInterface,
> and similarly on TestJSImplInterface/TestExampleInterface?

Comment 33

[Boris Zbarsky [:bzbarsky]]

> is to make sure that we will synchronize doing the following steps.

Yes, I agree.  So per spec, the thing maintained by CEReactions is a stack of element queues; each element queue contains elements.

> to the right custom element reactions stack in the correct global object

The spec makes no reference to specific global objects here.  There is a single custom element reactions stack per unit of related similar-origin browsing contexts.  In Gecko terms, that's per DocGroup, right?  Why are we storing these stacks per-global, and is the difference in behavior observable?

Comment 34

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #33)
> > is to make sure that we will synchronize doing the following steps.
> 
> Yes, I agree.  So per spec, the thing maintained by CEReactions is a stack
> of element queues; each element queue contains elements.
> 
> > to the right custom element reactions stack in the correct global object
> 
> The spec makes no reference to specific global objects here.  There is a
> single custom element reactions stack per unit of related similar-origin
> browsing contexts.  In Gecko terms, that's per DocGroup, right?  Why are we
> storing these stacks per-global, and is the difference in behavior
> observable?

You are right. I need to change my code to align Gecko implementation. Thanks for pointing out the problems.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8837924 [details] [diff] [review]
Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v4

OK, so...

1)  [CEReactions] is no longer allowed on [PutForwards] readonly attributes, because it's never needed there.

2)  The spec is now clear that the CEReactions thing happens around the actual call to the Gecko C++ code, not up front before argument conversions and stuff.  You should probably align with that.

See https://github.com/whatwg/html/pull/2378 for details.

Comment 36

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL Bindings. v5

In this patch, I modified WebIDL.py to align spec.

Comment 37

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 2: Implement the support for CEReactions in Codegen. v5

Addressed comment #28 and comment #35.

Comment 38

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 3: Add CEReactions for CustomElementRegistry. v4

Addressed comment #33, move custom element reactions stack to DocGroup.

Comment 39

[John Dai[:jdai]]

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

Comment 40

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 1: Move custom element reactions stack to DocGroup. v6

Reorder patch due to code dependency.

Comment 41

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 2: Implement the support for CEReactions in WebIDL parser. v6

Comment 42

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 3: Implement the support for CEReactions in Codegen. v6

Comment 43

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 4: Add CEReactions for CustomElementRegistry. v6

Comment 44

[John Dai[:jdai]]

Attachment: The resulting codegen of CustomElementRegistry (CustomElementRegistryBinding.cpp)

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844412 [details] [diff] [review]
Bug 1309147 - Part 2: Implement the support for CEReactions in WebIDL parser. v6

>+               "Should have thrown for [CEReactions] used on a getter")

That one _is_ a "named getter".

>+               "Should have thrown for [CEReactions] used on a creator")

And "named creator" here.  I hadn't asked you to change these, right?

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844413 [details] [diff] [review]
Bug 1309147 - Part 3: Implement the support for CEReactions in Codegen. v6

>+    obj = js::CheckedUnwrap(obj, /* stopAtWindowProxy = */ false);

Why the second arg?  I don't think you should be passing that second arg.

Is the IsWrapper() check meant to be an optimization?  CheckedUnwrap() will return its argument if the argument is not a wrapper...  But I can see how doing the IsWrapper() check like this might avoid an out-of-line call.

Have you done any before/after performance timings for something like this testcase?

  var div = document.createElement("div");
  document.body.appendChild(div);
  var count = 1000000;
  var start = performance.now();
  for (var i = 0; i < count; ++i) {
    div.setAttribute("foo", "bar");
  }
  var end = performance.now();
  document.writeln((end - start)/count * 1e6);

>+            if ((hasattr(idlNode, 'isSetter') and idlNode.isSetter()) or
>+                (hasattr(idlNode, 'isDeleter') and idlNode.isDeleter())):

I don't really like this action-at-a-distance stuff.  Either we should make the name of the object an argument to CGPerSignatureCall (and thread it up to the relevant proxy code), or the relevant proxy code bits should put a Handle named "obj" on the stack, or we should rename the arg in the proxy bits to "obj".

Making the name an argument to CGPerSignatureCall makes the most sense to me, I think, as a separate changeset under this one.

>+            elif (idlNode.isAttr() and setter) or idlNode.isMethod():

That seems like a complicated way of saying "if not getter:", no?

I'd like to see what your codegen for the new bits on TestInterface and for TestCEReactionsInterface looks like.

Comment 47

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #46)
> Comment on attachment 8844413 [details] [diff] [review]
> Bug 1309147 - Part 3: Implement the support for CEReactions in Codegen. v6
> 
> >+    obj = js::CheckedUnwrap(obj, /* stopAtWindowProxy = */ false);
> 
> Why the second arg?  I don't think you should be passing that second arg.
> 
Will do.
> Is the IsWrapper() check meant to be an optimization?  CheckedUnwrap() will
> return its argument if the argument is not a wrapper...  But I can see how
> doing the IsWrapper() check like this might avoid an out-of-line call.
> 
> Have you done any before/after performance timings for something like this
> testcase?
> 
After I ran before/after performance timings test, I should remove IsWrapper() check. 

>   var div = document.createElement("div");
>   document.body.appendChild(div);
>   var count = 1000000;
>   var start = performance.now();
>   for (var i = 0; i < count; ++i) {
>     div.setAttribute("foo", "bar");
>   }
>   var end = performance.now();
>   document.writeln((end - start)/count * 1e6);
> 
> >+            if ((hasattr(idlNode, 'isSetter') and idlNode.isSetter()) or
> >+                (hasattr(idlNode, 'isDeleter') and idlNode.isDeleter())):
> 
> I don't really like this action-at-a-distance stuff.  Either we should make
> the name of the object an argument to CGPerSignatureCall (and thread it up
> to the relevant proxy code), or the relevant proxy code bits should put a
> Handle named "obj" on the stack, or we should rename the arg in the proxy
> bits to "obj".
> 
> Making the name an argument to CGPerSignatureCall makes the most sense to
> me, I think, as a separate changeset under this one.
> 
Will do.
> >+            elif (idlNode.isAttr() and setter) or idlNode.isMethod():
> 
> That seems like a complicated way of saying "if not getter:", no?
> 
Yes, there are the same thing, I'll change to "if not getter:". Thank you.
> I'd like to see what your codegen for the new bits on TestInterface and for
> TestCEReactionsInterface looks like.
Will upload codegen binding file. Thank you.

Comment 48

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #47)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #46)
> > Comment on attachment 8844413 [details] [diff] [review]
> > Bug 1309147 - Part 3: Implement the support for CEReactions in Codegen. v6
> > 
> > >+    obj = js::CheckedUnwrap(obj, /* stopAtWindowProxy = */ false);
> > 
> > Why the second arg?  I don't think you should be passing that second arg.
> > 
> Will do.
> > Is the IsWrapper() check meant to be an optimization?  CheckedUnwrap() will
> > return its argument if the argument is not a wrapper...  But I can see how
> > doing the IsWrapper() check like this might avoid an out-of-line call.
> > 
> > Have you done any before/after performance timings for something like this
> > testcase?
> > 
> After I ran before/after performance timings test, I should remove
> IsWrapper() check. 
> 
I ran two cases, one is optimized build, another is debug build. Both cases without IsWrapper() check faster than with IsWrapper() check.

Comment 49

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 2: Implement the support for CEReactions in WebIDL parser. v7

Addressed comment #45.

Comment 50

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 3: Make the parameter name an argument to CGPerSignatureCall. v1

Addressed comment #46.

Comment 51

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 4: Implement the support for CEReactions in Codegen. v7

Addressed comment #46.

Comment 52

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 5: Add CEReactions for CustomElementRegistry. v6

Rename patch name.

Comment 53

[John Dai[:jdai]]

Attachment: The resulting codegen of TestInterface and TestCEReactionsInterface (TestCodeGenBinding.cpp)

Comment 54

[Boris Zbarsky [:bzbarsky]]

> I ran two cases, one is optimized build, another is debug build. 

There's no point testing debug for performance.

> Both cases without IsWrapper() check faster than with IsWrapper() check.

What are the actual numbers?  What are the numbers without these patches at all?

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844866 [details] [diff] [review]
Bug 1309147 - Part 3: Make the parameter name an argument to CGPerSignatureCall. v1

"the parameter name" isn't right.  What you're adding is the name of the "this" value's JSObject*.

I think you should just default it to "obj" and pass "proxy" in the one spot that needs it.

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844871 [details] [diff] [review]
Bug 1309147 - Part 4: Implement the support for CEReactions in Codegen. v7

OK, one more question.  Given the new setup, why do you need a Rooted?  That is, why can you not do:

  JSObject* obj = js::CheckedUnwrap(aObj);

and then the rest of the code?  Should be a bit faster...

Comment 57

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844871 [details] [diff] [review]
Bug 1309147 - Part 4: Implement the support for CEReactions in Codegen. v7

>+        if (idlNode.getExtendedAttribute('CEReactions') is not None and
>+            ((hasattr(idlNode, 'isSetter') and idlNode.isSetter()) or
>+            (hasattr(idlNode, 'isDeleter') and idlNode.isDeleter()) or
>+            not getter or idlNode.isMethod())):

This is a very complicated way of writing:

  if (idlNode.getExtendedAttribute('CEReactions') is not None and 
      not getter):

no?  Why do you need all the other bits?

>+                ceReaction.emplace(reactionsStack);

Fix the indent, please.

I think if you take out the Rooted you don't need to pass a JSContext to GetCustomElementReactionsStack either.


>+CustomElementReactionsStack*
>+GetCustomElementReactionsStack(JSContext* aCx, JS::Handle<JSObject*> aObj);

Needs documentation explaining what this actually does.  Specifically, which docgroup it returns the stack for, and that it can return null, etc.

Comment 58

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #54)
> > I ran two cases, one is optimized build, another is debug build. 
> 
> There's no point testing debug for performance.
> 
> > Both cases without IsWrapper() check faster than with IsWrapper() check.
> 
> What are the actual numbers?  What are the numbers without these patches at
> all?

Here are the actual numbers which are 10 times average:
without these patches at all:
430.951

with js::IsWrapper:
657.106

without js::IsWrapper:
596.969

without JS::Rooted use JSObject* obj = js::CheckedUnwrap(aObj); instead:
561.04

Comment 59

[Boris Zbarsky [:bzbarsky]]

That's a pretty significant regression.  :(  Also, those times are about 2x to 3x slower than I would have expected for recentish laptop-level hardware for the "without these patches" number.  These are numbers from an opt build?  On what sort of hardware?

What happens if you just comment out the CheckedUnwrap bits and set obj = aObj?  Trying to figure out what the costs are here...

For example, it might make sense to simply define that any "this" value for a [CEReactions] thing must implement some method on it to get the custom element registry, or the docgroup, or something.  And then for nodes we could just implement it via OwnerDoc().  It might be faster than messing around with JS bits.

Comment 60

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #59)
> That's a pretty significant regression.  :(  Also, those times are about 2x
> to 3x slower than I would have expected for recentish laptop-level hardware
> for the "without these patches" number.  These are numbers from an opt
> build?  On what sort of hardware?
> 
Yes. These numbers are opt build, and it's my desktop machine which is Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz.

> What happens if you just comment out the CheckedUnwrap bits and set obj =
> aObj?  Trying to figure out what the costs are here...
> 
It took 552.0315ms, 1% improvement[1]. I guess performance issue not in here. I found the issue is in the CreateAndPushElementQueue() and PopAndInvokeElementQueue(). If I comment out those functions, it took 436.441ms with 22.2% improvement[1], and it costs 0.1% compare to without these patches at all. I found that there are some places I can improve in my code:
1) Change nsTArray to AutoTArray, it took 494.109ms with 12.5% improvement[1].
2) Reduce function call. In PopAndInvokeElementQueue(), check elementQueue size at first in order to reduce call InvokeReactions().[2] It took 458.529ms with 18.3% improvement[1], and it costs 6% compare to without these patches at all. 


[1] Those numbers are compare to 561.04ms which is without JS::Rooted use JSObject* obj = js::CheckedUnwrap(aObj); instead.
[2] https://searchfox.org/mozilla-central/source/dom/base/CustomElementRegistry.cpp#909

> For example, it might make sense to simply define that any "this" value for
> a [CEReactions] thing must implement some method on it to get the custom
> element registry, or the docgroup, or something.  And then for nodes we
> could just implement it via OwnerDoc().  It might be faster than messing
> around with JS bits.

Comment 61

[Boris Zbarsky [:bzbarsky]]

> These numbers are opt build, and it's my desktop machine which is Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz.

Using the exact testcase from comment 46?  That seems _really_ slow.  I don't understand why it's so slow.  It shouldn't be!

What numbers do you see in a Firefox release or nightly?

> I found the issue is in the CreateAndPushElementQueue() and PopAndInvokeElementQueue().

Oh, right, we do actual work with this registry once we get it...  ;)

What do the numbers look like if you _don't_ make the document.body.appendChild(div) call?

Comment 62

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #61)
> > These numbers are opt build, and it's my desktop machine which is Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz.
> 
> Using the exact testcase from comment 46?  That seems _really_ slow.  I
> don't understand why it's so slow.  It shouldn't be!
> 
Yes. I use the exact testcase from comment 46.
> What numbers do you see in a Firefox release or nightly?
> 
It's 55.0a1 nightly.

> > I found the issue is in the CreateAndPushElementQueue() and PopAndInvokeElementQueue().
> 
> Oh, right, we do actual work with this registry once we get it...  ;)
> 
> What do the numbers look like if you _don't_ make the
> document.body.appendChild(div) call?

Based on comment #60 patches, I commented out document.body.appendChild(div), the number is 405.1035ms.

Comment 63

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #62)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no
> decent message, automatic r-) from comment #61)
> > > These numbers are opt build, and it's my desktop machine which is Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz.
> > 
> > Using the exact testcase from comment 46?  That seems _really_ slow.  I
> > don't understand why it's so slow.  It shouldn't be!
> > 
> Yes. I use the exact testcase from comment 46.
> > What numbers do you see in a Firefox release or nightly?
> > 
> It's 55.0a1 nightly.
In firefox release 52, the number is 265.869ms. In firefox nightly 55.0a1, the number is 283.3105ms.
> 
> > > I found the issue is in the CreateAndPushElementQueue() and PopAndInvokeElementQueue().
> > 
> > Oh, right, we do actual work with this registry once we get it...  ;)
> > 
> > What do the numbers look like if you _don't_ make the
> > document.body.appendChild(div) call?
> 
> Based on comment #60 patches, I commented out
> document.body.appendChild(div), the number is 405.1035ms.

Comment 64

[Boris Zbarsky [:bzbarsky]]

> It's 55.0a1 nightly.

But in comment 63 you have much lower numbers, more in line with what I expect...

Comment 65

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #64)
> > It's 55.0a1 nightly.
> 
> But in comment 63 you have much lower numbers, more in line with what I
> expect...

Yes, I'm going to change my local revision to firefox nightly 55.0a1 and find the difference.

Comment 66

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #65)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no
> decent message, automatic r-) from comment #64)
> > > It's 55.0a1 nightly.
> > 
> > But in comment 63 you have much lower numbers, more in line with what I
> > expect...
> 
> Yes, I'm going to change my local revision to firefox nightly 55.0a1 and
> find the difference.

After I reviewed my testcase again, I found it was my stupid error, I put "class SomeCustomElement extends HTMLElement {};" in the first line of the testcase. That is the reason why it slowed down 100ms. After I removed that line, it took 259.631ms. It took 217.61ms for without patch testcase. Firefox nightly 55.01a1 took 119.3345ms. Firefox 52 took 197.996ms.

Comment 67

[Boris Zbarsky [:bzbarsky]]

> I put "class SomeCustomElement extends HTMLElement {};" in the first line of the testcase.
> That is the reason why it slowed down 100ms.

That's a ... pretty stiff performance cliff, given that the <div> has nothing to do with that class declaration, and that there is no custom element registration stuff involved, right.  Why is that performance cliff there?

> it took 259.631ms. It took 217.61ms for without patch testcase. Firefox nightly 55.01a1 took 119.3345ms.
> Firefox 52 took 197.996ms.

So if I have this straight, a nightly is at 119ms but you own build, with no patches, from current tip source is 217ms?  If correct, that's pretty suspicious...  What OS is this on?  If Windows, that _might_ be accounted for by PGO, I suppose...

Comment 68

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #33)
> > is to make sure that we will synchronize doing the following steps.
> 
> Yes, I agree.  So per spec, the thing maintained by CEReactions is a stack
> of element queues; each element queue contains elements.
> 
> > to the right custom element reactions stack in the correct global object
> 
> The spec makes no reference to specific global objects here.  There is a
> single custom element reactions stack per unit of related similar-origin
> browsing contexts.  In Gecko terms, that's per DocGroup, right?  Why are we
> storing these stacks per-global, and is the difference in behavior
> observable?

If you don't mind, I would like to move this part to bug 1347446. This bug mainly focus on [CEReactions] things. I'll update those changes in next patch.

Comment 69

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #67)
> > I put "class SomeCustomElement extends HTMLElement {};" in the first line of the testcase.
> > That is the reason why it slowed down 100ms.
> 
> That's a ... pretty stiff performance cliff, given that the <div> has
> nothing to do with that class declaration, and that there is no custom
> element registration stuff involved, right.  Why is that performance cliff
> there?
> 

Even the testcase only add "class x {};" in javascript code and do nothing, it'll increase twice execute time. I can upload some testcase and test result for JS engine expert.

> > it took 259.631ms. It took 217.61ms for without patch testcase. Firefox nightly 55.01a1 took 119.3345ms.
> > Firefox 52 took 197.996ms.
> 
> So if I have this straight, a nightly is at 119ms but you own build, with no
> patches, from current tip source is 217ms?  If correct, that's pretty
> suspicious...  What OS is this on?  If Windows, that _might_ be accounted
> for by PGO, I suppose...

I use Linux 16.04. Your guess is right, it's about PGO. Thank you for giving me this hint. I downloaded PGO build from try server[1][2], it took 122.0195ms which is included my patches, so it increased 3ms compare to Firefox nightly 55.01a1 which is without these patches. 

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d530ca9f3e2b8eb48d8fd4289d4f257425312fc9
[2] https://queue.taskcluster.net/v1/task/RZSvnK9WQ5iOhKvQ-3G0-Q/runs/0/artifacts/public/build/target.tar.bz2

Comment 70

[John Dai[:jdai]]

Attachment: test_slow_class.html

This test adds performance.now() in every statement in order to find where is performance cliff. Also, it prints execution time every 10000 time.

Comment 71

[John Dai[:jdai]]

Attachment: test_result_with_class.txt

Comment 72

[John Dai[:jdai]]

Attachment: test_result_without_class.txt

Comment 73

[Boris Zbarsky [:bzbarsky]]

> If you don't mind, I would like to move this part to bug 1347446.

I don't mind at all.  Spinning that off into a separate bug makes a lot of sense.

> Even the testcase only add "class x {};" in javascript code and do nothing, it'll increase twice execute
> time.

Oh.  Please file a separate bug on this and cc me?  I would guess class decls inhibit ion compilation or something, and our code is in the same compilation unit as the class decl.

For now, we can test with that line removed or all the code we're actually testing wrapped up in a function.

> it took 122.0195ms which is included my patches, so it increased 3ms compare to Firefox nightly
> 55.01a1 which is without these patches.

Alright, that's more like it.  That seems like an acceptable hit.  ;)

Comment 74

[Boris Zbarsky [:bzbarsky]]

> Please file a separate bug on this and cc me? 

Actually, no need.  Bug 1167472 covers this.  Apparently no baseline support either, not just no ion support!

Comment 75

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL parser. v8

Address comment #45.

Comment 76

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 2: Add the name of 'this' value's JSObject* for codegen to generate CEReaction code. v8

Address comment #46.

Comment 77

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 3: Implement the support for CEReactions in Codegen. v8

Address comment #57.

Comment 78

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 4: Add CEReactions for CustomElementRegistry. v8

Comment 79

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 5: Eliminate performance cliff when access CEReactions code. v8

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

Comment 80

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 5: Eliminate performance cliff when access CEReactions code. v9

- Revised AutoTArray sizes in order to avoid wasting space.
- Updated new try result since previous one was broken.

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

Hi bz, could you help to review these patches? Thank you.

Comment 81

[Boris Zbarsky [:bzbarsky]]

Sure.  Just request review on the ones you want me to review.

Comment 82

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8854743 [details] [diff] [review]
Bug 1309147 - Part 5: Eliminate performance cliff when access CEReactions code. v9

>-  aElementQueue.Clear();

Why is that removal OK?

Comment 83

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 5: Eliminate performance cliff when access CEReactions code. v10

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #82)
> Comment on attachment 8854743 [details] [diff] [review]
> Bug 1309147 - Part 5: Eliminate performance cliff when access CEReactions
> code. v9
> 
> >-  aElementQueue.Clear();
> 
> Why is that removal OK?

Oh. It's my bad. I was accidentally modified this line. Updated a new patch without this removal.

Comment 84

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8852845 [details] [diff] [review]
Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL parser. v8

>+                raise WebIDLError("[CEReactions] only allowed on "
>+                                  "readonly attributes",

That error message is backwards, no?  Please fix that.

Did anything else in here change from the last version I reviewed?  If not, r=me.

Comment 85

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8852846 [details] [diff] [review]
Bug 1309147 - Part 2: Add the name of 'this' value's JSObject* for codegen to generate CEReaction code. v8

r=me

Comment 86

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8852848 [details] [diff] [review]
Bug 1309147 - Part 3: Implement the support for CEReactions in Codegen. v8

>+  if (!docGroup) {

Can that actually be null?  Paging mystor for this.

>Get CustomElementReactionsStack from DocGroup which is from
>+// inner window's document.

How about:

  Get the CustomElementReactionsStack for the docgroup of the global of the underlying object of aObj.

> This can be null if inner window or
> +// DocGroup or js::CheckedUnwrap is null.

  This can be null if aObj can't be CheckUnwrapped, or if the global of the result has no docgroup (e.g. because it's not a Window global).

r=me modulo that null-check issue.

Comment 87

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8852849 [details] [diff] [review]
Bug 1309147 - Part 4: Add CEReactions for CustomElementRegistry. v8

r=me

Comment 88

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8855189 [details] [diff] [review]
Bug 1309147 - Part 5: Eliminate performance cliff when access CEReactions code. v10

This commit needs a better commit message.  I'm not sure what it's doing and why it's going about it the way it is.

> CustomElementReactionsStack::PopAndInvokeElementQueue()
>+  if (!elementQueue.IsEmpty()) {

Why does this help?  Just by avoiding the extra function call?  I don't see what other work it would avoid...

>+  typedef AutoTArray<nsWeakPtr, 1> ElementQueue;

Why is "1" the right number?  Needs documentation, at least.

>+  typedef AutoTArray<nsAutoPtr<CustomElementReaction>, 4> ReactionQueue;

Why is "4" the right number?

>+  AutoTArray<ElementQueue, 8> mReactionsStack;

Why is "8" the right number?

Comment 89

[Nika Layzell [:nika] (ni? for response)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #86)
> Comment on attachment 8852848 [details] [diff] [review]
> Bug 1309147 - Part 3: Implement the support for CEReactions in Codegen. v8
> 
> >+  if (!docGroup) {
> 
> Can that actually be null?  Paging mystor for this.

Yes, with the TabGroup and DocGroup methods I was fairly consistent IIRC with naming the functions `GetFoo` if they could return nullptr, and `Foo()` if they could not. So for example `nsPIDOMWindow::TabGroup()` is infallible and will always produce a TabGroup, while `nsPIDOMWindow::GetDocGroup()` could fail if there is no document attached to the window.

Comment 90

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL parser. v9

- Revised error message grammar.
- Carried r+.

> >+                raise WebIDLError("[CEReactions] only allowed on "
> >+                                  "readonly attributes",
> 
> That error message is backwards, no?  Please fix that.
> 
> Did anything else in here change from the last version I reviewed?  If not,
> r=me.

The error message has been changed from the last version is because our spec[1] changed as well.

[1] https://github.com/whatwg/html/issues/2362

Comment 91

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 3: Implement the support for CEReactions in Codegen. v9

- Address comment #86, which revised GetCustomElementReactionsStack documentation.
- Carry r+.

Comment 92

[Boris Zbarsky [:bzbarsky]]

>- Revised error message grammar.

The problem wasn't the grammar.  The problem is it's _backwards_.  [CEReactions] is NOT allowed on readonly attributes.

Comment 93

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 1: Implement the support for CEReactions in WebIDL parser. v10

- Revised error message is backwards.

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #92)
> >- Revised error message grammar.
> 
> The problem wasn't the grammar.  The problem is it's _backwards_. 
> [CEReactions] is NOT allowed on readonly attributes.

Sorry about that, I wasn't understand the backwards meaning. Update new patch to address them.

Comment 94

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #88)
> Comment on attachment 8855189 [details] [diff] [review]
> Bug 1309147 - Part 5: Eliminate performance cliff when access CEReactions
> code. v10
> 
> This commit needs a better commit message.  I'm not sure what it's doing and
> why it's going about it the way it is.
> 
> > CustomElementReactionsStack::PopAndInvokeElementQueue()
> >+  if (!elementQueue.IsEmpty()) {
> 
> Why does this help?  Just by avoiding the extra function call?  I don't see
> what other work it would avoid...
> 

It does some help. The reason I added this check is because not every [CEReactions] push element into ElementQueue. If we can determine it at early stage, it can improve performance. It's around 6% performance improve in non-pgo build (see comment #60).

For more detail, there are only two functions doing push element into ElementQueue:
1) Enqueue a custom element upgrade reaction
2) Enqueue a custom element callback reaction

For 1), there are two functions will trigger enqueue upgrade reaction:
- CustomElements.define(name, constructor) step 15.[1]
- Try to upgrade an element [2]

For 2), there are two functions will trigger enqueue callback reaction:
- Upgrade an element[3]
- Creating and inserting nodes[4] for append portion.

[1] https://html.spec.whatwg.org/#enqueue-a-custom-element-upgrade-reaction
[2] https://html.spec.whatwg.org/#concept-try-upgrade
[3] https://html.spec.whatwg.org/#concept-upgrade-an-element
[4] https://html.spec.whatwg.org/#create-an-element-for-the-token

> >+  typedef AutoTArray<nsWeakPtr, 1> ElementQueue;
> 
> Why is "1" the right number?  Needs documentation, at least.
> 
> >+  typedef AutoTArray<nsAutoPtr<CustomElementReaction>, 4> ReactionQueue;
> 
> Why is "4" the right number?
> 
> >+  AutoTArray<ElementQueue, 8> mReactionsStack;
> 
> Why is "8" the right number?

I'll address them in next patch. Those numbers was decided by spec reading.
I need to do more performance testing in order to find the right numbers.

Comment 95

[Boris Zbarsky [:bzbarsky]]

> It does some help.

OK, so just avoiding the function call overhead.  That could use a comment so someone doesn't helpfully "fix" it.

> I need to do more performance testing in order to find the right numbers.

Again, the key is to have a comment explaining where the number comes from.  Even if it's just guessing, documenting that is important so people know how safe it is to change the number.

Comment 96

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 5: Eliminate performance cliff when access CEReactions code. v11

- Address comment 95, which is adding more comments in unclear areas.

Comment 97

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8856488 [details] [diff] [review]
Bug 1309147 - Part 5: Eliminate performance cliff when access CEReactions code. v11

>+  // The number choice for 1 is because in most of case element queue only has
>+  // one element.

OK, but does it ever have more than 1?  In what cases?  Why are those cases rare?  Is there a cost to doing 2 or 4 or 4096 here?  What is that cost?  

Where do these ElementQueue live?  For how long?

>+  // The number choice for 4 is because when upgrade an element, it'll put
>+  // upgraded, attributeChangedCallback and connectedCallback.

I don't understand this comment....

Is the point that when upgrading a single element it can have up to three things it will put in the ReactionQueue?  What happens when we upgrade multiple elements?

The ReactionQueue objects are only used as values in the ElementReactionQueueMap, so the tradeoff here is memory usage of that map, right?  How long do the entries in this map live?

>+  // The number choice for 8 is based on gut feeling.

  "The choice of 8 for the auto size here is based on gut feeling."

Comment 98

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #97)
> Comment on attachment 8856488 [details] [diff] [review]
> Bug 1309147 - Part 5: Eliminate performance cliff when access CEReactions
> code. v11
> 
> >+  // The number choice for 1 is because in most of case element queue only has
> >+  // one element.
> 
> OK, but does it ever have more than 1?  In what cases?  Why are those cases
> rare?  Is there a cost to doing 2 or 4 or 4096 here?  What is that cost?  
> 
If there are multiple custom element nodes in document fragment, append them to dom tree, it'll trigger connected callback in multiple custom element nodes. However, I don't have testing data to measure cost and support my hypothesis. IMHO, it's better to revise comment message to 'gut feeling' in this moment. I prefer defer performance measurement until we implement upgrade and callback v1 spec, because they are implemented by v0 spec now and v0 spec doesn't have connected callback.

> Where do these ElementQueue live?  For how long?
ElementQueue lifecycles within [CEReactions] operation[1] in most of cases. Cloning, and range manipulation are exception, they'll queue a microtask and delay until after all the relevant user agent processing steps have completed, and then run together as a batch. I don't have accurate testing numbers to answer how long. 


[1] https://html.spec.whatwg.org/multipage/scripting.html#cereactions 
> 
> >+  // The number choice for 4 is because when upgrade an element, it'll put
> >+  // upgraded, attributeChangedCallback and connectedCallback.
> 
> I don't understand this comment....
> 
> Is the point that when upgrading a single element it can have up to three
> things it will put in the ReactionQueue?  What happens when we upgrade
> multiple elements?
> 
Yes, if we upgrade a single element, it'll put three things in the ReactionQueue. If there are multiple elements, each element has its own ReactionQueue, it'll put into their ReactionQueue. Same as above, no accurate tested data to support it.

> The ReactionQueue objects are only used as values in the
> ElementReactionQueueMap, so the tradeoff here is memory usage of that map,
> right?  How long do the entries in this map live?
> 
> >+  // The number choice for 8 is based on gut feeling.
> 
>   "The choice of 8 for the auto size here is based on gut feeling."
Will do.

Comment 99

[Boris Zbarsky [:bzbarsky]]

> IMHO, it's better to revise comment message to 'gut feeling' in this moment.

OK.

> I prefer defer performance measurement until we implement upgrade and callback v1 spec

That's fine.  Please make sure there's a bug tracking it.

> I don't have accurate testing numbers to answer how long. 

OK.  I mean, the real question is what is the cost of making this array larger?

> Yes, if we upgrade a single element, it'll put three things in the ReactionQueue.
> If there are multiple elements, each element has its own ReactionQueue, it'll put
> into their ReactionQueue.

OK.  Can we end up with _more_ than 3 things in the ReactionQueue?  Fewer things?  If not, then the comment should probably say something like:

  // When we upgrade an element it will place exactly 3 things in a ReactionQueue:
  // "upgraded", "attributeChanged", and "connected" callbacks.

or so, and make the auto size 3.  If the size can be something other than 3, I'd like to understand what it can be.

Comment 100

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8856488 [details] [diff] [review]
Bug 1309147 - Part 5: Eliminate performance cliff when access CEReactions code. v11

r=me, but I'd still like answers to comment 99 before this lands.

Comment 101

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #99)
> > IMHO, it's better to revise comment message to 'gut feeling' in this moment.
> 
> OK.
> 
> > I prefer defer performance measurement until we implement upgrade and callback v1 spec
> 
> That's fine.  Please make sure there's a bug tracking it.
Filed bug 1356761.

> 
> > I don't have accurate testing numbers to answer how long. 
> 
> OK.  I mean, the real question is what is the cost of making this array
> larger?
> 
I'll answer this question when I'm going to fix bug 1356761.

> > Yes, if we upgrade a single element, it'll put three things in the ReactionQueue.
> > If there are multiple elements, each element has its own ReactionQueue, it'll put
> > into their ReactionQueue.
> 
> OK.  Can we end up with _more_ than 3 things in the ReactionQueue?  Fewer
> things?  If not, then the comment should probably say something like:
> 
>   // When we upgrade an element it will place exactly 3 things in a
> ReactionQueue:
>   // "upgraded", "attributeChanged", and "connected" callbacks.
> 
> or so, and make the auto size 3.  If the size can be something other than 3,
> I'd like to understand what it can be.
If I read the spec correctly, the max reactions put into queue is 3 and it's possible to have fewer reactions in ReactionQueue. 
- There is 1 reaction in reaction queue, when 1) it becomes disconnected. 2) it’s adopted into a new document. 3) Its attributes are changed, appended, removed, or replaced. [1]
- There is 3 reactions in reaction queue when doing upgrade operation, e.g., create an element[2], insert a node[3]. 

[1] https://html.spec.whatwg.org/multipage/scripting.html#custom-element-reactions
[2] https://dom.spec.whatwg.org/#concept-create-element  
[3] https://dom.spec.whatwg.org/#concept-node-insert

Comment 102

[Boris Zbarsky [:bzbarsky]]

> the max reactions put into queue is 3

OK.  In that case, making this an auto array with size 3 and a code comment explaining exactly what your bug comment there says makes the most sense to me.

Comment 103

[John Dai[:jdai]]

Attachment: Bug 1309147 - Part 5: Eliminate performance cliff when access CEReactions code. v12

Addressed comment 101. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19ea89641b04c6d69464976f054d0a3b590ed4a1&filter-tier=1&group_state=expanded&selectedJob=92249500

Comment 104

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/470180d17b73
Part 1: Implement the support for CEReactions in WebIDL parser. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe0ed97a8238
Part 2: Add the name of 'this' value's JSObject* for codegen to generate CEReaction code. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/c41ec20ea0d4
Part 3: Implement the support for CEReactions in Codegen. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/782add02e863
Part 4: Add CEReactions for CustomElementRegistry. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/44af0a9b467c
Part 5: Eliminate performance cliff when accessing CEReactions code. r=bz

Comment 105

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/470180d17b73
https://hg.mozilla.org/mozilla-central/rev/fe0ed97a8238
https://hg.mozilla.org/mozilla-central/rev/c41ec20ea0d4
https://hg.mozilla.org/mozilla-central/rev/782add02e863
https://hg.mozilla.org/mozilla-central/rev/44af0a9b467c