Bug 187748 - Add SPI to defer running async script until after document load
Summary: Add SPI to defer running async script until after document load
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-17 23:27 PDT by Wenson Hsieh
Modified: 2018-07-18 10:17 PDT (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2018-07-17 23:27:53 PDT
<rdar://problem/42317378>
Comment 1 Wenson Hsieh 2018-07-17 23:52:42 PDT
Created attachment 345229 [details]
First pass
Comment 2 Tim Horton 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
Comment 3 Ryosuke Niwa 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.
Comment 4 Ryosuke Niwa 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.
Comment 5 Wenson Hsieh 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
Comment 6 Wenson Hsieh 2018-07-18 08:48:59 PDT
Created attachment 345250 [details]
Patch for EWS
Comment 7 WebKit Commit Bot 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>