[webkit-reviews] review denied: [Bug 32993] Blob.slice support : [Attachment 48536] Proposed Patch

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 32993: Blob.slice support
https://bugs.webkit.org/show_bug.cgi?id=32993

Attachment 48536: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=48536&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
I think the new test send-sliced-dragged-file.html will have to be added to
Skipped lists on at least 3 ports since they lack implementation of
eventSender.beginDragWithFiles:

grep -R "send-dragged-file" LayoutTests/ --include=Skipped
 platform/gtk/Skipped:http/tests/local/send-dragged-file.html
 platform/qt/Skipped:http/tests/local/send-dragged-file.html
 platform/win/Skipped:http/tests/local/send-dragged-file.html

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog

This ChangeLog could use a bit more info, you actually move the code to setup
the input element for "drop file on the input element" kind of tests into
separate file, create a script to change modification time of the file - not
just adding a new test. 

> diff --git
a/LayoutTests/http/tests/local/resources/drag-file-to-file-input-element.js
b/LayoutTests/http/tests/local/resources/drag-file-to-file-input-element.js

It seems this file should have a different name. It has nothing with actual
"drag-file..", it sets up the input element for dragging a file to it. Maybe
"setup-input-element-for-drag.js"?

> diff --git
a/LayoutTests/http/tests/local/resources/send-sliced-dragged-file.js
b/LayoutTests/http/tests/local/resources/send-sliced-dragged-file.js

> +function dragAndSliceStableFile(start, length)
> +{
> +    sliceStart = start;
> +    sliceLength = length;
> +    setFileInputDropCallback(onStableFileDrop);
> +    eventSender.beginDragWithFiles(["resources/file-for-drag-to-send.txt"]);


I think instead of having global sliceStart and slicelength it's possibel to
do:
setFileInputDropCallback(function() { onStableFileDrop(start, length); });

> +    // Touch the underlying temp file.
> +    touchTempFile();

Awesome! Long setTimeout calls are all gone :-)

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

> +	   1) File.slice() does a synchronous IO to captures the current size
and

Typo: "to captures" -> "to capture"

> diff --git a/WebCore/html/HTMLFormElement.cpp
b/WebCore/html/HTMLFormElement.cpp
> -			   result->appendFile(value.file()->path(),
shouldGenerateFile);
> +			   result->appendFile(value.file()->path(), 0, -1,
shouldGenerateFile);

There should be third parameter here - modificationTime. (as per declaration of
the appendFile in FormData.h in this patch).

> diff --git a/WebCore/platform/network/FormData.h
b/WebCore/platform/network/FormData.h

> @@ -32,11 +33,14 @@ class FormDataElement {
>  public:
>      FormDataElement() : m_type(data) { }
>      FormDataElement(const Vector<char>& array) : m_type(data), m_data(array)
{ }
> -    FormDataElement(const String& filename, bool shouldGenerateFile) :
m_type(encodedFile), m_filename(filename),
m_shouldGenerateFile(shouldGenerateFile) { }
> +    FormDataElement(const String& filename, long long fileStart, long long
fileLength, time_t fileModificationTime, bool shouldGenerateFile) :
m_type(encodedFile), m_filename(filename), m_fileStart(fileStart),
m_fileLength(fileLength), m_fileModificationTime(fileModificationTime),
m_shouldGenerateFile(shouldGenerateFile) { }

I think there should be a FormDataElement that simply has a Blob in it. While
it was just filename, it was ok to keep a single string but now it's getting to
all the internal members of Blob. Instead, we should be able to construct
FormDataElement from Blob and keep reference to the Blob, without unpacking the
data members. This will play well with future modifications of the Blob, when
Blob itself will be an abstract interface, while FileBlob and perhaps
CanvasBlob will have different internals.

It also allows to keep a Blob that didn't capture a snapshot intact until the
time when actual file data will be pulled out. With the patch as is,
xhr.send(file) will capture modificationTime of 0 at the moment of call and
later fail comparing modification time. Keeping the Blob until the actual data
read operation will preserve the ability to check the "validity" of the Blob at
the right time.

> diff --git a/WebCore/platform/network/mac/FormDataStreamMac.mm
b/WebCore/platform/network/mac/FormDataStreamMac.mm

What about other platforms? I see you are preparing Chromium implementation in
parallel, but since the actual reading of bytes is going to happen in resource
loader of the embedder, do we need to define API for checking the validity of
the data (perhaps on ResourceHandleClient ?). I think the embedder's code will
at least need to figure out Content-Length header and then actually send data.
So at some point it might "ask for a snapshot" for the Blob that was passed as
Element of FormData. Also the embedder should have a way to return an error
back that indicates that loading failed because the Element of the FormData
happened to be invalid at the moment of sending.

I think this probably deserves a bit more thinking.

> +	   if (nextInput.m_fileModificationTime) {
> +	       time_t modificationTime;
> +	       if (!getFileModificationTime(nextInput.m_filename,
modificationTime) || modificationTime != nextInput.m_fileModificationTime)
> +		   return false;
> +	   }

If file was always unreadable (even at the snapshot time), it may be reasonable
to send it as 0 bytes.

I'm going to r- this patch for now because it's not clear how various embedders
will implement the functionality of checking the validity of the Blob and
return error result. It is also not clear how to add other types of Blob in the
future (will it require all embedders to add another type of FormDataElement or
will there be a new interface for them which can serve as "Blob reader" for
them).