Bug 36568 – Support BlobBuilder defined in FileAPI/FileWriter

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Kinuko Yasuda]

http://dev.w3.org/2009/dap/file-system/file-writer.html

[Kinuko Yasuda]

Created attachment 52068 [details]
Patch

[Kinuko Yasuda]

Uploaded the first patch for adding BlobBuilder.

[Darin Fisher (:fishd, Google)]

Why does Blob get a stringValue accessor in debug builds?

[Kinuko Yasuda]

(In reply to comment #3)
> Why does Blob get a stringValue accessor in debug builds?

I wanted to have a handy way to test it.  I'll get rid of it if it shouldn't be there.

[Darin Fisher (:fishd, Google)]

Well, we run the layout tests both in debug and release mode, so whatever we do to support testing shouldn't be debug only.  Perhaps there should be a layoutTestController method for this, or maybe the web platform should just have a way to read from a Blob :-)

[Kinuko Yasuda]

(In reply to comment #5)
Right.  Another option I came up with was to use xhr.send(blob) (which sends a WebString in this patch) and let the server script check the content.  This is also handy but how we should handle xhr.send(blob) for blobs created by BlobBuilder is a bit unclear for now.  How do you think?

(By the way I'm getting a qt build error... does anyone know how I can fix it?  Thanks.)

[Kinuko Yasuda]

A note for me: need to merge with this one https://bugs.webkit.org/show_bug.cgi?id=36678

[Darin Fisher (:fishd, Google)]

> Right.  Another option I came up with was to use xhr.send(blob) (which sends a
> WebString in this patch) and let the server script check the content.  This is
> also handy but how we should handle xhr.send(blob) for blobs created by
> BlobBuilder is a bit unclear for now.  How do you think?

Using xhr.send(blob) sounds like a good way to solve the testing problem.  It would mean having to make these tests be http tests, but I guess that's OK.  It is nice when layout tests can also run in a normal browser using normal features of the web platform.

Under http/tests, you should see perl and php scripts that are used to provide for dynamic content.  You can probably just create a script like that which will echo the HTTP request body.  There may already be a script in http/tests that does that :)

[Kinuko Yasuda]

Created attachment 52206 [details]
Patch

[Early Warning System Bot]

Attachment 52206 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1646066

[WebKit Review Bot]

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

[Dmitry Titov]

Comment on attachment 52206 [details]
Patch

r-, due to the size of the patch.

It is very hard to review 80Kb patch, and if the review happens, the quality of it usually suffers. We ask contributors to come up with smaller patches, especially if it is relatively easy to do.

In this case, it's possible to structure the work like this:
1. Have an uber-bug which says "Implement BlobBuilder" and serves as umbrella for other bugs/patches.
2. Optionally attach the uber-patch with prototype into that bug.
3. Split work - in this case, it could be implementation of internal implementation objects, like FileBlob, then JSC bindings with a test or two, then v8 bindings, then more tests.
4. Have separate bugs with those smaller patches, referrign to the umbrella bug. Normally, we have 1 patch per bug.

I myself didn't appreciate much the 'smaller patches' concept until I became a reviewer. It takes about the same time to carefully review the patch as it would take to write it. In the case of a big patch, the process repeats on every iteration. It is really better and faster to come up with smaller patches.

[Kinuko Yasuda]

(In reply to comment #12)
> (From update of attachment 52206 [details])
> r-, due to the size of the patch.

Got it, it was still too big.  I will open more separate bugs and split the patch, making this bug as umbrella for them.  Thanks for your advice.

[Kinuko Yasuda]

Closing as it's been added.