RESOLVED FIXED 187748
Add SPI to defer running async script until after document load 
https://bugs.webkit.org/show_bug.cgi?id=187748
Summary Add SPI to defer running async script until after document load
Wenson Hsieh
Reported 2018-07-17 23:27:53 PDT
<rdar://problem/42317378>
Attachments
First pass (24.12 KB, patch)
2018-07-17 23:52 PDT, Wenson Hsieh 		rniwa: review+
DetailsFormatted DiffDiff
Patch for EWS (22.62 KB, patch)
2018-07-18 08:48 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Wenson Hsieh
Comment 1 2018-07-17 23:52:42 PDT
Created attachment 345229 [details] First pass
Tim Horton
Comment 2 2018-07-18 00:00:46 PDT
Comment on attachment 345229 [details] First pass View in context: https://bugs.webkit.org/attachment.cgi?id=345229&action=review Seems reasonable but I’d like a rniwa or someone like that review. > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:76 > +@property (nonatomic, setter=_setShouldRunAsynchronousScriptsAfterDocumentLoad:) BOOL _shouldRunAsynchronousScriptsAfterDocumentLoad WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); maybe shouldDeferAsynchronousScriptsUntilAfterDocumentLoad? Or maybe your name is ok. something is a little weird
Ryosuke Niwa
Comment 3 2018-07-18 00:14:46 PDT
Comment on attachment 345229 [details] First pass View in context: https://bugs.webkit.org/attachment.cgi?id=345229&action=review > Source/WebCore/dom/ScriptRunner.cpp:109 > + if (m_document.shouldDeferAsynchronousScriptsUntilParsingFinishes()) > + m_scriptsToExecuteAfterDocumentFinishesParsing.append(pendingAsyncScript); > + else > + m_scriptsToExecuteSoon.append(pendingAsyncScript); I don't think it makes sense to have a separate vector for this since m_scriptsToExecuteAfterDocumentFinishesParsing and m_scriptsToExecuteSoon should never have entires at the same time. Instead, ScriptRunner::timerFired() should check m_document.shouldDeferAsynchronousScriptsUntilParsingFinishes() and avoid executing scripts in m_scriptsToExecuteSoon.
Ryosuke Niwa
Comment 4 2018-07-18 00:15:51 PDT
Comment on attachment 345229 [details] First pass View in context: https://bugs.webkit.org/attachment.cgi?id=345229&action=review > Tools/TestWebKitAPI/Tests/WebKitCocoa/RunScriptAfterDocumentLoad.mm:39 > + "<head><script src='async.js' async></script></head>" Make sure load event on this script element isn't fired until the parsing finishes as well.
Wenson Hsieh
Comment 5 2018-07-18 08:39:48 PDT
Comment on attachment 345229 [details] First pass View in context: https://bugs.webkit.org/attachment.cgi?id=345229&action=review >> Source/WebCore/dom/ScriptRunner.cpp:109 >> + m_scriptsToExecuteSoon.append(pendingAsyncScript); > > I don't think it makes sense to have a separate vector for this since > m_scriptsToExecuteAfterDocumentFinishesParsing and m_scriptsToExecuteSoon > should never have entires at the same time. > > Instead, ScriptRunner::timerFired() should check m_document.shouldDeferAsynchronousScriptsUntilParsingFinishes() > and avoid executing scripts in m_scriptsToExecuteSoon. Good idea. This makes the patch even simpler, as well! >> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:76 >> +@property (nonatomic, setter=_setShouldRunAsynchronousScriptsAfterDocumentLoad:) BOOL _shouldRunAsynchronousScriptsAfterDocumentLoad WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); > > maybe shouldDeferAsynchronousScriptsUntilAfterDocumentLoad? Or maybe your name is ok. something is a little weird I like shouldDeferAsynchronousScriptsUntilAfterDocumentLoad better too! Renamed. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/RunScriptAfterDocumentLoad.mm:39 >> + "<head><script src='async.js' async></script></head>" > > Make sure load event on this script element isn't fired until the parsing finishes as well. 👍 Added this to my test
Wenson Hsieh
Comment 6 2018-07-18 08:48:59 PDT
Created attachment 345250 [details] Patch for EWS
WebKit Commit Bot
Comment 7 2018-07-18 10:08:53 PDT
Comment on attachment 345250 [details] Patch for EWS Clearing flags on attachment: 345250 Committed r233915: <https://trac.webkit.org/changeset/233915>
Note You need to log in before you can comment on or make changes to this bug.