[webkit-reviews] review denied: [Bug 182444] ServiceWorker registration should store any script fetched through importScripts : [Attachment 341771] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied youenn fablet <youennf at gmail.com>'s
request for review:
Bug 182444: ServiceWorker registration should store any script fetched through
importScripts
https://bugs.webkit.org/show_bug.cgi?id=182444

Attachment 341771: Patch

https://bugs.webkit.org/attachment.cgi?id=341771&action=review




--- Comment #8 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 341771
  --> https://bugs.webkit.org/attachment.cgi?id=341771
Patch

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

> Source/WebCore/workers/WorkerScriptLoader.cpp:57
> +    bool isServiceWorkerGlobalScope =
workerGlobalScope.isServiceWorkerGlobalScope();

is<ServiceWorkerGlobalScope>(workerGlobalScope) is the preferred way.

> Source/WebCore/workers/WorkerScriptLoader.cpp:235
> +    return !m_cachedScript.isNull() ? m_cachedScript : m_script.toString();

I am not sure how useful m_cachedScript is here. I think we could reuse
m_script and merely append() the script to it, no? Looking at the
implementation of StringBuilder, a single append(String) then toString() should
be pretty efficient.

> Source/WebCore/workers/service/ServiceWorkerContextData.cpp:35
> +    HashMap<URL, ImportedScript> map;

Could you please add a CrossThreadCopierBase specialization for HashMap to
CrossThreadCopier.h? (we already have one for Vector and HashSet). Then you can
use crossThreadCopy(scriptResourceMap) here.

> Source/WebCore/workers/service/ServiceWorkerContextData.h:46
> +	   String responseURL;

Why isn't this stored as a URL type?

> Source/WebCore/workers/service/ServiceWorkerContextData.h:145
> +    // FIXME: Add a templated version of HashMap encode/decode

What's wrong with the one in ArgumentCoders.h? We already encode / decode
HashMap, there's got to be a better way than duplicating this code here.

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:133
> +std::optional<ServiceWorkerContextData::ImportedScript>
ServiceWorkerGlobalScope::scriptResource(const URL& url) const

Cannot we return a ServiceWorkerContextData::ImportedScript* to avoid copying?

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:50
> +static const int schemaVersion = 2;

do we need to rename all the v1 methods below to v2? 

also, there is an ASSERT_NOT_REACHED() below when this is not true:
if (currentSchema == v1RecordsTableSchema() || currentSchema ==
v1RecordsTableSchemaAlternate())

Wouldn't people with an existing database hit it?

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:355
> +	   HashMap<URL, ServiceWorkerContextData::ImportedScript>
scriptResourceMap;

Where is this populated?

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:360
> +

extra blank line?