RESOLVED WONTFIX 93070
Move DOM operations in the tree builder to HTMLConstructionSite. 
https://bugs.webkit.org/show_bug.cgi?id=93070
Summary Move DOM operations in the tree builder to HTMLConstructionSite.
Kwang Yul Seo
Reported 2012-08-02 23:44:36 PDT
HTMLConstructionSite has DOM operations methods required for tree construction. But some DOM operations are still performed directly in the tree builder. Extracted such DOM operations and moved them to HTMLConstructionSite for consistency. This change removes code duplication and improves the readability of HTMLTreeBuilder, esepcially HTMLTreeBuilder::callTheAdoptionAgency(AtomicHTMLToken*).
Attachments
Patch (8.00 KB, patch)
2012-08-02 23:48 PDT, Kwang Yul Seo 		no flags
DetailsFormatted DiffDiff
Patch (8.73 KB, patch)
2012-08-03 01:36 PDT, Kwang Yul Seo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Kwang Yul Seo
Comment 1 2012-08-02 23:48:05 PDT
Created attachment 156266 [details] Patch
Kwang Yul Seo
Comment 2 2012-08-02 23:53:05 PDT
This patch is a prerequisite for speculative parsing. Because we can't create nodes while in speculation, all DOM operations must be queued until we know if the speculation is successful or not. I want to ensure that all tree construction and DOM manipulations are performed only in HTMLConstructionSite.
Adam Barth
Comment 3 2012-08-03 00:00:39 PDT
Comment on attachment 156266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156266&action=review > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:-1612 > - newItem->element()->attach(); Can we use lazyAttach here?
Adam Barth
Comment 4 2012-08-03 00:01:30 PDT
Comment on attachment 156266 [details] Patch This is probably right, but I'm too tired to review it well right now. I'll give it a shot tomorrow.
Kwang Yul Seo
Comment 5 2012-08-03 00:10:16 PDT
(In reply to comment #3) > (From update of attachment 156266 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156266&action=review > > > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:-1612 > > - newItem->element()->attach(); > > Can we use lazyAttach here? I am not sure. I don't understand the difference between lazyAttach and attach here. I just copied the original code verbatim to make sure not to change the behavior.
Kwang Yul Seo
Comment 6 2012-08-03 00:10:37 PDT
(In reply to comment #4) > (From update of attachment 156266 [details]) > This is probably right, but I'm too tired to review it well right now. I'll give it a shot tomorrow. Okay. Thanks!
Kwang Yul Seo
Comment 7 2012-08-03 01:36:48 PDT
Created attachment 156288 [details] Patch
Kwang Yul Seo
Comment 8 2012-08-03 01:37:09 PDT
(In reply to comment #7) > Created an attachment (id=156288) [details] > Patch I missed removeAllChildren in the previous patch.
Kwang Yul Seo
Comment 9 2012-08-03 07:13:23 PDT
See also Bug 93113
Adam Barth
Comment 10 2012-08-03 10:25:14 PDT
@skyul: I wonder if we should keep these patches in your git branch for the moment. If we don't end up with speculative parsing, then we probably won't want to add this layer of indirection...
Kwang Yul Seo
Comment 11 2012-08-03 10:38:51 PDT
(In reply to comment #10) > @skyul: I wonder if we should keep these patches in your git branch for the moment. If we don't end up with speculative parsing, then we probably won't want to add this layer of indirection... Okay. I won't land any patch on speculative parsing until we reach the final decision. BTW, I though that this patch has its own benefit because it removes code duplication in the adoption agency algorithm. But I agree some new methods in HTMLConstructionSite just add a layer of indirection.
Adam Barth
Comment 12 2012-08-03 10:46:54 PDT
> BTW, I though that this patch has its own benefit because it removes code duplication in the adoption agency algorithm. But I agree some new methods in HTMLConstructionSite just add a layer of indirection. Yeah, that part of the patch is valuable independent of speculative parsing. My guess is that we can come to a decision about speculative parsing relatively quickly, so this won't cause a long delay.
Kwang Yul Seo
Comment 13 2012-08-03 10:58:39 PDT
(In reply to comment #12) > Yeah, that part of the patch is valuable independent of speculative parsing. > > My guess is that we can come to a decision about speculative parsing relatively quickly, so this won't cause a long delay. I think I can complete the initial implementation within a week. I will push my work-in-progress patches into my git branch next Monday for interim review. Thank you!
Ryosuke Niwa
Comment 14 2012-08-07 23:04:38 PDT
Comment on attachment 156288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156288&action=review > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:771 > + m_tree.detachNode(m_tree.openElements()->bodyElement()); I don't think detachNode is a good name. It reminds me of detach(), which has a completely different semantic.
Kwang Yul Seo
Comment 15 2012-08-08 00:22:38 PDT
(In reply to comment #14) > I don't think detachNode is a good name. It reminds me of detach(), which has a completely different semantic. Okay. What about removeChild? Does it sound better?
Kwang Yul Seo
Comment 16 2012-08-08 00:26:49 PDT
(In reply to comment #15) > (In reply to comment #14) > > I don't think detachNode is a good name. It reminds me of detach(), which has a completely different semantic. > > Okay. What about removeChild? Does it sound better? Oops. I was confused. It should be something like removeFromItsParent.
Kwang Yul Seo
Comment 17 2012-08-08 02:22:48 PDT
Close this bug in favor of Bug 93455.
Note You need to log in before you can comment on or make changes to this bug.