1347446 - Move custom element reactions stack to DocGroup

Status: RESOLVED FIXED

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

Description

[John Dai[:jdai]]

Per bug 1309147 comment #33, we need to move custom element reactions stack to DocGroup.

Comment 1

[John Dai[:jdai]]

Attachment: patch, v1

In this patch, I introduce a CustomElementReactionsStack, which is a member of docgroup and move upgrade reaction's code into it.

Comment 2

[John Dai[:jdai]]

Comment on attachment 8847478 [details] [diff] [review]
patch, v1

Retrieved this feedback since it has some part of patch need to grab from bug 1309147.

Comment 3

[John Dai[:jdai]]

Attachment: patch, v2

I added Element header file in the dom/flyweb/FlyWebDiscoveryManager.cpp, it is because flyweb uses unity builds. I list those errors in below:

[1]
0:07.03 In file included from /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsISupportsUtils.h:15:0,
 0:07.03                  from /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsISupports.h:130,
 0:07.03                  from /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsHashKeys.h:11,
 0:07.03                  from /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsClassHashtable.h:12,
 0:07.03                  from /home/john/workspace/john/workspace/firefox/dom/flyweb/FlyWebDiscoveryManager.cpp:9,
 0:07.03                  from /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dom/flyweb/Unified_cpp_dom_flyweb0.cpp:2:
 0:07.03 /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h: In instantiation of 'static void mozilla::RefPtrTraits<U>::Release(U*) [with U = mozilla::dom::Element]':
 0:07.03 /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:395:40:   required from 'static void RefPtr<T>::ConstRemovingRefPtrTraits<U>::Release(U*) [with U = mozilla::dom::Element; T = mozilla::dom::Element]'
 0:07.03 /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:78:44:   required from 'RefPtr<T>::~RefPtr() [with T = mozilla::dom::Element]'
 0:07.03 /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/CustomElementRegistry.h:38:7:   required from 'nsAutoPtr<T>::~nsAutoPtr() [with T = mozilla::dom::CustomElementCallback]'
 0:07.03 /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:559:40:   required from 'static void nsTArrayElementTraits<E>::Destruct(E*) [with E = nsAutoPtr<mozilla::dom::CustomElementCallback>]'
 0:07.03 /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:1942:28:   required from 'void nsTArray_Impl<E, Alloc>::DestructRange(nsTArray_Impl<E, Alloc>::index_type, nsTArray_Impl<E, Alloc>::size_type) [with E = nsAutoPtr<mozilla::dom::CustomElementCallback>; Alloc = nsTArrayInfallibleAllocator; nsTArray_Impl<E, Alloc>::index_type = long unsigned int; nsTArray_Impl<E, Alloc>::size_type = long unsigned int]'
 0:07.03 /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:1986:16:   required from 'void nsTArray_Impl<E, Alloc>::RemoveElementsAt(nsTArray_Impl<E, Alloc>::index_type, nsTArray_Impl<E, Alloc>::size_type) [with E = nsAutoPtr<mozilla::dom::CustomElementCallback>; Alloc = nsTArrayInfallibleAllocator; nsTArray_Impl<E, Alloc>::index_type = long unsigned int; nsTArray_Impl<E, Alloc>::size_type = long unsigned int]'
 0:07.03 /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:1668:34:   required from 'void nsTArray_Impl<E, Alloc>::Clear() [with E = nsAutoPtr<mozilla::dom::CustomElementCallback>; Alloc = nsTArrayInfallibleAllocator]'
 0:07.03 /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:881:27:   required from 'nsTArray_Impl<E, Alloc>::~nsTArray_Impl() [with E = nsAutoPtr<mozilla::dom::CustomElementCallback>; Alloc = nsTArrayInfallibleAllocator]'
 0:07.03 /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:2152:7:   required from here
 0:07.03 /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:40:5: error: invalid use of incomplete type 'class mozilla::dom::Element'

[2]
 0:07.03 In file included from /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/xpcpublic.h:19:0,
 0:07.04                  from /home/john/workspace/john/workspace/firefox/dom/base/nsJSEnvironment.h:19,
 0:07.04                  from /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/CallbackObject.h:31,
 0:07.04                  from /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/CallbackInterface.h:19,
 0:07.04                  from /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/FlyWebDiscoveryManagerBinding.h:10,
 0:07.04                  from /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/FlyWebDiscoveryManager.h:14,
 0:07.04                  from /home/john/workspace/john/workspace/firefox/dom/flyweb/FlyWebDiscoveryManager.cpp:17,
 0:07.04                  from /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dom/flyweb/Unified_cpp_dom_flyweb0.cpp:2:
 0:07.04 /home/john/workspace/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsPIDOMWindow.h:50:7: note: forward declaration of 'class mozilla::dom::Element'
 0:07.04 ICECC[11207] 16:29:37: Compiled on 10.247.28.92
 0:07.04

Comment 4

[Edgar Chen [:edgar]]

Comment on attachment 8847522 [details] [diff] [review]
patch, v2

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

f=me with the comments.

::: dom/base/CustomElementRegistry.cpp
@@ +478,5 @@
>        if (!elem) {
>          continue;
>        }
>  
> +      mWindow->GetDocGroup()->GetCustomElementReactionsStack()->EnqueueUpgradeReaction(this, elem, aDefinition);

It is not necessary to get reactionStack from mWindow repeatedly for each enqueue.

@@ +900,5 @@
>    MOZ_ASSERT(!mReactionsStack.IsEmpty(),
>               "Reaction stack shouldn't be empty");
>  
>    ElementQueue& elementQueue = mReactionsStack.LastElement();
> +  CustomElementReactionsStack::InvokeReactions(elementQueue);

s/CustomElementReactionsStack::/ /

@@ +977,5 @@
>    aElementQueue.Clear();
>  }
>  
> +CustomElementReactionsStack::~CustomElementReactionsStack()
> +{

We do nothing here, it seems we could just use default destructor.

::: dom/base/CustomElementRegistry.h
@@ +171,5 @@
> +public:
> +  NS_INLINE_DECL_REFCOUNTING(CustomElementReactionsStack)
> +
> +  CustomElementReactionsStack() :
> +    mIsBackupQueueProcessing(false)

It will be good to have a consistent style for all ctors in this file.
i.e

  CustomElementReactionsStack()
    : mIsBackupQueueProcessing(false)

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

This method was private previously. Does it need to become public when it moves to CustomElementReactionsStack?

Same for ElementQueue, InvokeBackupQueue() and Enqueue().

Comment 5

[John Dai[:jdai]]

Attachment: patch, v3

Addressed comment #4.
Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0839d7be3baa653353a74fda82d83bb3a56f6fbac&group_state=expanded&filter-tier=1

Comment 6

[John Dai[:jdai]]

> @@ +977,5 @@
> >    aElementQueue.Clear();
> >  }
> >  
> > +CustomElementReactionsStack::~CustomElementReactionsStack()
> > +{
> 
> We do nothing here, it seems we could just use default destructor.
> 
We can't use default destructor, because CustomElementReactionsStack is a Reference-counted class, it'll hit a build error " static assertion failed: Reference-counted class CustomElementReactionsStack should not have a public destructor. Make this class's destructor non-public". I changed to inline destructor instead.

Comment 7

[William Chen [:wchen]]

Comment on attachment 8847896 [details] [diff] [review]
patch, v3

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

::: dom/base/CustomElementRegistry.cpp
@@ +473,5 @@
>    nsAutoPtr<nsTArray<nsWeakPtr>> candidates;
>    mCandidatesMap.RemoveAndForget(aKey, candidates);
>    if (candidates) {
> +    CustomElementReactionsStack* reactionsStack =
> +      mWindow->GetDocGroup()->GetCustomElementReactionsStack();

We should also check  that mWindow->GetDocGroup() is non-null.

@@ +542,5 @@
>                                ErrorResult& aRv)
>  {
>    // We do this for [CEReaction] temporarily and it will be removed
>    // after webidl supports [CEReaction] annotation in bug 1309147.
> +  AutoCEReaction ceReaction(mWindow->GetDocGroup()->GetCustomElementReactionsStack());

mWindow->GetDocGroup() should also be checked here.

::: dom/base/DocGroup.cpp
@@ +40,5 @@
>  DocGroup::DocGroup(TabGroup* aTabGroup, const nsACString& aKey)
>    : mKey(aKey), mTabGroup(aTabGroup)
>  {
>    // This method does not add itself to mTabGroup->mDocGroups as the caller does it for us.
> +  mReactionsStack = new CustomElementReactionsStack();

Let's make this lazy because it's going to be very rarely used.

::: dom/base/DocGroup.h
@@ +57,5 @@
>    TabGroup* GetTabGroup()
>    {
>      return mTabGroup;
>    }
> +  CustomElementReactionsStack* GetCustomElementReactionsStack()

Drop |Get| from the method name because it's infallible and should always return a non-null value.

Comment 8

[John Dai[:jdai]]

Attachment: patch, v4

Addressed comment #7.

> ::: dom/base/DocGroup.h
> @@ +57,5 @@
> >    TabGroup* GetTabGroup()
> >    {
> >      return mTabGroup;
> >    }
> > +  CustomElementReactionsStack* GetCustomElementReactionsStack()
> 
> Drop |Get| from the method name because it's infallible and should always
> return a non-null value.

We have to specify mozilla::dom namespace in front of CustomElementReactionsStack in order to avoid compiler get confused.

Comment 9

[John Dai[:jdai]]

Attachment: patch, v5

Throw error when there is no docgroup.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c1b19c4d37c283513d93ef261af5c3572c58cad&group_state=expanded

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f85b06a8e5
Move custom element reactions stack to DocGroup. r=wchen

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c3f85b06a8e5