Bug 78596

Summary: <shadow> should be rendered correctly
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: DOMAssignee: Shinya Kawanaka <shinyak>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dominicc, gustavo, hayato, morrita, pnormand, rolandsteiner, shinyak, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 78771, 78778, 79901, 79902, 80373, 80501    
Bug Blocks: 77503, 79197    
Attachments:
Description Flags
WIP
none
WIP
none
Patch
none
WIP
none
WIP
none
Patch
none
WIP
none
Patch
none
Patch
none
Patch
none
Patch morrita: review+

Shinya Kawanaka
Reported 2012-02-14 03:09:21 PST
<shadow> should render an older shadow subtree.
Attachments
WIP (87.36 KB, patch)
2012-02-22 03:50 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
WIP (70.32 KB, patch)
2012-02-23 22:57 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (67.77 KB, patch)
2012-02-28 22:56 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
WIP (58.42 KB, patch)
2012-03-01 00:43 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
WIP (57.97 KB, patch)
2012-03-01 01:02 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (57.97 KB, patch)
2012-03-01 01:15 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
WIP (46.31 KB, patch)
2012-03-05 00:44 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (49.39 KB, patch)
2012-03-06 23:11 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (49.22 KB, patch)
2012-03-06 23:23 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (49.33 KB, patch)
2012-03-07 00:55 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (49.09 KB, patch)
2012-03-08 00:53 PST, Shinya Kawanaka 		morrita: review+
DetailsFormatted DiffDiff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.
Shinya Kawanaka
Comment 1 2012-02-17 00:16:54 PST
This will be easily done if my InsertionPoint refactoring. In that case, <shadow> should be derived from InsertionPoint.
Shinya Kawanaka
Comment 2 2012-02-22 03:50:40 PST
Created attachment 128173 [details] WIP
Shinya Kawanaka
Comment 3 2012-02-23 22:57:03 PST
Created attachment 128657 [details] WIP
WebKit Review Bot
Comment 4 2012-02-23 23:34:16 PST
Comment on attachment 128657 [details] WIP Attachment 128657 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11603713
Early Warning System Bot
Comment 5 2012-02-23 23:42:51 PST
Comment on attachment 128657 [details] WIP Attachment 128657 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11576739
Philippe Normand
Comment 6 2012-02-23 23:49:07 PST
Comment on attachment 128657 [details] WIP Attachment 128657 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11608723
WebKit Review Bot
Comment 7 2012-02-24 00:16:53 PST
Comment on attachment 128657 [details] WIP Attachment 128657 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11574752
Gyuyoung Kim
Comment 8 2012-02-24 00:40:31 PST
Comment on attachment 128657 [details] WIP Attachment 128657 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11599720
Shinya Kawanaka
Comment 9 2012-02-28 22:56:17 PST
Created attachment 129394 [details] Patch
Hajime Morrita
Comment 10 2012-02-28 23:14:01 PST
Comment on attachment 129394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129394&action=review > Source/WebCore/ChangeLog:19 > + Please split this up to these three changes if possible. This patch is simply to big. You can use this bug as a metabug. > Source/WebCore/html/shadow/HTMLShadowElement.h:56 > + ShadowRoot* distributedShadowRoot; Please follow the naming guideline.
Shinya Kawanaka
Comment 11 2012-02-29 01:12:28 PST
Let me try to split the patch.
Shinya Kawanaka
Comment 12 2012-03-01 00:43:57 PST
Created attachment 129654 [details] WIP
Shinya Kawanaka
Comment 13 2012-03-01 01:02:54 PST
Created attachment 129658 [details] WIP
Shinya Kawanaka
Comment 14 2012-03-01 01:15:51 PST
Created attachment 129661 [details] Patch
Hajime Morrita
Comment 15 2012-03-01 01:25:46 PST
Comment on attachment 129661 [details] Patch - I don't think we need to hide HTMLShadowElement behind the flag it can be like as HTMLContentElement. - Pelase split firstRendererOf() and lastRendererOf() refactoring in separate patch. Having it in the same change makes it really hard to see the actual change.
Dimitri Glazkov (Google)
Comment 16 2012-03-01 09:26:10 PST
(In reply to comment #15) > (From update of attachment 129661 [details]) > - I don't think we need to hide HTMLShadowElement behind the flag it can be like as HTMLContentElement. I think we do need this behind flag. Otherwise, there are visible effects of the Shadow DOM implementation even when the flag is enabled. See bug 79935 for details. > - Pelase split firstRendererOf() and lastRendererOf() refactoring in separate patch. > Having it in the same change makes it really hard to see the actual change.
Hajime Morrita
Comment 17 2012-03-01 22:56:59 PST
(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 129661 [details] [details]) > > - I don't think we need to hide HTMLShadowElement behind the flag it can be like as HTMLContentElement. > > I think we do need this behind flag. Otherwise, there are visible effects of the Shadow DOM implementation even when the flag is enabled. See bug 79935 for details. > > > - Pelase split firstRendererOf() and lastRendererOf() refactoring in separate patch. > > Having it in the same change makes it really hard to see the actual change. Let me clarify, I think: - It's OK to have HTMLShadowElement.cpp/h files for regardless of ENABLE(SHADOW_DOM). - It's NOT ok to have HTMLShadowElement.idl compiled in unless we ENABLE(SHADOW_DOM). - For tag name, I think we need to fix Bug 79935 WITHOUT hiding HTMLContentElement.h/cpp behind ENABLE(SHADOW_DOM). It should be hidden by runtime flag, not compilation flag. I guess we need parser support here. Let me investigate this. If we can pull up most of the code from HTMLContentElement to InsertionPoint, and can implement built-in shadows using only InsertionPoint without using HTMLContentElement. That would be best.
Shinya Kawanaka
Comment 18 2012-03-05 00:44:41 PST
Created attachment 130067 [details] WIP
Build Bot
Comment 19 2012-03-05 01:00:33 PST
Comment on attachment 130067 [details] WIP Attachment 130067 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11803879
Build Bot
Comment 20 2012-03-05 01:02:17 PST
Comment on attachment 130067 [details] WIP Attachment 130067 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11801839
Early Warning System Bot
Comment 21 2012-03-05 01:08:52 PST
Comment on attachment 130067 [details] WIP Attachment 130067 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11802866
WebKit Review Bot
Comment 22 2012-03-05 01:17:21 PST
Comment on attachment 130067 [details] WIP Attachment 130067 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11801847
Gyuyoung Kim
Comment 23 2012-03-05 01:34:21 PST
Comment on attachment 130067 [details] WIP Attachment 130067 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11802879
Gustavo Noronha (kov)
Comment 24 2012-03-05 02:33:13 PST
Comment on attachment 130067 [details] WIP Attachment 130067 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11809291
Shinya Kawanaka
Comment 25 2012-03-06 23:11:54 PST
Created attachment 130550 [details] Patch
Shinya Kawanaka
Comment 26 2012-03-06 23:23:14 PST
Created attachment 130553 [details] Patch
Hajime Morrita
Comment 27 2012-03-06 23:58:36 PST
Comment on attachment 130553 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130553&action=review > Source/WebCore/html/shadow/HTMLShadowElement.cpp:64 > + if (ShadowRoot* root = toShadowRoot(shadowTreeRootNode())) Why not treeScope()? shadowTreeRootNode() traverses down the tree and better be avoided if possible. > Source/WebCore/html/shadow/InsertionPoint.cpp:52 > if (ShadowRoot* root = toShadowRoot(shadowTreeRootNode())) { Ditto. > Source/WebCore/html/shadow/InsertionPoint.h:71 > + ShadowRoot* shadowRootAssignedFrom; I don't think we need this. > Source/WebCore/html/shadow/InsertionPoint.h:76 > if (!node || node->isContentElement()) It looks Node::isContentElement() is no longer need and can be morphed to isInsertionPoint(). It also even don't need to stay in Node. can be pulled down to HTMLElement.
Shinya Kawanaka
Comment 28 2012-03-07 00:46:27 PST
> > Source/WebCore/html/shadow/InsertionPoint.h:76 > > if (!node || node->isContentElement()) > > It looks Node::isContentElement() is no longer need and can be morphed to isInsertionPoint(). > It also even don't need to stay in Node. can be pulled down to HTMLElement. Let's do this in another bug? I don't think it's good idea to merge this refactoring into this patch...
Shinya Kawanaka
Comment 29 2012-03-07 00:52:11 PST
(In reply to comment #28) > > > Source/WebCore/html/shadow/InsertionPoint.h:76 > > > if (!node || node->isContentElement()) > > > > It looks Node::isContentElement() is no longer need and can be morphed to isInsertionPoint(). > > It also even don't need to stay in Node. can be pulled down to HTMLElement. > > Let's do this in another bug? > I don't think it's good idea to merge this refactoring into this patch... And I want to remove isShadowElement from Node.
Shinya Kawanaka
Comment 30 2012-03-07 00:55:53 PST
Created attachment 130561 [details] Patch
Shinya Kawanaka
Comment 31 2012-03-08 00:53:15 PST
Created attachment 130791 [details] Patch
Shinya Kawanaka
Comment 32 2012-03-08 00:55:16 PST
(In reply to comment #27) > (From update of attachment 130553 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130553&action=review > > > Source/WebCore/html/shadow/HTMLShadowElement.cpp:64 > > + if (ShadowRoot* root = toShadowRoot(shadowTreeRootNode())) > > Why not treeScope()? shadowTreeRootNode() traverses down the tree and better be avoided if possible. Done. > > > Source/WebCore/html/shadow/InsertionPoint.cpp:52 > > if (ShadowRoot* root = toShadowRoot(shadowTreeRootNode())) { > > Ditto. Done. > > > Source/WebCore/html/shadow/InsertionPoint.h:71 > > + ShadowRoot* shadowRootAssignedFrom; > > I don't think we need this. Done. > > > Source/WebCore/html/shadow/InsertionPoint.h:76 > > if (!node || node->isContentElement()) > > It looks Node::isContentElement() is no longer need and can be morphed to isInsertionPoint(). > It also even don't need to stay in Node. can be pulled down to HTMLElement. Already fixed in another patch
Hajime Morrita
Comment 33 2012-03-08 02:02:31 PST
Comment on attachment 130791 [details] Patch Looks good. Let's land this once bots become green.
Shinya Kawanaka
Comment 34 2012-03-08 03:25:54 PST
Committed r110161: <http://trac.webkit.org/changeset/110161>
Note You need to log in before you can comment on or make changes to this bug.