Bug 43736 – Refactor HTMLScriptRunner to allow deferred scripts to share code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Gentilcore]

Factor out a base ScriptRunner and ScriptRunnerHost

[Tony Gentilcore]

Created attachment 63914 [details]
Patch

[Tony Gentilcore]

Since everything in ScriptRunner operators on a PendingScript, an alternate approach would be to make these members of PendingScript. But I kind of like keeping PendingScript a POD. Let me know if you think otherwise.

[Eric Seidel (no email)]

I'm not sure I understand the problem we're trying to solve here. AsyncScriptRunner (the factored bit out of Document) needs to deal with PendingScripts and share code with the parser-driven HTMLScriptRunner?

Should this be done by composition instead of inheritance?  Should HTMLScriptRunner become ParserScriptRunner?

I'm just not sure I understand.  Maybe you can brief me over IRC or clarify more what you're going after in the bug.

[Tony Gentilcore]

(In reply to comment #3)
> I'm not sure I understand the problem we're trying to solve here. AsyncScriptRunner (the factored bit out of Document) needs to deal with PendingScripts and share code with the parser-driven HTMLScriptRunner?
> 
> Should this be done by composition instead of inheritance?

Oh yeah, you are right. I'll change that after we are straight on everything else.

>  Should HTMLScriptRunner become ParserScriptRunner?

I don't think so. HTMLScriptRunner deals with HTMLInputStream and other HTML specific things. It also assumes things like the src attribute contains the URL (as opposed to the href attribute). In any case, that would be orthogonal to this I think.

> 
> I'm just not sure I understand.  Maybe you can brief me over IRC or clarify more what you're going after in the bug.

My plan is that there will be 3 ScriptRunners (DeferredScriptRunner, AsyncScriptRunner, and HTMLScriptRunner).

1. HTMLScriptRunner. The only modifications I have planned after this patch are to call document()->asyncScriptRunner->runScript() and document()->deferredScriptRunner()->runScript() in the appropriate place in runScript().

2. DeferredScriptRunner. Originally I was going to add a Deque<PendingScript> to HTMLScriptRunner and copy the first PendingScript from the Deque into m_parsingBlockingScript. However, Adam discovered that I can't really use that code path because NestingLevel reopens the insertion point, when in fact deferred scripts should not have an insertion point (doc.writes are dropped on the floor). My new plan is to have the Document own a DeferredScriptRunner in the same way it owns an AsyncScriptRunner. DeferredScriptRunner will have a trivial implementation that utilizes all of the methods in ScriptRunner for requesting and executing its Deque of PendingScripts. That is mostly coded and I can show you the WIP if that helps.

3. AsyncScriptRunner. Currently ScriptElementData duplicates a lot of the functionality that I've factored out to ScriptRunner here. Factoring out the code I've moved into ScriptRunner will allow AsyncScriptRunner to use the ScriptRunner functionality and it will allow ScriptElement to just use AsyncScriptRunner instead of duplicating the code.

[Tony Gentilcore]

Created attachment 63934 [details]
Composite instead of inherit ScriptRunner

[Tony Gentilcore]

> > Should this be done by composition instead of inheritance?
> 
> Oh yeah, you are right. I'll change that after we are straight on everything else.

I went ahead and did this.

> DeferredScriptRunner will have a trivial implementation that utilizes all of the methods in ScriptRunner for requesting and executing its Deque of PendingScripts. That is mostly coded and I can show you the WIP if that helps.
This patch shows this refactor + the new DeferredScriptRunner. Hopefully it makes the usefulness of this refactor clear.
https://bugs.webkit.org/attachment.cgi?id=63968&action=prettypatch

[Tony Gentilcore]

Created attachment 64138 [details]
Patch

[Eric Seidel (no email)]

Don't you have to split out the code which increments the scripting level too?  It seems you wnat to split making an insertion point away from incrementing the scripting nesting level.

[Tony Gentilcore]

(In reply to comment #8)
> Don't you have to split out the code which increments the scripting level too?  It seems you wnat to split making an insertion point away from incrementing the scripting nesting level.

I think so, but I'm not quite sure how that will look yet. Do you think that should be added to this patch or is it reasonable to do separately? My thought is that it can be separated.

[Tony Gentilcore]

(In reply to comment #9)
> (In reply to comment #8)
> > Don't you have to split out the code which increments the scripting level too?  It seems you wnat to split making an insertion point away from incrementing the scripting nesting level.
> 
> I think so, but I'm not quite sure how that will look yet. Do you think that should be added to this patch or is it reasonable to do separately? My thought is that it can be separated.

In any case, I think this is a positive refactor and a step in the right direction. Is it okay to check in as an incremental step?

[Tony Gentilcore]

Created attachment 64388 [details]
Separate nesting level from insertion point

[Tony Gentilcore]

(In reply to comment #8)
> Don't you have to split out the code which increments the scripting level too?  It seems you wnat to split making an insertion point away from incrementing the scripting nesting level.

Yep, after I got deferred on top of this again, I see what you mean. The patch is updated to split that out now.

[Eric Seidel (no email)]

Comment on attachment 64388 [details]
Separate nesting level from insertion point

Yay!  Thank you.

[WebKit Commit Bot]

Comment on attachment 64388 [details]
Separate nesting level from insertion point

Clearing flags on attachment: 64388

Committed r65350: <http://trac.webkit.org/changeset/65350>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.