Bug 166925 – Implement InlineClassicScript

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

[Yusuke Suzuki]

Implement InlineClassicScript

[Yusuke Suzuki]

Created attachment 298578 [details]
Patch

[Yusuke Suzuki]

Created attachment 298580 [details]
Patch

[Yusuke Suzuki]

Created attachment 298581 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 298581 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        This patch is further clean up. We introduce InlineClassicScript, which is

Nit: *a* further *cleanup*.

> Source/WebCore/ChangeLog:17
> +        It is important for dynamic-import. When dynamic-import operator is called,
> +        we need to get the ScriptFetcher for the caller script SourceOrigin.

This sentence is a bit wordy.  Also, why do we need to the ScriptFetcher for the caller script SourceOrigin?
It’s not clear to me how ScriptFetcher is used in dynamic-import operator by just reading this description.

> Source/JavaScriptCore/runtime/SourceOrigin.h:35
> -    explicit SourceOrigin(const String& string)
> +    explicit SourceOrigin(const String& string, ScriptFetcher* fetcher = nullptr)

Instead of adding an extra argument to this constructor, why don’t we add an overloading constructor
that takes Ref<ScriptFetcher>&& instead so that we can avoid ref churn?

> Source/WebCore/bindings/js/CachedScriptFetcher.h:52
> +    void notifyClientFinished();

Where is this defined? I don’t think CachedScriptFetcher is used a CachedResourceClient.

> Source/WebCore/bindings/js/CachedScriptSourceProvider.h:52
> -    CachedScriptSourceProvider(CachedScript* cachedScript, JSC::SourceProviderSourceType sourceType)
> -        : SourceProvider(JSC::SourceOrigin { cachedScript->response().url() }, cachedScript->response().url(), TextPosition(), sourceType)
> +    CachedScriptSourceProvider(CachedScript* cachedScript, JSC::SourceProviderSourceType sourceType, CachedScriptFetcher* scriptFetcher)
> +        : SourceProvider(JSC::SourceOrigin { cachedScript->response().url(), scriptFetcher }, cachedScript->response().url(), TextPosition(), sourceType)

Ditto about adding an overloading constructor avoid ref churn.

> Source/WebCore/dom/InlineClassicScript.cpp:37
> +        scriptElement.element().attributeWithoutSynchronization(HTMLNames::nonceAttr),

Please store Element& in a local variable instead of calling four times.

> Source/WebCore/dom/ScriptElement.cpp:279
> -        executeClassicScript(ScriptSourceCode(scriptContent(), document.url(), position, JSC::SourceProviderSourceType::Program));
> +        executeClassicScript(ScriptSourceCode(scriptContent(), document.url(), position, JSC::SourceProviderSourceType::Program, InlineClassicScript::create(*this).ptr()));

This is will result in two calls to ref(). Once in here and again inside SourceOrigin::SourceOrigin.
Why don’t we make SourceOrigin and ScriptSourceCode take Ref<CachedScriptFetcher>&& to avoid this?

[Yusuke Suzuki]

Comment on attachment 298581 [details]
Patch

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

Thanks!

>> Source/WebCore/ChangeLog:9
>> +        This patch is further clean up. We introduce InlineClassicScript, which is
> 
> Nit: *a* further *cleanup*.

Fixed.

>> Source/WebCore/ChangeLog:17
>> +        we need to get the ScriptFetcher for the caller script SourceOrigin.
> 
> This sentence is a bit wordy.  Also, why do we need to the ScriptFetcher for the caller script SourceOrigin?
> It’s not clear to me how ScriptFetcher is used in dynamic-import operator by just reading this description.

Subsequent module loading requests need the metadata, like `crossorigin` attribute information, `nonce` information etc.
They are delivered by ScriptFetcher.

>> Source/JavaScriptCore/runtime/SourceOrigin.h:35
>> +    explicit SourceOrigin(const String& string, ScriptFetcher* fetcher = nullptr)
> 
> Instead of adding an extra argument to this constructor, why don’t we add an overloading constructor
> that takes Ref<ScriptFetcher>&& instead so that we can avoid ref churn?

Nice. Added new overloaded constructor.

>> Source/WebCore/bindings/js/CachedScriptFetcher.h:52
>> +    void notifyClientFinished();
> 
> Where is this defined? I don’t think CachedScriptFetcher is used a CachedResourceClient.

Yeah, it accidentally remains. It should be dropped.

>> Source/WebCore/bindings/js/CachedScriptSourceProvider.h:52
>> +        : SourceProvider(JSC::SourceOrigin { cachedScript->response().url(), scriptFetcher }, cachedScript->response().url(), TextPosition(), sourceType)
> 
> Ditto about adding an overloading constructor avoid ref churn.

For CachedScriptSourceProvider case, we always have CachedScriptFetcher. So We can just convert this constructor to take Ref<CachedScriptFetcher>&&.

>> Source/WebCore/dom/InlineClassicScript.cpp:37
>> +        scriptElement.element().attributeWithoutSynchronization(HTMLNames::nonceAttr),
> 
> Please store Element& in a local variable instead of calling four times.

Fixed.

>> Source/WebCore/dom/ScriptElement.cpp:279
>> +        executeClassicScript(ScriptSourceCode(scriptContent(), document.url(), position, JSC::SourceProviderSourceType::Program, InlineClassicScript::create(*this).ptr()));
> 
> This is will result in two calls to ref(). Once in here and again inside SourceOrigin::SourceOrigin.
> Why don’t we make SourceOrigin and ScriptSourceCode take Ref<CachedScriptFetcher>&& to avoid this?

We changed to do that by overloading the constructor with Ref<CachedScriptFetcher>&& parameter.

[Yusuke Suzuki]

Committed r210627: <http://trac.webkit.org/changeset/210627>

[Csaba Osztrogonác]

(In reply to comment #6)
> Committed r210627: <http://trac.webkit.org/changeset/210627>

It broke the Apple Mac build everywhere, see build.webkit.org for details.

[Yusuke Suzuki]

Committed r210630: <http://trac.webkit.org/changeset/210630>