WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
43736
Refactor HTMLScriptRunner to allow deferred scripts to share code
https://bugs.webkit.org/show_bug.cgi?id=43736
Summary
Refactor HTMLScriptRunner to allow deferred scripts to share code
Tony Gentilcore
Reported
2010-08-09 11:48:00 PDT
Factor out a base ScriptRunner and ScriptRunnerHost
Attachments
Patch
(34.43 KB, patch)
2010-08-09 12:04 PDT
,
Tony Gentilcore
no flags
Details
Formatted Diff
Diff
Composite instead of inherit ScriptRunner
(34.18 KB, patch)
2010-08-09 15:27 PDT
,
Tony Gentilcore
no flags
Details
Formatted Diff
Diff
Patch
(9.94 KB, patch)
2010-08-11 11:33 PDT
,
Tony Gentilcore
no flags
Details
Formatted Diff
Diff
Separate nesting level from insertion point
(10.96 KB, patch)
2010-08-13 16:37 PDT
,
Tony Gentilcore
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Tony Gentilcore
Comment 1
2010-08-09 12:04:05 PDT
Created
attachment 63914
[details]
Patch
Tony Gentilcore
Comment 2
2010-08-09 12:05:30 PDT
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)
Comment 3
2010-08-09 13:53:59 PDT
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
Comment 4
2010-08-09 14:14:36 PDT
(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
Comment 5
2010-08-09 15:27:19 PDT
Created
attachment 63934
[details]
Composite instead of inherit ScriptRunner
Tony Gentilcore
Comment 6
2010-08-09 20:12:19 PDT
> > 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
Comment 7
2010-08-11 11:33:30 PDT
Created
attachment 64138
[details]
Patch
Eric Seidel (no email)
Comment 8
2010-08-11 11:41:39 PDT
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
Comment 9
2010-08-11 11:51:12 PDT
(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
Comment 10
2010-08-12 16:04:12 PDT
(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
Comment 11
2010-08-13 16:37:31 PDT
Created
attachment 64388
[details]
Separate nesting level from insertion point
Tony Gentilcore
Comment 12
2010-08-13 16:54:55 PDT
(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 13
2010-08-13 18:57:59 PDT
Comment on
attachment 64388
[details]
Separate nesting level from insertion point Yay! Thank you.
WebKit Commit Bot
Comment 14
2010-08-13 20:10:01 PDT
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
Comment 15
2010-08-13 20:10:07 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug