Bug 182695 - Use child text content when determining whether to bail early in running a script
Summary: Use child text content when determining whether to bail early in running a sc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-12 08:51 PST by Domenic Denicola
Modified: 2020-05-25 07:48 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.39 KB, patch)
2020-05-22 08:47 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (5.36 KB, patch)
2020-05-25 04:28 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Domenic Denicola 2018-02-12 08:51:34 PST
Test: http://w3c-test.org/html/semantics/scripting-1/the-script-element/emptyish-script-elements.html

Spec: https://html.spec.whatwg.org/#prepare-a-script step 5

Recent spec change: https://github.com/whatwg/html/pull/3451 (to match Firefox and Edge and be simpler)

The observable difference here is due to whether or not you mark the script as "already-started", preventing future executions when its child text content changes.
Comment 1 Rob Buis 2020-05-22 08:47:12 PDT
Created attachment 400052 [details]
Patch
Comment 2 youenn fablet 2020-05-25 01:00:22 PDT
Comment on attachment 400052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400052&action=review

> Source/WebCore/dom/ScriptElement.cpp:184
> +    const String& sourceText = scriptContent();

s/const String&/auto, scriptContent() is returning a String.
If we need to optimise this a bit, maybe we should add a TextNodeTraversal::isEmpty routine so that we create the string only when executing the script.
Comment 3 Rob Buis 2020-05-25 04:28:18 PDT
Created attachment 400193 [details]
Patch
Comment 4 Rob Buis 2020-05-25 07:42:06 PDT
Comment on attachment 400052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400052&action=review

>> Source/WebCore/dom/ScriptElement.cpp:184
>> +    const String& sourceText = scriptContent();
> 
> s/const String&/auto, scriptContent() is returning a String.
> If we need to optimise this a bit, maybe we should add a TextNodeTraversal::isEmpty routine so that we create the string only when executing the script.

Done. I think it is okay since the majority of the time the script should have non empty content. Also I checked chromium code and they also just get the contents and then test for empty.
Comment 5 EWS 2020-05-25 07:47:56 PDT
Committed r262125: <https://trac.webkit.org/changeset/262125>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400193 [details].
Comment 6 Radar WebKit Bug Importer 2020-05-25 07:48:19 PDT
<rdar://problem/63597671>