Bug 187748

Summary:

Add SPI to defer running async script until after document load

Product:

WebKit

Reporter:

Wenson Hsieh <wenson_hsieh>

Component:

WebKit Misc.

Assignee:

Wenson Hsieh <wenson_hsieh>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, ggaren, kangil.han, rniwa, thorton, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
First pass	rniwa: review+
Patch for EWS	none

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+	Details Formatted Diff Diff
Patch for EWS (22.62 KB, patch) 2018-07-18 08:48 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
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.