Bug 93113 - Move DOM operations in the element stack to HTMLConstructionSite.
Summary: Move DOM operations in the element stack 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-03 07:11 PDT by Kwang Yul Seo
Modified: 2012-08-08 02:23 PDT (History)
2 users (show)

See Also:


Attachments
Patch (6.87 KB, patch)
2012-08-03 07:14 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-03 07:11:53 PDT
HTMLConstructionSite has DOM operations methods required for tree construction. But some DOM operations are still performed directly in the element stack. Extracted such DOM operations and moved them to HTMLConstructionSite for consistency.

See also Bug 93070.
Comment 1 Kwang Yul Seo 2012-08-03 07:14:48 PDT
Created attachment 156367 [details]
Patch
Comment 2 Kwang Yul Seo 2012-08-03 07:16:00 PDT
This patch is also a prerequisite for speculative parsing.
Comment 3 Kwang Yul Seo 2012-08-03 10:16:44 PDT
Now HTMLElementStack has a pointer to its owner HTMLConstructionSite. I couldn't come up with a better refactoring. Any idea?
Comment 4 Eric Seidel (no email) 2012-08-07 15:30:15 PDT
Perhaps HTMLConstructionSite and ElementStack shoudl share some sort of DOMOperations abstraction which they indirect through to make dom modifications.  I assume this is done in preparation for making parsing possible on another thread?
Comment 5 Kwang Yul Seo 2012-08-07 16:22:06 PDT
(In reply to comment #4)
> Perhaps HTMLConstructionSite and ElementStack shoudl share some sort of DOMOperations abstraction which they indirect through to make dom modifications.  I assume this is done in preparation for making parsing possible on another thread?

It's a good idea. But I chose to move DOM operations to HTMLConstructionSite and also call them construction site tasks in my current implementation of speculative parsing ( https://github.com/kseo/webkit/tree/speculativeparser ) because I need to put beginParsingChildren and finishParsingChildren in the task queue. While speculating, we can't immediately call beginParsingChildren or finishParsingChildren on element because element creation itself is deferred.

I identified the list of DOM creations/mutations in the parser:

HTMLTreeBuilder
    - addChildAndAttach
    - detachNode
    - moveAllChildrenToNewParent
    - setAttribute
    - removeAllChildren

HTMLElementStack
    - beginParsingChildren
    - finishParsingChildren

HTMLConstructionSite
    - dispatchDocumentElementAvailableIfNeeded
    - mergeAttributesFromTokenIntoElement
    - setCompatibilityMode
    - setCompatibilityModeFromDoctype
    - setDemoted
    - insertedByParser
    
    - insert
    - insertOnDocument
    - insertFosterParentedChild
    - insertComment
    - insertCommentOnDocument
    - insertDoctype
    - insertText
    - insertFosterParentedText

    - createElement
    - createHTMLElement
    - createHTMLHtmlElement
    - createHTMLScriptElement
Comment 6 Eric Seidel (no email) 2012-08-07 16:34:29 PDT
I think you really want to end up with something like GraphicsContext here.

GraphicsContext exists as an (per-platform) abstraction around the drawing context.  We pass it around.  And it's normally a compile-time defined implementation, but some ports have (in the past) implemented it as a run-time defined implementation.

Similarly, it seems like this off-thread parser wants an abstraction through which to talk to the DOM (and possibly a separate one for the reading... although it sounds like you already have the reading part done).

I think this abstraction wants to be separate from HTMLConstructionSite, and used by HTMLConstructionSite.

Having it be its own separate object, will allow us to separate the mechanics of cross-thread DOM mutation (or queuing of DOM operations) from the logic of what we want to do with the DOM.

HTMLConstructionSite remains the HTML parser's "what we want to do with the DOM" logic, and some new class (perhaps DOMModifier or DOMOperations or whatever) is the way that we talk to the DOM.

When we're on the main thread, we use a dumb DOMModifier implementation which just calls the DOM operations synchronously.

When we're off the main thread, we use a smarter implementation which allocates some sort of objects (similar to how the undo stack, and Editor and EditingCommand work) which are queued up for later execution on the main thread.

The code then becomes mostly ignorant of the question of if it's on the main thread or not, and this DOMModifications abstraction handles all the details.

I'm not sure if complete ignorance of threaded-ness in HTMLConstructionSite, etc is possible, but were I implementing this, I might start with such as the goal.

Hopefully my musings above are helpful.
Comment 7 Kwang Yul Seo 2012-08-07 17:15:02 PDT
(In reply to comment #6)
> I think this abstraction wants to be separate from HTMLConstructionSite, and used by HTMLConstructionSite.
> 
> Having it be its own separate object, will allow us to separate the mechanics of cross-thread DOM mutation (or queuing of DOM operations) from the logic of what we want to do with the DOM.

Sounds reasonable!
 
> HTMLConstructionSite remains the HTML parser's "what we want to do with the DOM" logic, and some new class (perhaps DOMModifier or DOMOperations or whatever) is the way that we talk to the DOM.
> 
> When we're on the main thread, we use a dumb DOMModifier implementation which just calls the DOM operations synchronously.
> 
> When we're off the main thread, we use a smarter implementation which allocates some sort of objects (similar to how the undo stack, and Editor and EditingCommand work) which are queued up for later execution on the main thread.
>
> The code then becomes mostly ignorant of the question of if it's on the main thread or not, and this DOMModifications abstraction handles all the details.

Yes, it's a great idea. My current implementation has m_isSpeculating flag in HTMLConstructionSite. When this flag is true, HTMLConstructionSite just calls the DOM operations synchronously. Otherwise, it queues up for later execution. This certainly works, but makes the code very complicated. You suggestion sounds much better.

> I'm not sure if complete ignorance of threaded-ness in HTMLConstructionSite, etc is possible, but were I implementing this, I might start with such as the goal.

Okay. I will try! BTW, it's not just about threaded-ness because my first speculative parsing implementation is still on the main thread. But it still needs to queue up DOM operations because element creation and DOM mutations have visible side effects.

> Hopefully my musings above are helpful.

Thanks for your good advice. It is really helpful.
Comment 8 Kwang Yul Seo 2012-08-07 17:17:03 PDT
(In reply to comment #7)
> Yes, it's a great idea. My current implementation has m_isSpeculating flag in HTMLConstructionSite. When this flag is true, HTMLConstructionSite just calls the DOM operations synchronously. Otherwise, it queues up for later execution. This certainly works, but makes the code very complicated. You suggestion sounds much better.

Oops. "When this flag is true" -> "When this flag is false"
Comment 9 Kwang Yul Seo 2012-08-07 17:41:54 PDT
(In reply to comment #6)
> Similarly, it seems like this off-thread parser wants an abstraction through which to talk to the DOM (and possibly a separate one for the reading... although it sounds like you already have the reading part done).

For the reading part, I think Bug 92057 is the right solution because we can't queue up DOM read operations.

So we need only DOMModifier or DOMModifcations abstraction.
Comment 10 Kwang Yul Seo 2012-08-08 02:22:29 PDT
Close this bug in favor of Bug 93455.