Bug 32993 – Blob.slice support

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Jian Li]

Need to implement Blob.slice support as defined in HTML 5 File API spec: http://www.w3.org/TR/FileAPI/#dfn-slice.

[Jian Li]

Created attachment 45579 [details]
Proposed Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45579 [details] without any errors.

[WebKit Review Bot]

Attachment 45579 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/151173

[WebKit Review Bot]

Attachment 45579 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/150287

[Maciej Stachowiak]

File API is not part of HTML5.

[Jian Li]

Created attachment 45581 [details]
Proposed Patch

Update the description in ChangeLog per the bug title change.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45581 [details] without any errors.

[WebKit Review Bot]

Attachment 45581 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/150327

[WebKit Review Bot]

Attachment 45581 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/151208

[Jian Li]

(In reply to comment #9)
> Attachment 45581 [details] did not build on qt:
> Build output: http://webkit-commit-queue.appspot.com/results/151208

The current patch is created on top of the patch for Bug 32912. Once it is checked in, I will create a new patch that should be buildable.

[WebKit Review Bot]

Attachment 45581 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/175057

[Dmitry Titov]

Comment on attachment 45581 [details]
Proposed Patch

Temporarily remove r? because the patch that must go first (bug 32912) is likely be updated in a way that can affect this patch.

[Jian Li]

Created attachment 48081 [details]
Proposed Patch

[WebKit Review Bot]

Attachment 48081 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/232841

[Jian Li]

Created attachment 48087 [details]
Proposed Patch

Fixed Chromium build break.

[Jian Li]

Created attachment 48442 [details]
Proposed Patch

Updated the patch to sync to the latest code, including patch for 34652.

[WebKit Review Bot]

Attachment 48442 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/250768

[Jian Li]

Created attachment 48447 [details]
Proposed Patch

Fixed chromium build break.

[Dmitry Titov]

Comment on attachment 48447 [details]
Proposed Patch

> 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 onUnstableFileDrop(file)
> +
> +    // The file modification time is second-based. So delay changing the file in order to get the new modification time for the file.
> +    setTimeout("onModifyFile()", 500);

Ouch. Maybe better to say "The file modification time has resolution of one second.", or something like that, to explain such a delay...

> --- a/WebCore/ChangeLog

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

"to capture"

> diff --git a/WebCore/html/Blob.cpp b/WebCore/html/Blob.cpp
> index 0b677ea..acf6142 100644
> --- a/WebCore/html/Blob.cpp
> +++ b/WebCore/html/Blob.cpp
> @@ -37,17 +37,72 @@ namespace WebCore {
>  
>  Blob::Blob(const String& path)
>      : m_path(path)
> +    , m_start(0)
> +    , m_length(-1)
> +    , m_snapshotSize(-1)
> +    , m_snapshotModificationTime(0)

These 4 variables seems all have 'special values' like 0 or -1 and this creates a matrix of possible behaviors. It's better to have a specific state expressed as bool variable ("m_isPathOnly" or "m_snapshotCaptured") and have Asserts in accessor functions so that nobody could consume things like length() if the length is not actually valid. At least we should be able to enumerate and document all possible combinations and have asserts to verify that we are not getting out of valid space. For example, is it possible to have start(100), length(-1)? Probably not.

>  unsigned long long Blob::size() const
>  {
> +    return static_cast<unsigned long long>((m_snapshotSize == -1) ? getFileSizeHelper() : m_length); 

This doesn't seem to need a cast.

> +long long Blob::getFileSizeHelper() const
> +{
>      // FIXME: JavaScript cannot represent sizes as large as unsigned long long, we need to
>      // come up with an exception to throw if file size is not represetable.
>      long long size;
>      if (!getFileSize(m_path, size))
>          return 0;

This 0 means the file does not exist or is unreadable. It is not the same as having a file of size 0. Should the code differentiate?

> +time_t Blob::getFileModificationTimeHelper() const
> +{
> +    time_t modificationTime;
> +    if (!getFileModificationTime(m_path, modificationTime))
> +        return 0;

Same thing here - the file likely does not exist (and may appear before the page will attempt to send it, which is a valid scenario).

> diff --git a/WebCore/platform/network/FormData.h b/WebCore/platform/network/FormData.h
> -    FormDataElement(const String& filename, bool shouldGenerateFile) : m_type(encodedFile), m_filename(filename), m_shouldGenerateFile(shouldGenerateFile) { }
> +    FormDataElement(const String& filename, int64_t fileStart, int64_t fileLength, time_t fileModificationTime, bool shouldGenerateFile) : m_type(encodedFile), m_filename(filename), m_fileStart(fileStart), m_fileLength(fileLength), m_fileModificationTime(fileModificationTime), m_shouldGenerateFile(shouldGenerateFile) { }

shouldn't use int64. long long is better in WebCore.

> +    int64_t m_fileStart;
> +    int64_t m_fileLength;

Ditto.

> +static bool advanceCurrentStream(FormStreamFields *form)

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

I think this check should be more complex. For example, we might have scenario when file did not exist before and now it does, and Blob is still 'floating' (no captured snapshot).
Also, getFileModificationTime() may return false if there is no file and we might want to fail operation (if we had captured snapshot before).

>          form->currentStream = CFReadStreamCreateWithFile(0, fileURL.get());
> +        if (nextInput.m_fileStart > 0) {
> +            CFNumberRef position = CFNumberCreate(0, kCFNumberLongLongType, &nextInput.m_fileStart);
> +            CFReadStreamSetProperty(form->currentStream, kCFStreamPropertyFileCurrentOffset, position);

This code will probably need to be looked at by someone who udnerstands CF better...

Because of int64 and need to clean up the code around 4 variales with 'special values' I mark it r-.
But, very good progress!

[Ojan Vafai]

Comment on attachment 48447 [details]
Proposed Patch

> +function onUnstableFileDrop(file)
> +    // The file modification time is second-based. So delay changing the file in order to get the new modification time for the file.
> +    setTimeout("onModifyFile()", 500);
> +}
> +
> +function onModifyFile()
> +    // Check the new modification time to make sure it is different. If not, repeat the above process.
> +    if (tempFileCurrentModificationTime == tempFileOriginalModificationTime)
> +        setTimeout('onModifyFile()', 500);
> +    else
> +        setTimeout('onUploadModifiedFile()', 0);
> +}

Is there any way to avoid using setTimeout here? Are there any events you could listen to? setTimeouts like these almost always lead to flaky tests. Also they make the tests slow to run (avg test runtime is ~30ms). If you can think of any way to avoid them, please do.

[Jian Li]

Created attachment 48536 [details]
Proposed Patch

All fixed. Also change the new layout test not to use setTimeout.

[Dmitry Titov]

Comment on attachment 48536 [details]
Proposed Patch

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).

[Jian Li]

Created attachment 49327 [details]
Proposed Patch

[Jian Li]

(In reply to comment #22)
> > 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.
> 
As we discussed, the current FormDataElement should be good enough to support file-based Blob and future binary Blob. So we do not want to make things more complicated here.

> > 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.
> 
Add ENABLE_BLOB_SLICE define so that Blob.slice is only exposed for those platforms that support sending partial file.

> 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.
> 
The logic to enforce this is in Blob.slice. See the code under the comment "If we fail to retrieve the size or modification time, probably due to that the file has been deleted, an empty blob will be returned."

[Jian Li]

Created attachment 49347 [details]
Proposed Patch

Updated the patch to use ENABLE(BLOB_SLICE) all necessary code.

[Dmitry Titov]

Comment on attachment 49347 [details]
Proposed Patch

The patch looks good to me.

I think the ENABLE_BLOB_SLICE should also be added into following files since it's good to keep them in sync:

WebKitLibraries/win/tools/vsprops/FeatureDefines.vsprops
WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.vsprops
WebCore/WebCore.pri
WebCore/GNUMakefile.am (also it would be nice to replace spaces with tabs in front of "WebCore/html/Blob.cpp,.h" in this file)
WebKit/chromium/features.gypi

r- because of this.

[Jian Li]

Created attachment 49408 [details]
Proposed Patch

All fixed.

[Dmitry Titov]

Comment on attachment 49408 [details]
Proposed Patch

r=me but please ask someone familiar with WebCore/platform/network/mac/FormDataStreamMac.mm to take a look before landing, since I'm not very familiar with CF.

It also would be nice to send headsup to owners of Qt, GTK, Apple Win ports to make sure they know about ENABLE_BLOB_SLICE.

[Darin Adler]

Comment on attachment 49408 [details]
Proposed Patch

> +    m_elements.append(FormDataElement(filename, 0, -1, 0.0, shouldGenerateFile));

I find the use of numeric constants here confusing. Do the values have special meaning? If so, then there should be named constants for us to use.

> +    m_elements.append(FormDataElement(filename, start, length, expectedModificationTime, false));

Here's a case where our use of boolean is confusing. You didn't add it, but the "false" hanging here is mysterious.

> -static void advanceCurrentStream(FormStreamFields *form)
> +static bool advanceCurrentStream(FormStreamFields *form)

If you're going to touch this line, then I think you should fix the formatting and move the * next to FormStreamFields.

You need a comment explaining what the boolean return value is. It's pretty mysterious otherwise.

>      while (form->currentStream && !CFReadStreamOpen(form->currentStream))
> -        advanceCurrentStream(form);
> +        if (!advanceCurrentStream(form))
> +            return false;

WebKit coding style requires braces in a case like this.

> +#if ENABLE(BLOB_SLICE)
> +    newInfo->currentStreamRangeLength = -1;
> +#endif

If -1 is a value with a special meaning it should be named constant.

> +    error->error = opened ? 0 : fnOpnErr;

How did you chose fnOpnErr? I don't think it's the correct error code to use here. It's the error code for attempting operations that work only on open files on a file that is not open.

> +#if ENABLE(BLOB_SLICE)
> +        if (form->currentStreamRangeLength != -1 && form->currentStreamRangeLength < bytesToRead)

Again, need a named constant for -1 to make it clear what it means.

> +            bytesToRead = static_cast<CFIndex>(form->currentStreamRangeLength);

What's the guarantee that a long long value can fit in a CFIndex? On 32-bit it might not fit.

> +            if (form->currentStreamRangeLength != -1)

Again, need a named constant for -1 to make it clear what it means.

> +            if (element.m_fileLength != -1) {

Again, need a named constant for -1 to make it clear what it means.

> +                length += element.m_fileLength;
> +                continue;

Does this handle the case correctly where m_fileLength is too big, and the file on disk is no longer that long?

[Jian Li]

(In reply to comment #29)
> (From update of attachment 49408 [details])
> > +    m_elements.append(FormDataElement(filename, 0, -1, 0.0, shouldGenerateFile));
> 
> I find the use of numeric constants here confusing. Do the values have special
> meaning? If so, then there should be named constants for us to use.

Added named constants in Blob.h for -1 and 0.0. The 0 for start seems to be straightforward and does not mean anything special. So I do not add the constant for it.
> 
> > +    m_elements.append(FormDataElement(filename, start, length, expectedModificationTime, false));
> 
> Here's a case where our use of boolean is confusing. You didn't add it, but the
> "false" hanging here is mysterious.

Changed the declaration of new function to also pass shouldGenerateFile so that we do not need to pass false here.
> 
> > -static void advanceCurrentStream(FormStreamFields *form)
> > +static bool advanceCurrentStream(FormStreamFields *form)
> 
> If you're going to touch this line, then I think you should fix the formatting
> and move the * next to FormStreamFields.

Fixed.
> 
> You need a comment explaining what the boolean return value is. It's pretty
> mysterious otherwise.
> 
> >      while (form->currentStream && !CFReadStreamOpen(form->currentStream))
> > -        advanceCurrentStream(form);
> > +        if (!advanceCurrentStream(form))
> > +            return false;
> 
> WebKit coding style requires braces in a case like this.

Fixed.
> 
> > +#if ENABLE(BLOB_SLICE)
> > +    newInfo->currentStreamRangeLength = -1;
> > +#endif
> 
> If -1 is a value with a special meaning it should be named constant.

Fixed.
> 
> > +    error->error = opened ? 0 : fnOpnErr;
> 
> How did you chose fnOpnErr? I don't think it's the correct error code to use
> here. It's the error code for attempting operations that work only on open
> files on a file that is not open.

I tried to find an appropriate system error code for this, but did not get the right one. Our error means either the file does not exist now or has been changed. I still can't find a good error code to use. So I use fnfErr. Probably we can use it to mean the original file is not found. Please tell me if there is a better one.
> 
> > +#if ENABLE(BLOB_SLICE)
> > +        if (form->currentStreamRangeLength != -1 && form->currentStreamRangeLength < bytesToRead)
> 
> Again, need a named constant for -1 to make it clear what it means.

Fixed.
> 
> > +            bytesToRead = static_cast<CFIndex>(form->currentStreamRangeLength);
> 
> What's the guarantee that a long long value can fit in a CFIndex? On 32-bit it
> might not fit.

Yes, I am aware of this. We only do the cast when form->currentStreamRangeLength is less than bufferLength.
    CFIndex bytesToRead = bufferLength;
    if (form->currentStreamRangeLength != Blob::toEndOfFile && form->currentStreamRangeLength < bytesToRead)
         bytesToRead = static_cast<CFIndex>(form->currentStreamRangeLength);
> 
> > +            if (form->currentStreamRangeLength != -1)
> 
> Again, need a named constant for -1 to make it clear what it means.
> 

Fixed.
> > +            if (element.m_fileLength != -1) {
> 
> Again, need a named constant for -1 to make it clear what it means.
> 

Fixed.
> > +                length += element.m_fileLength;
> > +                continue;
> 
> Does this handle the case correctly where m_fileLength is too big, and the file
> on disk is no longer that long?

Yes. We will check if the file has been changed right before reading the file (see advanceCurrentStream). I added a comment for it.

[Jian Li]

Created attachment 49605 [details]
Proposed Patch

[WebKit Commit Bot]

Comment on attachment 49408 [details]
Proposed Patch

Cleared Dmitry Titov's review+ from obsolete attachment 49408 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Dmitry Titov]

Comment on attachment 49605 [details]
Proposed Patch

It looks to me that Jian has addressed Darin's comments. Since I did most of reviewing in this bug before, I'll r+ it.

[Jian Li]

Committed as http://trac.webkit.org/changeset/55670.