Bug 40593

Created attachment 59045 [details] Patch

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

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 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?

Created attachment 59172 [details] Patch

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

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

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?

Created attachment 59175 [details] Patch

(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 on attachment 59175 [details] Patch You still have a bunch of reference to JSBlobBuilder* even thought it doesn't seem to exist anymore.

(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 on attachment 59175 [details] Patch Oh, you're right, my mistake. You should renominate your patch for review.

Comment on attachment 59175 [details] Patch (renominating for review)

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

Comment on attachment 59175 [details] Patch I might have put the blob tests in their own subfolder, but this looks great. Thanks!

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

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

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

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

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