[webkit-reviews] review granted: [Bug 176659] Finish off the FormData implementation : [Attachment 320370] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 176659: Finish off the FormData implementation
https://bugs.webkit.org/show_bug.cgi?id=176659

Attachment 320370: Patch

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




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

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

Is it OK that all these functions are inefficient if there are a huge numbers
of elements, because this is a vector and not an associative data structure?

> Source/WebCore/fileapi/Blob.cpp:82
> +Blob::Blob(const Blob& blob)

Where is this new function used? Given that it seems that a Blob is both
reference counted and immutable, I don’t understand when we need to copy one.
Nor can I spot it reading the code.

> Source/WebCore/html/DOMFormData.cpp:72
> +}
> +auto DOMFormData::get(const String& name) ->
std::optional<FormDataEntryValue>

Missing blank line.

> Source/WebCore/html/DOMFormData.cpp:126
> +    return WTF::KeyValuePair<String, FormDataEntryValue> { item.key(),
item.data() };

Should we add a makeKeyValuePair function so we don’t have to write out types
in a case like this?

> Source/WebCore/html/DOMFormData.idl:33
> +// FIXME: Should be Exposed=(Window,Worker)

Would be nice to have the comment explain what this is waiting on.

> Source/WebCore/html/FormDataList.cpp:44
> +	   auto file = File::create(blob.get(), filename.isNull() ?
ASCIILiteral("blob") : filename);
> +	   return { key, WTFMove(file) };

Do this as a one-liner? It would only be 4 characters longer than the
File::create line.

> Source/WebCore/html/FormDataList.cpp:49
> +	   auto file = File::create(downcast<File>(blob.get()), filename);
> +	   return { key, WTFMove(file) };

Ditto.

> Source/WebCore/html/FormDataList.cpp:71
> +template<typename EntryMaker>
> +void FormDataList::set(const String& key, const EntryMaker& entryMaker)

Looking at the logic below, it seems that EntryMaker is always called, and
there is no ordering issue due to side effects that I can see. Thus, I think
this function could take an Item&& instead and would not need to be a template.

> Source/WebCore/html/FormDataList.cpp:73
> +    size_t initialMatchLocation = WTF::notFound;

I suggest we use std::optional for clarity instead of WTF::notFound.

> Source/WebCore/html/FormDataList.cpp:79
> +	       m_items[i] = entryMaker();

This would be easier to read if the loop was kept as a pure "findKey"
operation.

Could move this one line of code into the if statement below instead of putting
it inside the loop. I can’t think of a good reason to have some of work inside
the loop and some outside.

> Source/WebCore/html/FormDataList.h:32
>      class Item {

Seems like this should be a struct instead of a class now. Unless we think
Variant is so inconvenient to work with that we want to hide it behind
"private".

> Source/WebCore/html/FormDataList.h:66
> +    CString normalizeString(const String&) const;

Seems a little strange now that we hold a TextEncoding just so that clients can
explicitly call normalizeString. Is m_encoding used for anything else? Is there
a more elegant way to arrange things?

> Source/WebCore/html/HTMLKeygenElement.cpp:126
> +    encoded_values.appendData(name(), value);

Could save one little reference count churn if a version that took String&&
existed. Probably not worth it.

Does this fix a bug because we use the proper TextEncoding now rather than
always UTF-8? Or is this always going to be an ASCII string and so it doesn’t
matter what encoding is used?

> Source/WebCore/platform/network/FormData.cpp:209
> +			   String generatedFileName;
> +			   shouldGenerateFile =
page->chrome().client().shouldReplaceWithGeneratedFileForUpload(path,
generatedFileName);

Should come back and change the ChromeClient function to use a return value
instead of an out argument. A boolean plus a String out argument seems out of
style in our code these days. Not sure what is ideal, though. Maybe
std::optional<String>? Or even just String using null string or empty string to
mean false, but we always could have done that.

> Source/WebCore/platform/network/FormData.cpp:231
> +		   auto file = WTF::get<RefPtr<File>>(item.data());

I would have written "auto& file = *WTF::get ..." here and had the local
variable be a File& instead of a guaranteed non-null RefPtr<File>.

> Source/WebCore/platform/network/FormData.cpp:233
> +		   if (!file->path().isEmpty())
> +		       appendFile(file->path(), shouldGenerateFile);

It is a bit awkward that we have to check holds_alternative<RefPtr<File>> and
then file->path().isEmpty() again here, and then rely on the fact that the side
effect of calling shouldReplaceWithGeneratedFileForUpload is in sync with this
code. I wonder if there’s any way to tie the code above to the code down here
in a tighter way. Annoying to re-do all the same conditionals after just two
lines of unconditional code.

It’s mainly because the append functions have to be in a certain order because
they have side effects, I think.

Similar problem with the various isMultipartForm. I think there is some
improvement possible here at some point with some additional refactoring.

> Source/WebCore/platform/network/FormData.cpp:237
> +		   auto normalizedStingData =
list.normalizeString(WTF::get<String>(item.data()));

Typo here: "Sting".

> Source/WebCore/platform/network/FormData.cpp:244
> +	       auto normalizedStingData =
list.normalizeString(WTF::get<String>(item.data()));

Typo here: "Sting".

> Source/WebCore/platform/network/FormData.cpp:260
> +    auto& e = m_elements.last();

Amazed that you resisted the impulse to rename this lastElement.

> Source/WebCore/platform/network/FormData.cpp:262
>      e.m_data.grow(oldSize + size);

This math seems like it needs an overflow check or an explanation of why it
doesn’t need one.

> Source/WebCore/platform/network/FormData.cpp:275
>      }
> +    
> +    return data;

Seems a bit strange to have this one blank line in this function.

> Source/WebCore/platform/network/FormData.cpp:312
> +    for (const auto& element : m_elements) {

I would have just done "auto&"; I don’t think it adds much clarity to state
const.

> Source/WebCore/platform/network/FormData.cpp:327
> +    for (const auto& element : m_elements) {

Ditto.

> Source/WebCore/platform/network/FormData.cpp:361
> +    for (const auto& element : m_elements) {

I would have just done auto& here; it will be const either way since this is a
const member function, no need to state it explicitly.

> Source/WebCore/platform/network/FormData.cpp:370
> +    for (const auto& element : m_elements) {

Ditto.