[webkit-reviews] review granted: [Bug 200102] Move FormData zip file generation to NetworkProcess and enable it for all WebKit clients for uploading directories : [Attachment 375253] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 200102: Move FormData zip file generation to NetworkProcess and enable it
for all WebKit clients for uploading directories
https://bugs.webkit.org/show_bug.cgi?id=200102

Attachment 375253: Patch

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




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

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

Looks great to me. Not sure how well we cover test cases; I have a few ideas
about refinements.

> Source/WebCore/platform/network/FormData.cpp:368
> +	       if
(fileModificationTime->secondsSinceEpoch().secondsAs<time_t>() !=
fileData->expectedFileModificationTime->secondsSinceEpoch().secondsAs<time_t>()
)

I see that this code is similar (copied/pasted?) to code we already have. But I
do have a question about it. Why is it correct to convert/round both of these
values to time_t before comparing them? Is it important to ignore insignificant
differences? I assume that on most Unix-like systems what’s actually stored in
the file system is a time_t, so if we don’t convert to a time_t there is some
risk that two matching times might compare as not equal, perhaps. But if that’s
right, then is this platform-specific in some way? Seems like we might need to
write a helper function so we can have a place to put a comment explaining the
technique.

> Source/WebCore/platform/network/FormData.cpp:379
> +    return { *this, WTFMove(generatedFiles) };

Since the filenames are all stored in the EncodedFileData, kind of seems like
we could just have a flag that says "this one was generated" and avoid having
to have a separate vector of strings.

> Source/WebCore/platform/network/FormData.h:205
> +    WEBCORE_EXPORT std::pair<Ref<FormData>, Vector<String>>
generateFilesForUpload();

We have three choices here: std::pair, std::tuple, custom struct with named
elements. Is std::pair the best of the three?

Also, I like the idea of a GeneratedFile object which holds a path name and
which deletes the file as a side effect when it’s deleted. We could keep moving
it around from place to place, but then when it’s deleted, so is the file.
Maybe too subtle? I think it makes it less likely we might forget to delete a
file in some code path, but more likely that the delete could happen on some
strange thread? If the only reason for the vector is to remember to delete the
files, then we could even make the string private.

> Source/WebCore/platform/network/FormData.h:295
> +String generateFileForUpload(const String&);

This function is a bit mysterious. I think this takes a directory and turns it
into a file. But that’s not clear from the name, nor is it clear what the
argument and return value represent.

> Source/WebCore/platform/network/mac/BlobDataFileReferenceMac.mm:31
> +#include "FormData.h"

I don’t see any use of FormData in this file. What did I miss?

> Source/WebCore/platform/network/mac/BlobDataFileReferenceMac.mm:61
>	   // The archive is put into a subdirectory of temporary directory for
historic reasons. Changing this will require WebCore to change at the same
time.

This comment is peculiar. The comment is in WebCore (probably was elsewhere at
some point) but says "will require WebCore to change".

I wonder what these historic reasons are.

> Source/WebCore/platform/network/mac/BlobDataFileReferenceMac.mm:91
> +	   m_replacementPath = WTFMove(generatedFile);

Not obvious to me where this file is deleted, but I suppose it’s nothing new,
just not something could quickly see from looking over the code.