Refactoring: HTMLContentSelector should be InsertionPoint-aware

• Component: DOM
• Status: RESOLVED FIXED

[Shinya Kawanaka]

Currently it only consider HTMLContentElement.
It should consider InsertionPoint instead of HTMLContentElement.

[Shinya Kawanaka]

I want to use HTMLContentSelector in <shadow> element for selecting the rest of light elements.

To support this, If HTMLContentSelector::select takes non HTMLContentElement InsertionPoint, we want to choose all of the rest light children.

[Shinya Kawanaka]

Created attachment 129433 [details]
Patch

[Hajime Morrita]

Comment on attachment 129433 [details]
Patch

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

> Source/WebCore/html/shadow/HTMLContentSelector.cpp:117
> +        query = adoptPtr(new ContentSelectorQuery(toHTMLContentElement(insertionPoint)));

I don't want to allocate this on heap since this will be hit for each attach()
even though we do it while following code.
Also, I hope these decisions being done by ContentSelectorQuery to make HTMLContentSelector element-type agnostic.

I feel that we should also get rid of such heap allocation. But it's another homework.

[Shinya Kawanaka]

Created attachment 129608 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #3)
> (From update of attachment 129433 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129433&action=review
> 
> > Source/WebCore/html/shadow/HTMLContentSelector.cpp:117
> > +        query = adoptPtr(new ContentSelectorQuery(toHTMLContentElement(insertionPoint)));
> 
> I don't want to allocate this on heap since this will be hit for each attach()
> even though we do it while following code.
> Also, I hope these decisions being done by ContentSelectorQuery to make HTMLContentSelector element-type agnostic.

Fixed this issue.

> 
> I feel that we should also get rid of such heap allocation. But it's another homework.

[Hajime Morrita]

Comment on attachment 129608 [details]
Patch

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

> Source/WebCore/html/shadow/ContentSelectorQuery.cpp:43
> +    if (!insertionPoint->isContentElement()) {

Having these explicit type check is unfortunate.
Can you pull thee stuff up to InsertionPoint to make this code path more straight?
My feeling is that we can just pull select() up and return empty or null in HTMLShadowElement. Or we mihght able to add some polymorphic method to InsertionPoint.

> Source/WebCore/html/shadow/ContentSelectorQuery.cpp:76
> +    if (!m_insertionPoint->isContentElement())

Ditto about type check.

[Shinya Kawanaka]

(In reply to comment #6)
> (From update of attachment 129608 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129608&action=review
> 
> > Source/WebCore/html/shadow/ContentSelectorQuery.cpp:43
> > +    if (!insertionPoint->isContentElement()) {
> 
> Having these explicit type check is unfortunate.
> Can you pull thee stuff up to InsertionPoint to make this code path more straight?
> My feeling is that we can just pull select() up and return empty or null in HTMLShadowElement. Or we mihght able to add some polymorphic method to InsertionPoint.
> 
> > Source/WebCore/html/shadow/ContentSelectorQuery.cpp:76
> > +    if (!m_insertionPoint->isContentElement())
> 
> Ditto about type check.

I agree. I'll update the patch soon.

[Shinya Kawanaka]

Created attachment 129633 [details]
Patch

[Hajime Morrita]

Comment on attachment 129633 [details]
Patch

Looks good.

[Shinya Kawanaka]

Committed r109313: <http://trac.webkit.org/changeset/109313>