Summary: | Add BlobBuilder.idl to expose BlobBuilder interface | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kinuko Yasuda <kinuko> | ||||||||
Component: | WebCore JavaScript | Assignee: | 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
Kinuko Yasuda
2010-06-14 15:15:58 PDT
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 |