Bug 40593

Summary: Add BlobBuilder.idl to expose BlobBuilder interface
Product: WebKit Reporter: Kinuko Yasuda <kinuko>
Component: WebCore JavaScriptAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, dimich, eric, jianli, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 40735, 40950    
Bug Blocks: 36568    
Attachments:
Description Flags
Patch
none
Patch
none
Patch abarth: review+

Description Kinuko Yasuda 2010-06-14 15:15:58 PDT
Need to add BlobBuilder.idl to expose BlobBuilder interface.
BlobBuilder is specified in FileWriter spec: http://dev.w3.org/2009/dap/file-system/file-writer.html
Comment 1 Kinuko Yasuda 2010-06-17 16:39:53 PDT
Created attachment 59045 [details]
Patch
Comment 2 WebKit Review Bot 2010-06-17 17:00:14 PDT
Attachment 59045 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3304298
Comment 3 Kinuko Yasuda 2010-06-17 18:12:39 PDT
Hmm, how can I work around not to make chromium-build look for BlobBuilder's v8 binding while (unconditionally) having BlobBuilderConstructor in DOMWindow.idl?

Is temporarily adding V8_BINDING guard appropriate?
Comment 4 Jian Li 2010-06-18 10:22:45 PDT
Comment on attachment 59045 [details]
Patch

I think you also need to update rebaseline test results for win/qt/gtk/chromium, like platform/qt/fast/dom/Window/window-property-descriptors-expected.txt.

WebCore/ChangeLog:61
 +          Add BlobBuilder.idl to expose BlobBuilder interface
Please also mention the link to the BlobBuilder definition in the FileWriter spec in the ChangeLog.

Also, I do not see any change to V8 bindings and gyp. Are you planning to add it later? If so, make sure you land both patches at the same time.

WebCore/bindings/js/JSBlobBuilderCustom.cpp:52
 +              String endings = "";
I think we can use the default empty string.

WebCore/html/BlobBuilder.idl:36
 +          Blob getBlob(in [ConvertUndefinedOrNullToNullString] DOMString contentType);
Do we need to add Optional here?
Comment 5 Kinuko Yasuda 2010-06-18 17:42:09 PDT
Created attachment 59172 [details]
Patch
Comment 6 Kinuko Yasuda 2010-06-18 17:45:29 PDT
(In reply to comment #4)
> (From update of attachment 59045 [details])
> I think you also need to update rebaseline test results for win/qt/gtk/chromium, like platform/qt/fast/dom/Window/window-property-descriptors-expected.txt.

Added them.

> WebCore/ChangeLog:61
>  +          Add BlobBuilder.idl to expose BlobBuilder interface
> Please also mention the link to the BlobBuilder definition in the FileWriter spec in the ChangeLog.

Done.

> Also, I do not see any change to V8 bindings and gyp. Are you planning to add it later? If so, make sure you land both patches at the same time.

Hmm, I added them to this patch.

> WebCore/bindings/js/JSBlobBuilderCustom.cpp:52
>  +              String endings = "";
> I think we can use the default empty string.

Fixed.

> WebCore/html/BlobBuilder.idl:36
>  +          Blob getBlob(in [ConvertUndefinedOrNullToNullString] DOMString contentType);
> Do we need to add Optional here?

Made it Optional (explicitly).
Comment 7 WebKit Review Bot 2010-06-18 17:55:53 PDT
Attachment 59172 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3277402
Comment 8 Adam Barth 2010-06-18 19:32:50 PDT
Comment on attachment 59172 [details]
Patch

WebCore/bindings/js/JSBlobBuilderCustom.cpp:44
 +  JSValue JSBlobBuilder::append(ExecState* exec)
Can this be non-custom with our new overloads support in the JSC bindings generator?
Comment 9 Kinuko Yasuda 2010-06-18 20:39:50 PDT
Created attachment 59175 [details]
Patch
Comment 10 Kinuko Yasuda 2010-06-18 20:41:36 PDT
(In reply to comment #8)
> (From update of attachment 59172 [details])
> WebCore/bindings/js/JSBlobBuilderCustom.cpp:44
>  +  JSValue JSBlobBuilder::append(ExecState* exec)
> Can this be non-custom with our new overloads support in the JSC bindings generator?

Oh yes, it can.  Fixed.
Comment 11 Adam Barth 2010-06-18 20:51:31 PDT
Comment on attachment 59175 [details]
Patch

You still have a bunch of reference to JSBlobBuilder* even thought it doesn't seem to exist anymore.
Comment 12 Kinuko Yasuda 2010-06-18 21:20:30 PDT
(In reply to comment #11)
> (From update of attachment 59175 [details])
> You still have a bunch of reference to JSBlobBuilder* even thought it doesn't seem to exist anymore.

Hmm, could you tell me which reference do you mean?   As for JSBlobBuilder.{cpp,h} I thought we still need them as they get generated.   Thanks.
Comment 13 Adam Barth 2010-06-18 21:26:58 PDT
Comment on attachment 59175 [details]
Patch

Oh, you're right, my mistake.  You should renominate your patch for review.
Comment 14 Kinuko Yasuda 2010-06-18 21:45:25 PDT
Comment on attachment 59175 [details]
Patch

(renominating for review)
Comment 15 WebKit Review Bot 2010-06-18 22:01:08 PDT
Attachment 59175 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3289345
Comment 16 Adam Barth 2010-06-19 16:55:06 PDT
Comment on attachment 59175 [details]
Patch

I might have put the blob tests in their own subfolder, but this looks great.  Thanks!
Comment 17 Kinuko Yasuda 2010-06-21 15:42:15 PDT
Committed r61585: <http://trac.webkit.org/changeset/61585>
Comment 18 WebKit Review Bot 2010-06-21 15:48:03 PDT
http://trac.webkit.org/changeset/61585 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release
Comment 19 Kinuko Yasuda 2010-06-21 16:06:53 PDT
(In reply to comment #18)
> http://trac.webkit.org/changeset/61585 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release

I had missed some files to include when I tried to land the patch with local tweaks (webkit-patch failed so I did it manually).  I'm going to retry submitting it later.
Comment 20 Kinuko Yasuda 2010-06-22 20:56:16 PDT
Committed r61650: <http://trac.webkit.org/changeset/61650>
Comment 21 WebKit Review Bot 2010-06-22 21:15:33 PDT
http://trac.webkit.org/changeset/61650 might have broken Qt Linux Release