[webkit-reviews] review granted: [Bug 200572] Blob should store its session ID : [Attachment 375913] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 9 08:29:57 PDT 2019


Darin Adler <darin at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 200572: Blob should store its session ID
https://bugs.webkit.org/show_bug.cgi?id=200572

Attachment 375913: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 375913
  --> https://bugs.webkit.org/attachment.cgi?id=375913
Patch

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

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:161
> +	   return Blob::create(scriptExecutionContext()->sessionID());
> +    return Blob::create(scriptExecutionContext()->sessionID(), *data,
m_private->mimeType());

What guarantees that the script execution context is not null?

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:205
> +	  
scheduleDispatchEvent(MessageEvent::create(Blob::create(scriptExecutionContext(
)->sessionID(), SharedBuffer::create(data, dataLength), emptyString()), { }));

What guarantees that the script execution context is not null?

> Source/WebCore/Modules/websockets/WebSocket.cpp:587
> +	  
dispatchEvent(MessageEvent::create(Blob::create(scriptExecutionContext()->sessi
onID(), WTFMove(binaryData), emptyString()),
SecurityOrigin::create(m_url)->toString()));

What guarantees that the script execution context is not null?

> Source/WebCore/bindings/js/JSDOMPromiseDeferred.h:141
> +    PAL::SessionID sessionID() const { return
globalObject()->scriptExecutionContext()->sessionID(); }

A little annoying that we have to add includes so this can be inlined.

What guarantees that neither the global object nor the script execution context
is null?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2098
> +	       file =
File::deserialize(jsCast<JSDOMGlobalObject*>(m_globalObject)->scriptExecutionCo
ntext()->sessionID(), filePath, URL(URL(), url->string()), type->string(),
name->string(), optionalLastModified);

What guarantees that neither the global object nor the script execution context
is null?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2864
> +	       return
getJSValue(Blob::deserialize(jsCast<JSDOMGlobalObject*>(m_globalObject)->script
ExecutionContext()->sessionID(), URL(URL(), url->string()), type->string(),
size, blobFilePathForBlobURL(url->string())).get());

What guarantees that neither the global object nor the script execution context
is null?

> Source/WebCore/fileapi/Blob.cpp:92
>      , m_size(-1)

Seems peculiar to use -1 instead of Optional or something clearer. Might want
to come back and revisit this.

> Source/WebCore/html/FileInputType.cpp:417
> +    m_fileListCreator =
FileListCreator::create(element()->document().sessionID(), paths,
shouldResolveDirectories, [this, shouldRequestIcon](Ref<FileList>&& fileList) {

Does this function ever get called when element() can be nullptr? Some of the
other functions in InputType classes seem to guard against this.

> Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp:152
> +	   ScriptExecutionContext& context = globalScope;

I don’t understand why this local variable is needed. Can’t we call
globalScope.sessionID()?

> Source/WebCore/xml/XMLHttpRequest.cpp:215
> +    return Blob::create(scriptExecutionContext()->sessionID(),
WTFMove(data), normalizedContentType);

What guarantees that script execution context is non-null?


More information about the webkit-reviews mailing list