Bug 40593 – Add BlobBuilder.idl to expose BlobBuilder interface

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Kinuko Yasuda]

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

[Kinuko Yasuda]

Created attachment 59045 [details]
Patch

[WebKit Review Bot]

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

[Kinuko Yasuda]

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?

[Jian Li]

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?

[Kinuko Yasuda]

Created attachment 59172 [details]
Patch

[Kinuko Yasuda]

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

[WebKit Review Bot]

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

[Adam Barth]

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?

[Kinuko Yasuda]

Created attachment 59175 [details]
Patch

[Kinuko Yasuda]

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

[Adam Barth]

Comment on attachment 59175 [details]
Patch

You still have a bunch of reference to JSBlobBuilder* even thought it doesn't seem to exist anymore.

[Kinuko Yasuda]

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

[Adam Barth]

Comment on attachment 59175 [details]
Patch

Oh, you're right, my mistake.  You should renominate your patch for review.

[Kinuko Yasuda]

Comment on attachment 59175 [details]
Patch

(renominating for review)

[WebKit Review Bot]

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

[Adam Barth]

Comment on attachment 59175 [details]
Patch

I might have put the blob tests in their own subfolder, but this looks great.  Thanks!

[Kinuko Yasuda]

Committed r61585: <http://trac.webkit.org/changeset/61585>

[WebKit Review Bot]

http://trac.webkit.org/changeset/61585 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release

[Kinuko Yasuda]

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

[Kinuko Yasuda]

Committed r61650: <http://trac.webkit.org/changeset/61650>

[WebKit Review Bot]

http://trac.webkit.org/changeset/61650 might have broken Qt Linux Release