Bug 93070 - Move DOM operations in the tree builder to HTMLConstructionSite.
Summary: Move DOM operations in the tree builder to HTMLConstructionSite.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kwang Yul Seo
URL:
Keywords:
Depends on:
Blocks: 90751
  Show dependency treegraph
 
Reported: 2012-08-02 23:44 PDT by Kwang Yul Seo
Modified: 2012-08-08 02:23 PDT (History)
2 users (show)

See Also:


Attachments
Patch (8.00 KB, patch)
2012-08-02 23:48 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (8.73 KB, patch)
2012-08-03 01:36 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 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*).
Comment 1 Kwang Yul Seo 2012-08-02 23:48:05 PDT
Created attachment 156266 [details]
Patch
Comment 2 Kwang Yul Seo 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.
Comment 3 Adam Barth 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?
Comment 4 Adam Barth 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.
Comment 5 Kwang Yul Seo 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.
Comment 6 Kwang Yul Seo 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!
Comment 7 Kwang Yul Seo 2012-08-03 01:36:48 PDT
Created attachment 156288 [details]
Patch
Comment 8 Kwang Yul Seo 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.
Comment 9 Kwang Yul Seo 2012-08-03 07:13:23 PDT
See also Bug 93113
Comment 10 Adam Barth 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...
Comment 11 Kwang Yul Seo 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.
Comment 12 Adam Barth 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.
Comment 13 Kwang Yul Seo 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!
Comment 14 Ryosuke Niwa 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.
Comment 15 Kwang Yul Seo 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?
Comment 16 Kwang Yul Seo 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.
Comment 17 Kwang Yul Seo 2012-08-08 02:22:48 PDT
Close this bug in favor of Bug 93455.