Closed Bug 1309184 Opened 8 years ago Closed 7 years ago

Implement upgrade reaction for custom element reactions

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jdai, Assigned: jdai)

References

Details

(Whiteboard: dom-ce-m2)

Attachments

(1 file, 9 obsolete files)

According to spec [1][2], we need to implement upgrade reaction and callback reaction for custom element reactions. Also, enqueue a custom element callback/upgrade reaction algorithm and invoke custom element reactions algorithm.


[1] https://html.spec.whatwg.org/multipage/scripting.html#upgrade-reaction
[2] https://html.spec.whatwg.org/multipage/scripting.html#callback-reaction
Whiteboard: dom-ce-m2
This bug will implement upgrade reaction for custom element reactions. I filed bug 1315885 for callback reaction.
Summary: Implement upgrade reaction and callback reaction for custom element reactions → Implement upgrade reaction for custom element reactions
No longer blocks: 1309176
Attachment #8808953 - Flags: feedback?(echen)
Comment on attachment 8808953 [details] [diff] [review]
Part 1:  Add stack-of-queues system for custom element reaction, v1

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

I think this part could just be merged with next part.

::: dom/base/CustomElementRegistry.h
@@ +127,5 @@
>  
>    // The document custom element order.
>    uint32_t mDocOrder;
> +
> +class CustomElementReaction

Please add some documentation, it helps reviewer to understand the implementation. Same for others.

@@ +133,5 @@
> +public:
> +  CustomElementReaction(CustomElementDefinition* aDefinition);
> +  virtual ~CustomElementReaction() = default;
> +  virtual void Invoke(Element* aElement) = 0;
> +protected:

Nit: blank line before this.

@@ +137,5 @@
> +protected:
> +  CustomElementDefinition* mDefinition;
> +};
> +
> +class CustomElementStack

I would call this CustomeElementReactionStack.

@@ +144,5 @@
> +  void InvokeReactions(nsTArray<nsCOMPtr<Element>>& aElementQueue);
> +  void InvokeBackupQueue();
> +  void EnqueueUpgradeReaction(Element* aElement,
> +                              CustomElementDefinition* aDefinition);
> +  NS_INLINE_DECL_REFCOUNTING(CustomElementStack)

Move upward, put it right after "public:" and add a blank line after it.

@@ +145,5 @@
> +  void InvokeBackupQueue();
> +  void EnqueueUpgradeReaction(Element* aElement,
> +                              CustomElementDefinition* aDefinition);
> +  NS_INLINE_DECL_REFCOUNTING(CustomElementStack)
> +private:

Nit: blank line before this.

@@ +152,5 @@
> +
> +  typedef nsClassHashtable<nsISupportsHashKey, nsTArray<nsAutoPtr<CustomElementReaction>>>
> +    ElementReactionQueueMap;
> +
> +  ElementReactionQueueMap mElementReactionQueueMap;

Please have documentation saying what this data structure used for. Same for others.

@@ +155,5 @@
> +
> +  ElementReactionQueueMap mElementReactionQueueMap;
> +
> +  nsTArray<nsTArray<nsCOMPtr<Element>>> mReactionStack;
> +  nsTArray<nsCOMPtr<Element>> mBackupQueue;

Is nsTArray<nsCOMPtr<Element>> the data structure of "Element Queue"? I think it worth to define a type for it. Maybe "typedef nsTArray<nsCOMPtr<Element>> ElementQueue" ..
Attachment #8808953 - Flags: feedback?(echen)
Comment on attachment 8808954 [details] [diff] [review]
Part 2: Implement upgrade reaction for custom element reactions, v1

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

Apart from implementing data structure, I think we should also do upgrade step around reaction stuff. Of cause, the upgrade step here will still be what we have now (replacing prototype only), the new step (construction stack stuff ...) will be implemented in bug 1299363.

Basically, there are two cases,
* Definition comes first, so upgrade reaction will be queued during element creation time, see step 6.2 of https://html.spec.whatwg.org/multipage/scripting.html#custom-element-reactions:concept-upgrade-an-element. And currently we do upgrade synchronously, see https://dxr.mozilla.org/mozilla-central/rev/783356f1476eafd8e4d6fa5f3919cf6167e84f8d/dom/html/nsHTMLContentSink.cpp#265. We should do upgrade around upgrade reaction stuff.

* Definition comes after, so upgrade reaction will be queued during definition time, see step 15 of https://html.spec.whatwg.org/multipage/scripting.html#custom-element-reactions:concept-upgrade-an-element. And currently we do upgrade synchronously, too, see https://dxr.mozilla.org/mozilla-central/rev/783356f1476eafd8e4d6fa5f3919cf6167e84f8d/dom/base/CustomElementRegistry.cpp#783.

::: dom/base/CustomElementRegistry.cpp
@@ +241,5 @@
>      // Add the base queue sentinel to the processing stack.
>      sProcessingStack->AppendElement((CustomElementData*) nullptr);
>    }
> +
> +  mCustomElementStack = new CustomElementStack();

Leak: the memory allocated here is never released.

@@ +503,5 @@
>            continue;
>          }
>        }
>  
> +      mCustomElementStack->EnqueueUpgradeReaction(elem, aDefinition);

Why replacing EnqueueLifecycleCallback? I guess it is an oversight.

@@ +871,5 @@
> +  mDefinition = aDefinition;
> +}
> +
> +void
> +CustomElementStack::EnqueueUpgradeReaction(Element* aElement,

Okay, so right now we have multiple classes method in same file. Please group them and add separator.

e.g.

//-----------------------------------------------------
// Class1

Class1::Method1()
{
  ...
}

Class1::Method2()
{
  ...
}

//-----------------------------------------------------
// Class2

Class2:: Method1()
{
  ...
}

Class2::Method2()
{
  ...
}

...

@@ +903,5 @@
> +  if (mIsBackupQueueProcessing) {
> +    return;
> +  }
> +
> +  mIsBackupQueueProcessing = true;

Make a small RAII class to handle setting/unsetting mIsBackupQueueProcessing flag.

@@ +905,5 @@
> +  }
> +
> +  mIsBackupQueueProcessing = true;
> +
> +  nsContentUtils::AddScriptRunner(

Hmm, spec says queuing a MicroTask, probably we could use DispatchToMicroTask() [1]. Or if you use ScriptRunner on purpose, please add some explanation.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1178665#c12

::: dom/base/CustomElementRegistry.h
@@ +126,5 @@
>    // TODO: Bug 1287348 - Implement construction stack for upgrading an element
>  
>    // The document custom element order.
>    uint32_t mDocOrder;
> +  void Upgrade(Element* aElement);

Having a Upgrade() method in CustomElementDefinition seems weird to me.
I would let Updrade() take additional argument for definition and put it in other place.
Attachment #8808954 - Flags: feedback?(echen)
(In reply to Edgar Chen [:edgar][:echen] from comment #6)
> Comment on attachment 8808954 [details] [diff] [review]
> 
> * Definition comes first, so upgrade reaction will be queued during element
> creation time, see step 6.2 of
> https://html.spec.whatwg.org/multipage/scripting.html#custom-element-
> reactions:concept-upgrade-an-element. 
  ^^^^
  Err, this should be step 6.2 of https://dom.spec.whatwg.org/#concept-create-element

> * Definition comes after, so upgrade reaction will be queued during
> definition time, see step 15 of
> https://html.spec.whatwg.org/multipage/scripting.html#custom-element-
> reactions:concept-upgrade-an-element. 
  ^^^^
  Err, this should be step 15 of https://html.spec.whatwg.org/multipage/scripting.html#element-definition
- Addressed comment #5 and comment #6.
- Use RAII class to handle setting/unsetting mIsBackupQueueProcessing flag.
- Use DispatchToMicroTask() to queue a MicroTask.
- Integrate with customelement v0 Upgrade() method.
Attachment #8808953 - Attachment is obsolete: true
Attachment #8808954 - Attachment is obsolete: true
Attachment #8811568 - Flags: feedback?(echen)
Attachment #8811568 - Flags: feedback?(echen)
Comment on attachment 8811568 [details] [diff] [review]
Implement upgrade reaction for custom element reactions, v2

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

> * Definition comes first, so upgrade reaction will be queued during element
> creation time, see step 6.2 of https://dom.spec.whatwg.org/#concept-create-element

create-element can be synchronous or asynchronous [1]. For asynchronous case (e.g. clone a node [2]), we need to enqueue an upgrade reaction just like what we did for "definition comes after" case. Will you handle asynchronous case in this bug? If not, please file a follow-up bug for that.

[1] See "synchronous custom elements flag" in https://dom.spec.whatwg.org/#concept-create-element
[2] See step 2.1 of https://dom.spec.whatwg.org/#concept-node-clone

::: dom/base/CustomElementRegistry.cpp
@@ +241,5 @@
>      // Add the base queue sentinel to the processing stack.
>      sProcessingStack->AppendElement((CustomElementData*) nullptr);
>    }
> +
> +  mCustomElementReactionStack = new CustomElementReactionStack(this);

Use initialization list, e.g.
  CustomElementRegistry::CustomElementRegistry()
    : ....
    , mCustomElementReactionStack(new CustomElementReactionStack(this))
  {
    ...
  }

@@ +247,5 @@
>  
>  CustomElementRegistry::~CustomElementRegistry()
>  {
>    mozilla::DropJSObjects(this);
> +  mCustomElementReactionStack = nullptr;

Destructor of a RefPtr will do release, so you don't need setting to nullptr here.

@@ +480,5 @@
>        if (!elem) {
>          continue;
>        }
>  
> +      EnqueueUpgradeReaction(elem, aDefinition);

* Now we process upgrade in microtask checkpoint, but the create callback is still at script blocker time, so the create callback might be processed before upgrade. Looks like we should put EnqueueLifecycleCallback() into UpgradeReaction().

Note that we need to be care of the problem like bug 1176829. But I wonder the issue will still exist if we already EnqueueLifecycleCallback() in microtask time....

* And now the upgrade will happen asynchronously, I expect we need to update existing test cases as well.

@@ +828,5 @@
> +CustomElementRegistry::EnqueueUpgradeReaction(Element* aElement,
> +                                              CustomElementDefinition* aDefinition)
> +{
> +  mCustomElementReactionStack->Enqueue(
> +    aElement, new CustomElementUpgradeReaction(aDefinition));

I prefer creating CustomElementUpgradeReaction in CustomElementReactionStack instead which makes the life cycle of reaction clearer.

So maybe,
mCustomElementReactionStack->Enqueue(aElement, aDefinition);
And do "new CustomElementUpgradeReaction(aDefinition)" in Enqueue().

@@ +871,5 @@
> +  }
> +}
> +
> +//-----------------------------------------------------
> +// CustomElementDefinition

Nit: blank line after this, here and elsewhere.

@@ +918,5 @@
> +
> +  CycleCollectedJSContext* context = CycleCollectedJSContext::Get();
> +  RefPtr<AutoSetBackupQueueProcessing> backupQueueProcessingRunnable =
> +    new AutoSetBackupQueueProcessing(this);
> +  context->DispatchToMicroTask(do_AddRef(backupQueueProcessingRunnable));

Can we just do "backupQueueProcessingRunnable.forget()"?

@@ +935,5 @@
> +    nsCOMPtr<Element> element = aElementQueue[i];
> +    ReactionQueue* reactions = mElementReactionQueueMap.Get(element);
> +    if (!reactions->IsEmpty()) {
> +      for (uint32_t j = 0; j < reactions->Length(); ++j) {
> +        nsAutoPtr<CustomElementReaction> reaction = reactions->ElementAt(j);

The destructor of "reaction" will delete the object it held, and the same object will be deleted again when "reactions->Clear()"? It looks like a potential issue to me.

::: dom/base/CustomElementRegistry.h
@@ +158,5 @@
> +  NS_INLINE_DECL_REFCOUNTING(CustomElementReactionStack)
> +
> +  CustomElementReactionStack(RefPtr<CustomElementRegistry> aRegistry) :
> +    mCustomElementRegistry(aRegistry),
> +    mIsBackupQueueProcessing(false) { }

Nit: here and elsewhere.

CustomElementReactionStack(RefPtr<CustomElementRegistry> aRegistry)
  : mCustomElementRegistry(aRegistry)
  , mIsBackupQueueProcessing(false)
{
}

@@ +159,5 @@
> +
> +  CustomElementReactionStack(RefPtr<CustomElementRegistry> aRegistry) :
> +    mCustomElementRegistry(aRegistry),
> +    mIsBackupQueueProcessing(false) { }
> +  typedef nsTArray<nsCOMPtr<Element>> ElementQueue;

Add blank line before this.

@@ +185,5 @@
> +  RefPtr<CustomElementRegistry> mCustomElementRegistry;
> +  bool mIsBackupQueueProcessing;
> +
> +private:
> +  class MOZ_RAII AutoSetBackupQueueProcessing  : public mozilla::Runnable {

Hmm, it seems we probably shouldn't mark a MOZ_RAII and call it AutoWhatever in a Runnable class, although it update mIsBackupQueueProcessing like a RAII class ... Probably having clear documentations is enough.

@@ +307,5 @@
>    // CustomElementData in this array, separated by nullptr that
>    // represent the boundaries of the items in the stack. The first
>    // queue in the stack is the base element queue.
>    static mozilla::Maybe<nsTArray<RefPtr<CustomElementData>>> sProcessingStack;
> +  RefPtr<CustomElementReactionStack> mCustomElementReactionStack;

Add blank line around this and add some documentations.

::: dom/base/nsDocument.cpp
@@ +8141,5 @@
>  
>    nsCOMPtr<Element> element;
>    nsresult rv = NS_NewElement(getter_AddRefs(element), nodeInfo.forget(),
>                                NOT_FROM_PARSER, aIs);
> +

Remove this unnecessary change.
(In reply to Edgar Chen [:edgar][:echen] from comment #9)
> Comment on attachment 8811568 [details] [diff] [review]
> Implement upgrade reaction for custom element reactions, v2
> 
> Review of attachment 8811568 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +480,5 @@
> >        if (!elem) {
> >          continue;
> >        }
> >  
> > +      EnqueueUpgradeReaction(elem, aDefinition);
> 
> * Now we process upgrade in microtask checkpoint, but the create callback is
> still at script blocker time, so the create callback might be processed
> before upgrade. Looks like we should put EnqueueLifecycleCallback() into
> UpgradeReaction().
> 
> Note that we need to be care of the problem like bug 1176829. But I wonder
> the issue will still exist if we already EnqueueLifecycleCallback() in
> microtask time....
> 
> * And now the upgrade will happen asynchronously, I expect we need to update
> existing test cases as well.

Wrong. Upgrade will not happen in microtask checkpoint here because CustomElementRegistry.define() is annotated with the [CEReactions] extended attribute [1]. The upgrade will be executed after the define() steps.
We could implement [CEReaction] steps [2] for define() first in this bug and provide a full solution for [CEReaction] (probably codegen the steps) later in bug 1309147.

[1] https://html.spec.whatwg.org/multipage/scripting.html#dom-window-customelements
[2] https://html.spec.whatwg.org/multipage/scripting.html#cereactions
Comment on attachment 8811568 [details] [diff] [review]
Implement upgrade reaction for custom element reactions, v2

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

::: dom/base/CustomElementRegistry.cpp
@@ +480,5 @@
>        if (!elem) {
>          continue;
>        }
>  
> +      EnqueueUpgradeReaction(elem, aDefinition);

See comment #10.
(In reply to Edgar Chen [:edgar][:echen] from comment #9)
> Comment on attachment 8811568 [details] [diff] [review]
> Implement upgrade reaction for custom element reactions, v2
> 
> Review of attachment 8811568 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > * Definition comes first, so upgrade reaction will be queued during element
> > creation time, see step 6.2 of https://dom.spec.whatwg.org/#concept-create-element
> 
> create-element can be synchronous or asynchronous [1]. For asynchronous case
> (e.g. clone a node [2]), we need to enqueue an upgrade reaction just like
> what we did for "definition comes after" case. Will you handle asynchronous
> case in this bug? If not, please file a follow-up bug for that.
> 
> [1] See "synchronous custom elements flag" in
> https://dom.spec.whatwg.org/#concept-create-element
> [2] See step 2.1 of https://dom.spec.whatwg.org/#concept-node-clone
> 
I filed bug 1319342.
> ::: dom/base/CustomElementRegistry.cpp
> @@ +241,5 @@
> >      // Add the base queue sentinel to the processing stack.
> >      sProcessingStack->AppendElement((CustomElementData*) nullptr);
> >    }
> > +
> > +  mCustomElementReactionStack = new CustomElementReactionStack(this);
> 
> Use initialization list, e.g.
>   CustomElementRegistry::CustomElementRegistry()
>     : ....
>     , mCustomElementReactionStack(new CustomElementReactionStack(this))
>   {
>     ...
>   }
> 
Will do.
> @@ +247,5 @@
> >  
> >  CustomElementRegistry::~CustomElementRegistry()
> >  {
> >    mozilla::DropJSObjects(this);
> > +  mCustomElementReactionStack = nullptr;
> 
> Destructor of a RefPtr will do release, so you don't need setting to nullptr
> here.
> 
Will do.
> @@ +480,5 @@
> >        if (!elem) {
> >          continue;
> >        }
> >  
> > +      EnqueueUpgradeReaction(elem, aDefinition);
> 
> * Now we process upgrade in microtask checkpoint, but the create callback is
> still at script blocker time, so the create callback might be processed
> before upgrade. Looks like we should put EnqueueLifecycleCallback() into
> UpgradeReaction().
> 
> Note that we need to be care of the problem like bug 1176829. But I wonder
> the issue will still exist if we already EnqueueLifecycleCallback() in
> microtask time....
> 
> * And now the upgrade will happen asynchronously, I expect we need to update
> existing test cases as well.
> 
Will do.
> @@ +828,5 @@
> > +CustomElementRegistry::EnqueueUpgradeReaction(Element* aElement,
> > +                                              CustomElementDefinition* aDefinition)
> > +{
> > +  mCustomElementReactionStack->Enqueue(
> > +    aElement, new CustomElementUpgradeReaction(aDefinition));
> 
> I prefer creating CustomElementUpgradeReaction in CustomElementReactionStack
> instead which makes the life cycle of reaction clearer.
> 
I prefer don't create CustomElementUpgradeReaction in CustomElementReactionStack, because it has other parameters in callback reaction, e.g.attributeChangedCallback [1] and it needs to do Enqueue()[2]. It can make Enqueue() function more clear. 

[1] https://html.spec.whatwg.org/#custom-element-reactions
[2] step 6. https://html.spec.whatwg.org/#enqueue-a-custom-element-callback-reaction 

> So maybe,
> mCustomElementReactionStack->Enqueue(aElement, aDefinition);
> And do "new CustomElementUpgradeReaction(aDefinition)" in Enqueue().
> 
> @@ +871,5 @@
> > +  }
> > +}
> > +
> > +//-----------------------------------------------------
> > +// CustomElementDefinition
> 
> Nit: blank line after this, here and elsewhere.
> 
Will do.
> @@ +918,5 @@
> > +
> > +  CycleCollectedJSContext* context = CycleCollectedJSContext::Get();
> > +  RefPtr<AutoSetBackupQueueProcessing> backupQueueProcessingRunnable =
> > +    new AutoSetBackupQueueProcessing(this);
> > +  context->DispatchToMicroTask(do_AddRef(backupQueueProcessingRunnable));
> 
> Can we just do "backupQueueProcessingRunnable.forget()"?
> 
Will do.
> @@ +935,5 @@
> > +    nsCOMPtr<Element> element = aElementQueue[i];
> > +    ReactionQueue* reactions = mElementReactionQueueMap.Get(element);
> > +    if (!reactions->IsEmpty()) {
> > +      for (uint32_t j = 0; j < reactions->Length(); ++j) {
> > +        nsAutoPtr<CustomElementReaction> reaction = reactions->ElementAt(j);
> 
> The destructor of "reaction" will delete the object it held, and the same
> object will be deleted again when "reactions->Clear()"? It looks like a
> potential issue to me.
> 
Will do.
> ::: dom/base/CustomElementRegistry.h
> @@ +158,5 @@
> > +  NS_INLINE_DECL_REFCOUNTING(CustomElementReactionStack)
> > +
> > +  CustomElementReactionStack(RefPtr<CustomElementRegistry> aRegistry) :
> > +    mCustomElementRegistry(aRegistry),
> > +    mIsBackupQueueProcessing(false) { }
> 
> Nit: here and elsewhere.
> 
Will do.
> CustomElementReactionStack(RefPtr<CustomElementRegistry> aRegistry)
>   : mCustomElementRegistry(aRegistry)
>   , mIsBackupQueueProcessing(false)
> {
> }
> 
> @@ +159,5 @@
> > +
> > +  CustomElementReactionStack(RefPtr<CustomElementRegistry> aRegistry) :
> > +    mCustomElementRegistry(aRegistry),
> > +    mIsBackupQueueProcessing(false) { }
> > +  typedef nsTArray<nsCOMPtr<Element>> ElementQueue;
> 
> Add blank line before this.
> 
Will do.
> @@ +185,5 @@
> > +  RefPtr<CustomElementRegistry> mCustomElementRegistry;
> > +  bool mIsBackupQueueProcessing;
> > +
> > +private:
> > +  class MOZ_RAII AutoSetBackupQueueProcessing  : public mozilla::Runnable {
> 
> Hmm, it seems we probably shouldn't mark a MOZ_RAII and call it AutoWhatever
> in a Runnable class, although it update mIsBackupQueueProcessing like a RAII
> class ... Probably having clear documentations is enough.
> 
Will do.
> @@ +307,5 @@
> >    // CustomElementData in this array, separated by nullptr that
> >    // represent the boundaries of the items in the stack. The first
> >    // queue in the stack is the base element queue.
> >    static mozilla::Maybe<nsTArray<RefPtr<CustomElementData>>> sProcessingStack;
> > +  RefPtr<CustomElementReactionStack> mCustomElementReactionStack;
> 
> Add blank line around this and add some documentations.
> 
Will do.
> ::: dom/base/nsDocument.cpp
> @@ +8141,5 @@
> >  
> >    nsCOMPtr<Element> element;
> >    nsresult rv = NS_NewElement(getter_AddRefs(element), nodeInfo.forget(),
> >                                NOT_FROM_PARSER, aIs);
> > +
> 
> Remove this unnecessary change.
Will do.
- Addressed comment #12.
- Add cycle collection in CustomElementReactionStack.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=af8dd0a51bd8d955e7419820efa27c0788120ba6
Attachment #8811568 - Attachment is obsolete: true
Attachment #8813048 - Flags: feedback?(echen)
Comment on attachment 8813048 [details] [diff] [review]
Implement upgrade reaction for custom element reactions, v3

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

::: dom/base/CustomElementRegistry.cpp
@@ +547,5 @@
>                                Function& aFunctionConstructor,
>                                const ElementDefinitionOptions& aOptions,
>                                ErrorResult& aRv)
>  {
> +  mCustomElementReactionStack->Push();

I guess this probably will be remove after bug 1309147?
If so, please add comment with bug number saying we do this for [CEReaction] temporally and will be removed after webidl supports [CEReaction] annotation.

@@ +780,5 @@
>    mWhenDefinedPromiseMap.Remove(nameAtom, getter_AddRefs(promise));
>    if (promise) {
>      promise->MaybeResolveWithUndefined();
>    }
> +  mCustomElementReactionStack->Pop();

Ditto.

@@ +834,5 @@
> +CustomElementRegistry::EnqueueUpgradeReaction(Element* aElement,
> +                                              CustomElementDefinition* aDefinition)
> +{
> +  mCustomElementReactionStack->Enqueue(
> +    aElement, new CustomElementUpgradeReaction(aDefinition));

Leak on CustomElementUpgradeReaction? No one takes charge of deleting the object.

@@ +857,5 @@
> +  nsWrapperCache* cache;
> +  CallQueryInterface(aElement, &cache);
> +  MOZ_ASSERT(cache, "Element doesn't support wrapper cache?");
> +
> +

Nit: remove one blank line.

@@ +864,5 @@
> +    return;
> +  }
> +
> +  JSContext *cx = jsapi.cx();
> +  // We want to set the custom prototype in the caller's comparment.

s/comparment/compartment/

@@ +867,5 @@
> +  JSContext *cx = jsapi.cx();
> +  // We want to set the custom prototype in the caller's comparment.
> +  // In the case that element is in a different compartment,
> +  // this will set the prototype on the element's wrapper and
> +  // thus only visible in the wrapper's compartment.

Comment says we want to set the custom prototype in the caller's compartment (More specifically, it should say "the compartment of define()'s caller").
And this works because we store the prototype from define() directly, so the the prototype's compartment is the caller's compartment.
Please add some explanations.

@@ +870,5 @@
> +  // this will set the prototype on the element's wrapper and
> +  // thus only visible in the wrapper's compartment.
> +  JS::RootedObject wrapper(cx);
> +  JS::Rooted<JSObject*> prototype(cx, aDefinition->mPrototype);
> +  { // Enter constructor's compartment.

prototype's compartment?

@@ +911,5 @@
> +CustomElementReactionStack::Push()
> +{
> +  // Push a new element queue onto the custom element reactions stack.
> +  ElementQueue elementQueue;
> +  mReactionStack.AppendElement(elementQueue);

Just "mReactionStack.AppendElement()" to avoid temporary.

@@ +921,5 @@
> +  // Pop the element queue from the custom element reactions stack,
> +  // and invoke custom element reactions in that queue.
> +  if (!mReactionStack.IsEmpty()) {
> +    ElementQueue& elementQueue = mReactionStack.LastElement();
> +    CustomElementReactionStack::InvokeReactions(elementQueue);

InvokeReactions(elementQueue);

@@ +968,5 @@
> +void
> +CustomElementReactionStack::InvokeReactions(ElementQueue& aElementQueue)
> +{
> +  for (uint32_t i = 0; i < aElementQueue.Length(); ++i) {
> +    nsCOMPtr<Element> element = do_QueryReferent(aElementQueue[i]);

if (!element) {
  continue;
}

@@ +969,5 @@
> +CustomElementReactionStack::InvokeReactions(ElementQueue& aElementQueue)
> +{
> +  for (uint32_t i = 0; i < aElementQueue.Length(); ++i) {
> +    nsCOMPtr<Element> element = do_QueryReferent(aElementQueue[i]);
> +    ReactionQueue* reactions = mElementReactionQueueMap.Get(element);

Assert if could not get valid reactions from mElementReactionQueueMap.

@@ +971,5 @@
> +  for (uint32_t i = 0; i < aElementQueue.Length(); ++i) {
> +    nsCOMPtr<Element> element = do_QueryReferent(aElementQueue[i]);
> +    ReactionQueue* reactions = mElementReactionQueueMap.Get(element);
> +    if (!reactions->IsEmpty()) {
> +      for (uint32_t j = 0; j < reactions->Length(); ++j) {

We won't run into this loop if reactions is empty, so it seems IsEmpty is a redundant check.

::: dom/base/CustomElementRegistry.h
@@ +132,5 @@
> +class CustomElementReaction
> +{
> +public:
> +  explicit CustomElementReaction(CustomElementDefinition* aDefinition) :
> +    mDefinition(aDefinition)

Nit: Put `:` before mDefinition.

e.g.
explicit CustomElementReaction(CustomElementDefinition* aDefinition)
  : mDefinition(aDefinition)

Please use this style for constructors.

@@ +136,5 @@
> +    mDefinition(aDefinition)
> +    {
> +    };
> +
> +  virtual void Invoke(CustomElementRegistry* aRegistry, nsCOMPtr<Element>& aElement) = 0;

Why not "Element* aElement"?

@@ +167,5 @@
> +    mIsBackupQueueProcessing(false)
> +    {
> +    }
> +
> +  typedef nsTArray<nsWeakPtr> ElementQueue;

Please add comment saying nsWeakPtr is a weak pointer of Element and the relevant element reaction queue is stored in ElementReactionQueueMap.

@@ +177,5 @@
> +  void InvokeReactions(ElementQueue& aElementQueue);
> +  void InvokeBackupQueue();
> +  void Enqueue(Element* aElement, CustomElementReaction* aReaction);
> +  // [CEReactions] Before executing the algorithm's steps
> +  void Push();

s/Push/CreateAndPushElementQueue/
And add comment saying something like, "Push a new element queue onto the custom element reactions stack".

@@ +179,5 @@
> +  void Enqueue(Element* aElement, CustomElementReaction* aReaction);
> +  // [CEReactions] Before executing the algorithm's steps
> +  void Push();
> +  // [CEReactions] After executing the algorithm's steps
> +  void Pop();

s/Pop/PopAndInvokeElementQueue/
And add comment saying something like, "Pop element queue from custom element reactions stack and invoke reactions".

@@ +182,5 @@
> +  // [CEReactions] After executing the algorithm's steps
> +  void Pop();
> +
> +private:
> +  virtual ~CustomElementReactionStack();

virtual ~CustomElementReactionStack() {}

@@ +191,5 @@
> +
> +  ElementReactionQueueMap mElementReactionQueueMap;
> +
> +  nsTArray<ElementQueue> mReactionStack;
> +  nsTArray<nsWeakPtr> mBackupQueue;

ElementQueue mBackupQueue;

@@ +220,5 @@
> +        mReactionStack->mIsBackupQueueProcessing = false;
> +      }
> +
> +    private:
> +      CustomElementReactionStack* mReactionStack;

Use raw pointer here is not safe. It could be possible that CustomElementReactionStack is already deleted before this runnable starts running.

@@ +320,5 @@
>    // queue in the stack is the base element queue.
>    static mozilla::Maybe<nsTArray<RefPtr<CustomElementData>>> sProcessingStack;
>  
>    // It is used to prevent reentrant invocations of element definition.
>    bool mIsCustomDefinitionRunning;

Nit: blank line after this.
Attachment #8813048 - Flags: feedback?(echen)
Comment on attachment 8813048 [details] [diff] [review]
Implement upgrade reaction for custom element reactions, v3

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

::: dom/base/CustomElementRegistry.h
@@ +216,5 @@
> +      }
> +
> +    private:
> +      ~AutoSetBackupQueueProcessing() {
> +        mReactionStack->mIsBackupQueueProcessing = false;

On a second thought, we probably should clear processing flag in Run(), instead of in destructor. For a corner case: runnable are run but not yet deleted (someone still hold a reference of runnable) and we try to enqueue a element into backup queue at the point.
- Addressed comment #14.
- Per offline discussion, I merged CustomElementReactionStack into CustomElementRegistry. There are two reasons why I need to merge.
Firstly, CustomElementReactionStack and CustomElementRegistry are same lifecycle. 
Secondly, if browser is closing, it may have potential risk when runnable has been deployed but not yet completed. 

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3f0cc79e43bba326a35bcc8e2c744f78cb7af36
Attachment #8813048 - Attachment is obsolete: true
Attachment #8813545 - Flags: feedback?(echen)
Comment on attachment 8813545 [details] [diff] [review]
Implement upgrade reaction for custom element reactions, v4

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

::: dom/base/CustomElementRegistry.cpp
@@ +161,5 @@
>    NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>    NS_INTERFACE_MAP_ENTRY(nsISupports)
>  NS_INTERFACE_MAP_END
>  
> +

Nit: Remove this blank line.

@@ +538,5 @@
>                                const ElementDefinitionOptions& aOptions,
>                                ErrorResult& aRv)
>  {
> +  // We do this for [CEReaction] temporally and will be removed
> +  // after webidl supports [CEReaction] annotation.

Add bug number which is filed to support [CEReaction] annotation.

@@ +851,5 @@
> +  nsWrapperCache* cache;
> +  CallQueryInterface(aElement, &cache);
> +  MOZ_ASSERT(cache, "Element doesn't support wrapper cache?");
> +
> +

Nit: Remove this blank line

@@ +902,5 @@
> +}
> +
> +void
> +CustomElementRegistry::Enqueue(Element* aElement,
> +                                    CustomElementReaction* aReaction)

Nit: Indentation. Align arguments.

@@ +916,5 @@
> +  }
> +
> +  // If the custom element reactions stack is empty, then:
> +  // Add element to the backup element queue.
> +  mBackupQueue.AppendElement(do_GetWeakReference(aElement));

It would be good if we could have some way to test this codepath. At least, please file a follow-up bug.

@@ +951,5 @@
> +
> +    ReactionQueue* reactions = mElementReactionQueueMap.Get(element);
> +
> +    MOZ_ASSERT(!reactions->IsEmpty(),
> +               "Reaction queue should not be empty");

MOZ_ASSERT(reactions, "Associated ReactionQueue must be found in mElementReactionQueueMap");
And remove blank line.

@@ +954,5 @@
> +    MOZ_ASSERT(!reactions->IsEmpty(),
> +               "Reaction queue should not be empty");
> +
> +    for (uint32_t j = 0; j < reactions->Length(); ++j) {
> +      CustomElementReaction* reaction = reactions->ElementAt(j);

AutoPtr<CustomElementReaction>& reaction = ...

@@ +955,5 @@
> +               "Reaction queue should not be empty");
> +
> +    for (uint32_t j = 0; j < reactions->Length(); ++j) {
> +      CustomElementReaction* reaction = reactions->ElementAt(j);
> +      reaction->Invoke(mCustomElementRegistry, element);

"this", instead of "mCustomElementRegistry"
We probably could just cache a pointer of mCustomElementRegistry in CustomElementReaction, then we don't need to pass "self" when calling Invoke.
And that pointer could be a raw pointer since the life cycle of CustomElementReaction is controlled by mCustomElementRegistry.

@@ +963,5 @@
> +  aElementQueue.Clear();
> +}
> +
> +//-----------------------------------------------------
> +// CustomElementDefinition

Thank you for doing this.

@@ +984,5 @@
> +
> +//-----------------------------------------------------
> +// CustomElementUpgradeReaction
> +
> +void

/* virtual */ void

@@ +989,5 @@
> +CustomElementUpgradeReaction::Invoke(CustomElementRegistry* aRegistry,
> +                                     Element* aElement)
> +{
> +  aRegistry->Upgrade(aElement, mDefinition);
> +}

Nit: Add blank line after this.

::: dom/base/CustomElementRegistry.h
@@ +143,5 @@
> +protected:
> +  CustomElementDefinition* mDefinition;
> +};
> +
> +class CustomElementUpgradeReaction : public CustomElementReaction

Add final annotation to the class.

@@ +152,5 @@
> +  {
> +  }
> +
> +private:
> +   void Invoke(CustomElementRegistry* aRegistry,Element* aElement) override;

1. Add virtual annotation.
2. Why this is private? It is called by CustomElementRegistry::InvokeReactions().
2. Nit: add space after ','.

@@ +198,5 @@
> +  /**
> +   * Enqueue a custom element upgrade reaction
> +   * https://html.spec.whatwg.org/multipage/scripting.html#enqueue-a-custom-element-upgrade-reaction
> +   */
> +  void EnqueueUpgradeReaction(Element* aElement,

Put EnqueueUpgradeReaction() and Enqueue() together, and could they be private methods?

@@ +204,5 @@
> +
> +  void Upgrade(Element* aElement, CustomElementDefinition* aDefinition);
> +
> +  // nsWeakPtr is a weak pointer of Element
> +  typedef nsTArray<nsWeakPtr> ElementQueue;

// nsWeakPtr is a weak pointer of Element.
// The element reaction queues are stored in ElementReactionQueueMap.
// We need to lookup ElementReactionQueueMap again to get relevant reaction queue.

@@ +210,5 @@
> +  /**
> +   * Invoke custom element reactions
> +   * https://html.spec.whatwg.org/multipage/scripting.html#invoke-custom-element-reactions
> +   */
> +  void InvokeReactions(ElementQueue& aElementQueue);

InvokeReactions() could be a private method.
Then the typedef for ElementQueue could be private, too.

@@ +279,5 @@
> +    ElementReactionQueueMap;
> +
> +  ElementReactionQueueMap mElementReactionQueueMap;
> +
> +  nsTArray<ElementQueue> mReactionStack;

s/mReactionStack/mReactionsStack/

@@ +282,5 @@
> +
> +  nsTArray<ElementQueue> mReactionStack;
> +  ElementQueue mBackupQueue;
> +
> +  RefPtr<CustomElementRegistry> mCustomElementRegistry;

We don't need this I guess.

@@ +283,5 @@
> +  nsTArray<ElementQueue> mReactionStack;
> +  ElementQueue mBackupQueue;
> +
> +  RefPtr<CustomElementRegistry> mCustomElementRegistry;
> +  bool mIsBackupQueueProcessing;

Put this with mIsCustomDefinitionRunning together with a blank line between them.

@@ +324,5 @@
> +        return NS_OK;
> +      }
> +
> +    private:
> +      CustomElementRegistry* mRegistry;

RefPtr<CustomElementRegistry>
Attachment #8813545 - Flags: feedback?(echen)
(In reply to Edgar Chen [:edgar][:echen] from comment #17)
> Comment on attachment 8813545 [details] [diff] [review]
> Implement upgrade reaction for custom element reactions, v4
> 
> Review of attachment 8813545 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/CustomElementRegistry.cpp
> @@ +161,5 @@
> >    NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> >    NS_INTERFACE_MAP_ENTRY(nsISupports)
> >  NS_INTERFACE_MAP_END
> >  
> > +
> 
> Nit: Remove this blank line.
> 
Will do.
> @@ +538,5 @@
> >                                const ElementDefinitionOptions& aOptions,
> >                                ErrorResult& aRv)
> >  {
> > +  // We do this for [CEReaction] temporally and will be removed
> > +  // after webidl supports [CEReaction] annotation.
> 
> Add bug number which is filed to support [CEReaction] annotation.
> 
Will do.
> @@ +851,5 @@
> > +  nsWrapperCache* cache;
> > +  CallQueryInterface(aElement, &cache);
> > +  MOZ_ASSERT(cache, "Element doesn't support wrapper cache?");
> > +
> > +
> 
> Nit: Remove this blank line
> 
Will do.
> @@ +902,5 @@
> > +}
> > +
> > +void
> > +CustomElementRegistry::Enqueue(Element* aElement,
> > +                                    CustomElementReaction* aReaction)
> 
> Nit: Indentation. Align arguments.
> 
Will do.
> @@ +916,5 @@
> > +  }
> > +
> > +  // If the custom element reactions stack is empty, then:
> > +  // Add element to the backup element queue.
> > +  mBackupQueue.AppendElement(do_GetWeakReference(aElement));
> 
> It would be good if we could have some way to test this codepath. At least,
> please file a follow-up bug.
> 
I think the test will cover in bug 1319342.
> @@ +951,5 @@
> > +
> > +    ReactionQueue* reactions = mElementReactionQueueMap.Get(element);
> > +
> > +    MOZ_ASSERT(!reactions->IsEmpty(),
> > +               "Reaction queue should not be empty");
> 
> MOZ_ASSERT(reactions, "Associated ReactionQueue must be found in
> mElementReactionQueueMap");
> And remove blank line.
> 
Will do.
> @@ +954,5 @@
> > +    MOZ_ASSERT(!reactions->IsEmpty(),
> > +               "Reaction queue should not be empty");
> > +
> > +    for (uint32_t j = 0; j < reactions->Length(); ++j) {
> > +      CustomElementReaction* reaction = reactions->ElementAt(j);
> 
> AutoPtr<CustomElementReaction>& reaction = ...
> 
Will do.
> @@ +955,5 @@
> > +               "Reaction queue should not be empty");
> > +
> > +    for (uint32_t j = 0; j < reactions->Length(); ++j) {
> > +      CustomElementReaction* reaction = reactions->ElementAt(j);
> > +      reaction->Invoke(mCustomElementRegistry, element);
> 
> "this", instead of "mCustomElementRegistry"
> We probably could just cache a pointer of mCustomElementRegistry in
> CustomElementReaction, then we don't need to pass "self" when calling Invoke.
> And that pointer could be a raw pointer since the life cycle of
> CustomElementReaction is controlled by mCustomElementRegistry.
> 
Will do.
> @@ +963,5 @@
> > +  aElementQueue.Clear();
> > +}
> > +
> > +//-----------------------------------------------------
> > +// CustomElementDefinition
> 
> Thank you for doing this.
> 
> @@ +984,5 @@
> > +
> > +//-----------------------------------------------------
> > +// CustomElementUpgradeReaction
> > +
> > +void
> 
> /* virtual */ void
> 
Will do.
> @@ +989,5 @@
> > +CustomElementUpgradeReaction::Invoke(CustomElementRegistry* aRegistry,
> > +                                     Element* aElement)
> > +{
> > +  aRegistry->Upgrade(aElement, mDefinition);
> > +}
> 
> Nit: Add blank line after this.
> 
Will do.
> ::: dom/base/CustomElementRegistry.h
> @@ +143,5 @@
> > +protected:
> > +  CustomElementDefinition* mDefinition;
> > +};
> > +
> > +class CustomElementUpgradeReaction : public CustomElementReaction
> 
> Add final annotation to the class.
> 
Will do.
> @@ +152,5 @@
> > +  {
> > +  }
> > +
> > +private:
> > +   void Invoke(CustomElementRegistry* aRegistry,Element* aElement) override;
> 
> 1. Add virtual annotation.
> 2. Why this is private? It is called by
> CustomElementRegistry::InvokeReactions().
> 2. Nit: add space after ','.
> 
Will do.
> @@ +198,5 @@
> > +  /**
> > +   * Enqueue a custom element upgrade reaction
> > +   * https://html.spec.whatwg.org/multipage/scripting.html#enqueue-a-custom-element-upgrade-reaction
> > +   */
> > +  void EnqueueUpgradeReaction(Element* aElement,
> 
> Put EnqueueUpgradeReaction() and Enqueue() together, and could they be
> private methods?
> 

I think EnqueueUpgradeReaction() should be public and Enqueue() should be private. Because create an element
 in step 5.4[1], it needs to enqueue a custom element upgrade reaction. For architecture perspective, we'll add EnqueueConnectedCallback() and other callback functions in bug 1315885. I think leave this two functions don't cause much harm.

[1] https://dom.spec.whatwg.org/#concept-create-element
> @@ +204,5 @@
> > +
> > +  void Upgrade(Element* aElement, CustomElementDefinition* aDefinition);
> > +
> > +  // nsWeakPtr is a weak pointer of Element
> > +  typedef nsTArray<nsWeakPtr> ElementQueue;
> 
> // nsWeakPtr is a weak pointer of Element.
> // The element reaction queues are stored in ElementReactionQueueMap.
> // We need to lookup ElementReactionQueueMap again to get relevant reaction
> queue.
> 
Will do.
> @@ +210,5 @@
> > +  /**
> > +   * Invoke custom element reactions
> > +   * https://html.spec.whatwg.org/multipage/scripting.html#invoke-custom-element-reactions
> > +   */
> > +  void InvokeReactions(ElementQueue& aElementQueue);
> 
> InvokeReactions() could be a private method.
> Then the typedef for ElementQueue could be private, too.
> 
Will do.
> @@ +279,5 @@
> > +    ElementReactionQueueMap;
> > +
> > +  ElementReactionQueueMap mElementReactionQueueMap;
> > +
> > +  nsTArray<ElementQueue> mReactionStack;
> 
> s/mReactionStack/mReactionsStack/
> 
Will do.
> @@ +282,5 @@
> > +
> > +  nsTArray<ElementQueue> mReactionStack;
> > +  ElementQueue mBackupQueue;
> > +
> > +  RefPtr<CustomElementRegistry> mCustomElementRegistry;
> 
> We don't need this I guess.
> 
Will do.
> @@ +283,5 @@
> > +  nsTArray<ElementQueue> mReactionStack;
> > +  ElementQueue mBackupQueue;
> > +
> > +  RefPtr<CustomElementRegistry> mCustomElementRegistry;
> > +  bool mIsBackupQueueProcessing;
> 
> Put this with mIsCustomDefinitionRunning together with a blank line between
> them.
> 
Will do.
> @@ +324,5 @@
> > +        return NS_OK;
> > +      }
> > +
> > +    private:
> > +      CustomElementRegistry* mRegistry;
> 
> RefPtr<CustomElementRegistry>

Will do.
Comment on attachment 8813664 [details] [diff] [review]
Implement upgrade reaction for custom element reactions, v5

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

::: dom/base/CustomElementRegistry.cpp
@@ +161,5 @@
>    NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>    NS_INTERFACE_MAP_ENTRY(nsISupports)
>  NS_INTERFACE_MAP_END
>  
> +

Nit: remove this blank line.

@@ +950,5 @@
> +
> +    ReactionQueue* reactions = mElementReactionQueueMap.Get(element);
> +
> +    MOZ_ASSERT(reactions,
> +               "Associated ReactionQueue must be found in mElementReactionQueueMap");

Nit: Move the previous blank line here.

::: dom/base/CustomElementRegistry.h
@@ +232,5 @@
> +  void Enqueue(Element* aElement, CustomElementReaction* aReaction);
> +
> +  // [CEReactions] Before executing the algorithm's steps
> +  // Push a new element queue onto the custom element reactions stack.
> +  void CreateAndPushElementQueue();

I guess this might need to be public for bug 1309147?

@@ +237,5 @@
> +
> +  // [CEReactions] After executing the algorithm's steps
> +  // Pop the element queue from the custom element reactions stack,
> +  // and invoke custom element reactions in that queue.
> +  void PopAndInvokeElementQueue();

Ditto.
Attachment #8813664 - Flags: feedback?(echen) → feedback+
Comment on attachment 8813953 [details] [diff] [review]
Implement upgrade reaction for custom element reactions, v6

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

Sorry for the delay. If you make any big changes I would like to look at the patches again before they land.

::: dom/base/CustomElementRegistry.cpp
@@ +536,5 @@
>                                Function& aFunctionConstructor,
>                                const ElementDefinitionOptions& aOptions,
>                                ErrorResult& aRv)
>  {
> +  // We do this for [CEReaction] temporally and will be removed

nit: temporally -> temporarily here and below

@@ +890,5 @@
> +{
> +  // Pop the element queue from the custom element reactions stack,
> +  // and invoke custom element reactions in that queue.
> +  if (mReactionsStack.IsEmpty()) {
> +    return;

This shouldn't happen if CreateAndPushElementQueue() and PopAndInvokeElementQueue() are balanced. When more reactions are implemented, this method may not work correctly if we are missing a call to push because we may end up trying to invoke the last element queue on the reaction stack multiple times (if ::InvokeReactions ends up invoking ::PopAndInvokeElementQueue without a corresponding push). Could we use an RAII class to do the push and pop, and assert that we always have something on the reaction stack? Also, could you please add an assertion below that RemoveElement actually removes an element?

@@ +955,5 @@
> +    for (uint32_t j = 0; j < reactions->Length(); ++j) {
> +      nsAutoPtr<CustomElementReaction>& reaction = reactions->ElementAt(j);
> +      reaction->Invoke(element);
> +    }
> +    reactions->Clear();

It would be nice to just remove the reaction queue from the map so we don't have an empty array that hangs around forever, although that will probably also require removing the assertion above.

::: dom/base/CustomElementRegistry.h
@@ +287,5 @@
> +  typedef nsTArray<nsAutoPtr<CustomElementReaction>> ReactionQueue;
> +  typedef nsClassHashtable<nsISupportsHashKey, ReactionQueue>
> +    ElementReactionQueueMap;
> +
> +  ElementReactionQueueMap mElementReactionQueueMap;

Elements have a structured called CustomElementData in the slots, you may be able put the reaction queue in that structure instead of using a map, but it will require a bit of refactoring to create the CustomElementData a bit earlier when we add elements to the reactions stack. I'll leave it up to you to determine if that's worth the effort.

@@ +312,5 @@
>        CustomElementRegistry* mRegistry;
>    };
>  
> +private:
> +  // RAII class to auto set processing the backup element queue flag.

This isn't really an RAII class and Auto suggests that this is a stack class which it isn't. How about renaming this ProcessBackupQueueRunnable or something similar?
Attachment #8813953 - Flags: review?(wchen) → review+
Blocks: 1325279
> ::: dom/base/CustomElementRegistry.h
> @@ +287,5 @@
> > +  typedef nsTArray<nsAutoPtr<CustomElementReaction>> ReactionQueue;
> > +  typedef nsClassHashtable<nsISupportsHashKey, ReactionQueue>
> > +    ElementReactionQueueMap;
> > +
> > +  ElementReactionQueueMap mElementReactionQueueMap;
> 
> Elements have a structured called CustomElementData in the slots, you may be
> able put the reaction queue in that structure instead of using a map, but it
> will require a bit of refactoring to create the CustomElementData a bit
> earlier when we add elements to the reactions stack. I'll leave it up to you
> to determine if that's worth the effort.
> 
File bug 1325279.
> @@ +955,5 @@
> > +    for (uint32_t j = 0; j < reactions->Length(); ++j) {
> > +      nsAutoPtr<CustomElementReaction>& reaction = reactions->ElementAt(j);
> > +      reaction->Invoke(element);
> > +    }
> > +    reactions->Clear();
> 
> It would be nice to just remove the reaction queue from the map so we don't
> have an empty array that hangs around forever, although that will probably
> also require removing the assertion above.
> 

The assertion above is to make sure each element has a associated reaction queue in mElementReactionQueueMap. I prefer keep this assertion.


Hi William,
Thanks for the review. I addressed comment #22. Could you help me to review it again? Thank you.
Attachment #8813953 - Attachment is obsolete: true
Attachment #8821979 - Flags: review?(wchen)
- Use DebugOnly<bool> on isRemovedElement.
 - Use mElementReactionQueueMap.RemoveAndForget() instead of mElementReactionQueueMap.Get().
 - Remove AutoCEReaction unnecessary comment.
Attachment #8821979 - Attachment is obsolete: true
Attachment #8821979 - Flags: review?(wchen)
Attachment #8822022 - Flags: review?(wchen)
Hi William,

It's just a gentle ping, could you help to review my patch?

Thanks,
John
Flags: needinfo?(wchen)
Attachment #8822022 - Flags: review?(wchen) → review+
Flags: needinfo?(wchen)
Comment on attachment 8822022 [details] [diff] [review]
Implement upgrade reaction for custom element reactions. r=wchen

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1a0c70c9df65532a8d33c42609d24bf6e1f90bc&filter-tier=1&group_state=expanded
Attachment #8822022 - Attachment description: Implement upgrade reaction for custom element reactions, v8 → Implement upgrade reaction for custom element reactions. r=wchen
Keywords: checkin-needed
please rebase the trunk.
applying bug_1309184_v8.patch
patching file dom/base/CustomElementRegistry.cpp
Hunk #1 FAILED at 131
1 out of 5 hunks FAILED -- saving rejects to file dom/base/CustomElementRegistry.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug_1309184_v8.patch
Keywords: checkin-needed
Rebased.
Attachment #8822022 - Attachment is obsolete: true
Attachment #8824303 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dafb5e5e3311
Implement upgrade reaction for custom element reactions. r=wchen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dafb5e5e3311
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
See Also: → 1347446
Blocks: 1319342
Hmm, so this made custom elements not use proper microtasks, I think.
I know it is super confusing but DispatchToMicroTask has nothing to do with microtasks, but about our broken Promise handling.
But this stuff is being fixed.
(In reply to Olli Pettay [:smaug] from comment #32)
> Hmm, so this made custom elements not use proper microtasks, I think.
> I know it is super confusing but DispatchToMicroTask has nothing to do with
> microtasks, but about our broken Promise handling.
> But this stuff is being fixed.

Thank you for the comment.
Do we need to do anything on custom elements? I am willing to help.
Or DispatchToMicroTask will do the right thing eventually?
Flags: needinfo?(bugs)
Let me see. I assume that will just get fixed while finally driving bug 1193394 in.
Flags: needinfo?(bugs)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: