Bug 100922 – [Shadow] ShadowRoot should know the existence of element having ElementShadow.

• Component: DOM
• Status: RESOLVED FIXED

[Shinya Kawanaka]

This is similar to https://bugs.webkit.org/show_bug.cgi?id=100921
We would like to know the existence of elements having shadow when we check the necessity of distribution invalidation.

[Shinya Kawanaka]

The patterns where the number of elements having shadow may be changed are:

(1) An element having shadow is inserted.
(2) An element having shadow is removed.
(3) ShadowRoot is added to an element
(4) ShadowRoot is removed from an element.

[Shinya Kawanaka]

Actually we don't have to consider (4) case, since there is no API to remove it.

[Shinya Kawanaka]

Created attachment 172005 [details]
Patch

[Shinya Kawanaka]

This patch will definitely be conflicted with a patch for Bug 100921.

[Shinya Kawanaka]

Created attachment 172250 [details]
Patch

[Hajime Morrita]

Comment on attachment 172250 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172250&action=review

> Source/WebCore/dom/Element.cpp:1061
> +    if (ElementShadow* elementShadow = shadow()) {

ShadowRoot also has insertedInto(). Can we move this to there?
insertedInto() is relatively hot function and better to keep slim if possible.
Same can be said for removedFrom().

[Shinya Kawanaka]

Comment on attachment 172250 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172250&action=review

>> Source/WebCore/dom/Element.cpp:1061
>> +    if (ElementShadow* elementShadow = shadow()) {
> 
> ShadowRoot also has insertedInto(). Can we move this to there?
> insertedInto() is relatively hot function and better to keep slim if possible.
> Same can be said for removedFrom().

OK.

[Shinya Kawanaka]

The number of calling ShadowRoot::insertedInto and ShadowRoot::removedFrom does not seem balanced correctly... Maybe we have another bug around there?

[Shinya Kawanaka]

Created attachment 172508 [details]
Patch

[Hajime Morrita]

Comment on attachment 172508 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172508&action=review

> Source/WebCore/dom/ShadowRoot.cpp:263
> +    // Note that host element might be being destroyed here.

What is the point of this comment?

> Source/WebCore/dom/ShadowRoot.h:102
> +    void unregisterElementHavingShadow() { --m_numberOfElementsHavingShadow; }

Let's ASSERT() the count cannot be zero.

> Source/WebCore/dom/ShadowRoot.h:134
> +    size_t m_numberOfElementsHavingShadow;

This is basically a count of ElementShadow, is that right?
If so, I'd prefer more brief name. "ElementHavingShadow" sound a bit cryptic.

[Shinya Kawanaka]

Created attachment 172514 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #10)
> (From update of attachment 172508 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172508&action=review
> 
> > Source/WebCore/dom/ShadowRoot.cpp:263
> > +    // Note that host element might be being destroyed here.
> 
> What is the point of this comment?

Removed.

> 
> > Source/WebCore/dom/ShadowRoot.h:102
> > +    void unregisterElementHavingShadow() { --m_numberOfElementsHavingShadow; }
> 
> Let's ASSERT() the count cannot be zero.

Added.

> 
> > Source/WebCore/dom/ShadowRoot.h:134
> > +    size_t m_numberOfElementsHavingShadow;
> 
> This is basically a count of ElementShadow, is that right?
> If so, I'd prefer more brief name. "ElementHavingShadow" sound a bit cryptic.

Yes. I used m_numberOfElementShadow... I couldn't come up with better name...

[Hajime Morrita]

Comment on attachment 172514 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172514&action=review

> Source/WebCore/testing/Internals.cpp:332
> +    ec = INVALID_ACCESS_ERR;

Let's take check-error-then do happy path pattern.
Pushing error path to late is unusual thus confusing.

[Shinya Kawanaka]

Created attachment 172518 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 172518 [details]
Patch for landing

Clearing flags on attachment: 172518

Committed r133575: <http://trac.webkit.org/changeset/133575>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.