[webkit-reviews] review granted: [Bug 187461] Fetch using FormData with file doesn't go through Service Worker : [Attachment 448282] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 187461: Fetch using FormData with file doesn't go through Service Worker
https://bugs.webkit.org/show_bug.cgi?id=187461

Attachment 448282: Patch

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




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

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

r=me with nits.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:278
> +	   if (!value.size()) {

could probably use `value.empty()`

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:284
> +	   builder.append(value.data(), value.size());

A bit disappointing that FragmentedSharedBuffer doesn't have an append()
overload that takes a Span<>. We should probably add one.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:306
> +	   if (!value.size()) {

could be `value.empty()`

> Source/WebCore/Modules/fetch/FormDataConsumer.cpp:59
> +    WTF::switchOn(m_formData->elements()[m_currentElementIndex++].data,
[this](const Vector<uint8_t>& content) {

May not need WTF::

> Source/WebCore/Modules/fetch/FormDataConsumer.cpp:70
> +    consume(Span<const uint8_t> { content.data(), content.size() });

Can't you simply call `content.span()` ?

> Source/WebCore/Modules/fetch/FormDataConsumer.cpp:121
> +    m_callback(Span<const uint8_t> { content.data(), content.size() });

Source and destination are both a Span<const uint8_t>, why aren't we passing
content directly instead of reconstructing a span? Am I missing something?