Bug 161674 – Introduce abstract class LoadableScript for classic script and module graph

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Introduce abstract class LoadableScript for classic script and module graph

[Yusuke Suzuki]

Created attachment 288095 [details]
Patch

WIP

[WebKit Commit Bot]

Attachment 288095 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/LoadableClassicScript.cpp:81:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/dom/LoadableClassicScript.cpp:93:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 288121 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 288193 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 288193 [details]
Patch

I'll introduce a little bit change.

[Yusuke Suzuki]

Created attachment 288196 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 288196 [details]
Patch

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

> Source/WebCore/dom/LoadableClassicScript.cpp:39
> +    if (!cachedScript)
> +        return nullptr;

create function typically doesn't return nullptr.
Why don't we check this in the caller instead?
That way, this function can return Ref<LoadableClassicScript> like other create functions.

> Source/WebCore/dom/LoadableClassicScript.cpp:99
> +                makeString("Refused to execute script from '", m_cachedScript->url().stringCenterEllipsizedToLength(), "' because its MIME type ('", m_cachedScript->mimeType(), "') is not executable, and strict MIME type checking is enabled.")

This line is really long.
Can we break the line somewhere?

> Source/WebCore/dom/LoadableClassicScript.h:40
> +// A CachedResourceHandle alone does not prevent the underlying CachedResource
> +// from purging its data buffer. This class holds a client until this class is
> +// destroyed in order to guarantee that the data buffer will not be purged.

I think this is the kind of a comment that can get outdated pretty fast.
I'd recommend moving it to the change log (since it's definitely correct when this patch is landed).

> Source/WebCore/dom/ScriptElement.cpp:262
> -bool ScriptElement::requestScript(const String& sourceUrl)
> +CachedResourceHandle<CachedScript> ScriptElement::requestCachedScript(const URL& sourceURL, const String& nonceAttribute, const String& crossOriginMode)

Could you change the order of requestCachedScript and requestClassicScript so that the diff looks nicer?
It's a bit strange to call this request *cachedScript* in that it may send a network request if the cache doesn't exist.
How about requestScriptWithCache or requestScriptFromCachedResourceLoader? Or loadCachedScriptResource?

> Source/WebCore/dom/ScriptElement.cpp:357
> -void ScriptElement::notifyFinished(CachedResource* resource)
> +void ScriptElement::executeQueuedScriptAndDispatchEvent(LoadableScript& loadableScript)

"queued script" seems rather vague. I'd rather call executeScriptInScriptRunner, executeScriptByScriptRunner, or executeScriptForScriptRunner.
By why is it significant that this function remove itself from loadableScript?
Maybe that'll lead to a more semantically descriptive function name as well.
Could you clarify that in the change log?

> Source/WebCore/dom/ScriptElement.cpp:364
> +void ScriptElement::executePendingScriptAndDispatchEvent(PendingScript& pendingScript)

Ditto.  It's totally clear to me that "PendingScript" means HTMLScriptRunner and QueuedScript means ScriptRunner.
I'd rather explicitly say this is executeScriptForHTMLScriptRunner.

> Source/WebCore/dom/ScriptElement.cpp:376
> +void ScriptElement::executeScript(LoadableScript& loadableScript)

Do we really need this function?  It seems like all call sites are also asserting ASSERT(!loadableScript.errorOccurred());
If we're keeping this function, perhaps it's better to call it executeScriptWithoutCheckingErrors to make the semantics clear.

> Source/WebCore/dom/ScriptRunner.cpp:116
> +        ASSERT(toScriptElementIfPossible(&script->element()));

Instead of repeating this in ASSERT and then using it, why don't we store in a local variable?

[Yusuke Suzuki]

Comment on attachment 288196 [details]
Patch

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

> Source/WebCore/ChangeLog:22
> +        (WebCore::LoadableClassicScript::errorOccurred): Beyond the boolean value, this can return the detail

And I've applied the change suggested in https://bugs.webkit.org/show_bug.cgi?id=148897. The name is changed from errorOccurred() to wasErrored().

>> Source/WebCore/dom/LoadableClassicScript.cpp:39
>> +        return nullptr;
> 
> create function typically doesn't return nullptr.
> Why don't we check this in the caller instead?
> That way, this function can return Ref<LoadableClassicScript> like other create functions.

Sounds better.
I've just moved the check to the caller side and ensure that this LoadableClassicScript::create always returns Ref and never fails.

>> Source/WebCore/dom/LoadableClassicScript.cpp:99
>> +                makeString("Refused to execute script from '", m_cachedScript->url().stringCenterEllipsizedToLength(), "' because its MIME type ('", m_cachedScript->mimeType(), "') is not executable, and strict MIME type checking is enabled.")
> 
> This line is really long.
> Can we break the line somewhere?

OK, inserted.

>> Source/WebCore/dom/LoadableClassicScript.h:40
>> +// destroyed in order to guarantee that the data buffer will not be purged.
> 
> I think this is the kind of a comment that can get outdated pretty fast.
> I'd recommend moving it to the change log (since it's definitely correct when this patch is landed).

Make sense. Moved.

>> Source/WebCore/dom/ScriptElement.cpp:262
>> +CachedResourceHandle<CachedScript> ScriptElement::requestCachedScript(const URL& sourceURL, const String& nonceAttribute, const String& crossOriginMode)
> 
> Could you change the order of requestCachedScript and requestClassicScript so that the diff looks nicer?
> It's a bit strange to call this request *cachedScript* in that it may send a network request if the cache doesn't exist.
> How about requestScriptWithCache or requestScriptFromCachedResourceLoader? Or loadCachedScriptResource?

Moved and this change makes the diff nicer.
And right. CachedScript can be retrieved through the network request. `requestScriptWithCache` seems better. Changed.

>> Source/WebCore/dom/ScriptElement.cpp:357
>> +void ScriptElement::executeQueuedScriptAndDispatchEvent(LoadableScript& loadableScript)
> 
> "queued script" seems rather vague. I'd rather call executeScriptInScriptRunner, executeScriptByScriptRunner, or executeScriptForScriptRunner.
> By why is it significant that this function remove itself from loadableScript?
> Maybe that'll lead to a more semantically descriptive function name as well.
> Could you clarify that in the change log?

It is because ScriptRunner is driven by the different way from HTMLScriptRunner.
While HTMLScriptRunner is driven by the PendingScript and HTML document parser, ScriptRunner is driven by the notification told by the ScriptElement.
As we can see in the ScriptElement::prepareScript, we call m_loadableScript->addClient(*this) only when we use the ScriptRunner.
And once the loading is done, ScriptElement receives the notification and drives the ScriptRunner by using ScriptElement::m_willExecuteInOrder information (defer / async).

But right, it is tricky. I think there is a chance to refactor ScriptRunner by using PendingScript::{setClient,clearClient}.
I'll handle this in the separate patch. https://bugs.webkit.org/show_bug.cgi?id=161726

>> Source/WebCore/dom/ScriptElement.cpp:364
>> +void ScriptElement::executePendingScriptAndDispatchEvent(PendingScript& pendingScript)
> 
> Ditto.  It's totally clear to me that "PendingScript" means HTMLScriptRunner and QueuedScript means ScriptRunner.
> I'd rather explicitly say this is executeScriptForHTMLScriptRunner.

Reasonable. Changed it to executeScriptForHTMLScriptRunner.

>> Source/WebCore/dom/ScriptElement.cpp:376
>> +void ScriptElement::executeScript(LoadableScript& loadableScript)
> 
> Do we really need this function?  It seems like all call sites are also asserting ASSERT(!loadableScript.errorOccurred());
> If we're keeping this function, perhaps it's better to call it executeScriptWithoutCheckingErrors to make the semantics clear.

This is defined just for the correspondence between `executeScript(const ScriptSourceCode&)` and `executeScript(LoadableScript&)`.
Since the current caller is only one (and there is no plan adding a new caller for this function), merging this into executeScriptAndDispatchEvent is OK.

>> Source/WebCore/dom/ScriptRunner.cpp:116
>> +        ASSERT(toScriptElementIfPossible(&script->element()));
> 
> Instead of repeating this in ASSERT and then using it, why don't we store in a local variable?

It is because I don't want to create the pointer local variable for the value that should not be nullptr.
But either is OK. I reverted it to the original one.

[Yusuke Suzuki]

Committed r205581: <http://trac.webkit.org/changeset/205581>

[Yusuke Suzuki]

Committed r205583: <http://trac.webkit.org/changeset/205583>