Bug 79901 - Refactoring: HTMLContentSelector should be InsertionPoint-aware
Summary: Refactoring: HTMLContentSelector should be InsertionPoint-aware
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords:
Depends on:
Blocks: 78596
  Show dependency treegraph
 
Reported: 2012-02-29 03:11 PST by Shinya Kawanaka
Modified: 2012-02-29 22:39 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.12 KB, patch)
2012-02-29 03:20 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (10.31 KB, patch)
2012-02-29 18:33 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (10.95 KB, patch)
2012-02-29 22:21 PST, Shinya Kawanaka
morrita: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-02-29 03:11:28 PST
Currently it only consider HTMLContentElement.
It should consider InsertionPoint instead of HTMLContentElement.
Comment 1 Shinya Kawanaka 2012-02-29 03:16:41 PST
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.
Comment 2 Shinya Kawanaka 2012-02-29 03:20:59 PST
Created attachment 129433 [details]
Patch
Comment 3 Hajime Morrita 2012-02-29 17:43:09 PST
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.
Comment 4 Shinya Kawanaka 2012-02-29 18:33:51 PST
Created attachment 129608 [details]
Patch
Comment 5 Shinya Kawanaka 2012-02-29 21:27:50 PST
(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.
Comment 6 Hajime Morrita 2012-02-29 21:36:28 PST
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.
Comment 7 Shinya Kawanaka 2012-02-29 21:59:21 PST
(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.
Comment 8 Shinya Kawanaka 2012-02-29 22:21:04 PST
Created attachment 129633 [details]
Patch
Comment 9 Hajime Morrita 2012-02-29 22:34:11 PST
Comment on attachment 129633 [details]
Patch

Looks good.
Comment 10 Shinya Kawanaka 2012-02-29 22:39:50 PST
Committed r109313: <http://trac.webkit.org/changeset/109313>