Bug 79901

Summary: Refactoring: HTMLContentSelector should be InsertionPoint-aware
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: DOMAssignee: Shinya Kawanaka <shinyak>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dominicc, hayato, morrita, rolandsteiner, shinyak
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 78596    
Attachments:
Description Flags
Patch
none
Patch
none
Patch morrita: review+

Shinya Kawanaka
Reported 2012-02-29 03:11:28 PST
Currently it only consider HTMLContentElement. It should consider InsertionPoint instead of HTMLContentElement.
Attachments
Patch (6.12 KB, patch)
2012-02-29 03:20 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (10.31 KB, patch)
2012-02-29 18:33 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (10.95 KB, patch)
2012-02-29 22:21 PST, Shinya Kawanaka 		morrita: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Shinya Kawanaka
Comment 1 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.
Shinya Kawanaka
Comment 2 2012-02-29 03:20:59 PST
Created attachment 129433 [details] Patch
Hajime Morrita
Comment 3 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.
Shinya Kawanaka
Comment 4 2012-02-29 18:33:51 PST
Created attachment 129608 [details] Patch
Shinya Kawanaka
Comment 5 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.
Hajime Morrita
Comment 6 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.
Shinya Kawanaka
Comment 7 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.
Shinya Kawanaka
Comment 8 2012-02-29 22:21:04 PST
Created attachment 129633 [details] Patch
Hajime Morrita
Comment 9 2012-02-29 22:34:11 PST
Comment on attachment 129633 [details] Patch Looks good.
Shinya Kawanaka
Comment 10 2012-02-29 22:39:50 PST
Committed r109313: <http://trac.webkit.org/changeset/109313>
Note You need to log in before you can comment on or make changes to this bug.