Bug 166925 - Implement InlineClassicScript
Summary: Implement InlineClassicScript
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on: 166888
Blocks: 150358 166926
  Show dependency treegraph
 
Reported: 2017-01-11 06:48 PST by Yusuke Suzuki
Modified: 2017-01-12 03:50 PST (History)
4 users (show)

See Also:


Attachments
Patch (42.88 KB, patch)
2017-01-11 06:54 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (52.11 KB, patch)
2017-01-11 07:13 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (49.86 KB, patch)
2017-01-11 07:13 PST, Yusuke Suzuki
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-01-11 06:48:58 PST
Implement InlineClassicScript
Comment 1 Yusuke Suzuki 2017-01-11 06:54:37 PST
Created attachment 298578 [details]
Patch
Comment 2 Yusuke Suzuki 2017-01-11 07:13:03 PST
Created attachment 298580 [details]
Patch
Comment 3 Yusuke Suzuki 2017-01-11 07:13:56 PST
Created attachment 298581 [details]
Patch
Comment 4 Ryosuke Niwa 2017-01-12 00:15:23 PST
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?
Comment 5 Yusuke Suzuki 2017-01-12 00:58:51 PST
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.
Comment 6 Yusuke Suzuki 2017-01-12 01:02:49 PST
Committed r210627: <http://trac.webkit.org/changeset/210627>
Comment 7 Csaba Osztrogonác 2017-01-12 03:47:13 PST
(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.
Comment 8 Yusuke Suzuki 2017-01-12 03:50:15 PST
Committed r210630: <http://trac.webkit.org/changeset/210630>