Bug 32912 - Implement File and Blob interfaces as defined in File API spec.
: Implement File and Blob interfaces as defined in File API spec.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Jian Li
:
Depends on:
Blocks: 32993
  Show dependency treegraph
 
Reported: 2009-12-23 16:59 PST by Jian Li
Modified: 2010-01-20 15:55 PST (History)
5 users (show)

See Also:


Attachments
Proposed Patch (37.97 KB, patch)
2009-12-23 17:21 PST, Jian Li
abarth: review-
jianli: commit‑queue-
Details | Formatted Diff | Diff
Proposed Patch (44.25 KB, patch)
2009-12-23 18:22 PST, Jian Li
jianli: commit‑queue-
Details | Formatted Diff | Diff
Proposed Patch (44.25 KB, patch)
2009-12-28 18:04 PST, Jian Li
dimich: review-
jianli: commit‑queue-
Details | Formatted Diff | Diff
Proposed Patch (43.78 KB, patch)
2010-01-14 15:51 PST, Jian Li
jianli: commit‑queue-
Details | Formatted Diff | Diff
Proposed Patch (43.65 KB, patch)
2010-01-14 17:26 PST, Jian Li
jianli: commit‑queue-
Details | Formatted Diff | Diff
Proposed Patch (39.08 KB, patch)
2010-01-19 10:51 PST, Jian Li
dimich: review-
jianli: commit‑queue-
Details | Formatted Diff | Diff
Proposed Patch (41.01 KB, patch)
2010-01-19 16:05 PST, Jian Li
dimich: review+
jianli: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2009-12-23 16:59:47 PST
This is the first step to implement HTML 5 File API as defined in http://www.w3.org/TR/FileAPI. We need to add Blob interface and change File interface to derive from it.

The implementation of File.urn and Blob.slice will not be in the scope of this bug. Separate bugs will be created.
Comment 1 Jian Li 2009-12-23 17:21:01 PST
Created attachment 45459 [details]
Proposed Patch
Comment 2 WebKit Review Bot 2009-12-23 17:22:30 PST
Attachment 45459 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/File.h:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/html/Blob.cpp:72:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/html/Blob.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 3
Comment 3 WebKit Review Bot 2009-12-23 17:31:57 PST
Attachment 45459 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/142502
Comment 4 Adam Barth 2009-12-23 18:00:44 PST
Comment on attachment 45459 [details]
Proposed Patch

Looks like you need to teach the V8 code generator about long long to get this to build.  Also, there are style nits and tabs in the patch.
Comment 5 Jian Li 2009-12-23 18:22:12 PST
Created attachment 45460 [details]
Proposed Patch

Fixed style problems and added V8 binding changes (I previously planned to put them in a separate patch).
Comment 6 Maciej Stachowiak 2009-12-28 17:46:40 PST
File API is not part of HTML5.
Comment 7 Jian Li 2009-12-28 17:50:48 PST
(In reply to comment #6)
> File API is not part of HTML5.

Sorry for the confusion. Should I just use "File API" to refer to the spec? Thanks.
Comment 8 Jian Li 2009-12-28 18:04:32 PST
Created attachment 45582 [details]
Proposed Patch

Update the description in ChangeLog per the bug title change.
Comment 9 Dmitry Titov 2010-01-08 15:03:05 PST
The change looks ok but  you 'cache' the length in the File at the moment of its creation, which will cause behavior change from current one -  if the file changes on the disk before XHR.send(file) is called. I don't know if this is good or bad.

I've sent mail to public-webapps trying to clarify this: http://www.mail-archive.com/public-webapps@w3.org/msg07198.html

File API spec does not tell much about behavior when files on disk change. My first reaction would be to mark any Blob obtained from slice() method as 'invalid' and fail operations with it once file on disk changes - while keeping the 'original' Blob which is also a File 'mutating', following the state of the file on the disk. This would keep current behavior of XHR.send(file) which is also consistent with regular form submission.
Comment 10 Darin Fisher (:fishd, Google) 2010-01-11 23:08:49 PST
<input type=file> does not require the referenced file to remain unmodified until it is submitted, and since Blob can just be a pointer to the file referenced by an INPUT element, I think it should similarly not care if the file is modified.  We should probably just treat Blob as a path to data.  This implies not caching file length.
Comment 11 Dmitry Titov 2010-01-12 12:32:35 PST
(In reply to comment #10)
> <input type=file> does not require the referenced file to remain unmodified
> until it is submitted, and since Blob can just be a pointer to the file
> referenced by an INPUT element, I think it should similarly not care if the
> file is modified.  We should probably just treat Blob as a path to data.  This
> implies not caching file length.

Agree. Marking it r- to remove the caching.
Comment 12 Jian Li 2010-01-14 15:51:49 PST
Created attachment 46616 [details]
Proposed Patch

Removed the caching for file length.
Comment 13 Jian Li 2010-01-14 17:26:08 PST
Created attachment 46624 [details]
Proposed Patch

Produced a new patch that resolved the conflict with other landed patch so that the commit queue can check this patch.
Comment 14 WebKit Review Bot 2010-01-14 20:46:55 PST
Attachment 46624 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/187848
Comment 15 Jian Li 2010-01-19 10:51:27 PST
Created attachment 46927 [details]
Proposed Patch

Removed those properties, that are not implemented yet, from IDL files, as suggested by dimich.
Comment 16 Dmitry Titov 2010-01-19 12:16:20 PST
Comment on attachment 46927 [details]
Proposed Patch

Almost there. Would be nice to get it building on trybots though since it's mostly a change to build files...

> diff --git a/WebCore/bindings/objc/PublicDOMInterfaces.h b/WebCore/bindings/objc/PublicDOMInterfaces.h

> +@property(readonly, copy) NSString *type;
> +@property(readonly, copy) NSString *urn;

These are from a future patch. Should be removed.

> diff --git a/WebCore/html/Blob.cpp b/WebCore/html/Blob.cpp

> + * Copyright (C) 2009 Google Inc. All rights reserved.

2009 -> 2010?

> +unsigned long long Blob::size() const
> +{
> +    if (m_length != -1)
> +        return static_cast<unsigned long long>(m_length);

Since this patch does not depend on Blob holding m_length and m_start, lets not add them in this patch. They will be added as part of slice() I think.

> diff --git a/WebCore/html/Blob.h b/WebCore/html/Blob.h

> + * Copyright (C) 2009 Google Inc. All rights reserved.

2010

> diff --git a/WebCore/html/Blob.idl b/WebCore/html/Blob.idl

> + * Copyright (C) 2009 Google Inc. All rights reserved.

2010

> +File::~File()

Does it need an empty destructor?
Comment 17 Jian Li 2010-01-19 16:05:15 PST
Created attachment 46957 [details]
Proposed Patch

All fixed. Thanks.
Comment 18 Dmitry Titov 2010-01-19 16:42:39 PST
Comment on attachment 46957 [details]
Proposed Patch

r=me
Comment 19 Jian Li 2010-01-20 15:55:06 PST
Committed as http://trac.webkit.org/changeset/53574.