Bug 36568 - Support BlobBuilder defined in FileAPI/FileWriter
: Support BlobBuilder defined in FileAPI/FileWriter
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To: Nobody
:
Depends on: 36903 40593
Blocks: 36567
  Show dependency treegraph
 
Reported: 2010-03-24 18:07 PDT by Kinuko Yasuda
Modified: 2010-06-22 22:07 PDT (History)
8 users (show)

See Also:


Attachments
Patch (78.98 KB, patch)
2010-03-30 12:19 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (80.41 KB, patch)
2010-03-31 13:22 PDT, Kinuko Yasuda
dimich: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Kinuko Yasuda 2010-03-30 12:19:43 PDT
Created attachment 52068 [details]
Patch
Comment 2 Kinuko Yasuda 2010-03-30 12:31:43 PDT
Uploaded the first patch for adding BlobBuilder.
Comment 3 Darin Fisher (:fishd, Google) 2010-03-30 12:44:11 PDT
Why does Blob get a stringValue accessor in debug builds?
Comment 4 Kinuko Yasuda 2010-03-30 13:12:29 PDT
(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.
Comment 5 Darin Fisher (:fishd, Google) 2010-03-30 14:07:09 PDT
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 :-)
Comment 6 Kinuko Yasuda 2010-03-30 16:12:46 PDT
(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.)
Comment 7 Kinuko Yasuda 2010-03-30 18:40:05 PDT
A note for me: need to merge with this one https://bugs.webkit.org/show_bug.cgi?id=36678
Comment 8 Darin Fisher (:fishd, Google) 2010-03-30 22:30:34 PDT
> 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 :)
Comment 9 Kinuko Yasuda 2010-03-31 13:22:02 PDT
Created attachment 52206 [details]
Patch
Comment 10 Early Warning System Bot 2010-03-31 13:33:15 PDT
Attachment 52206 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1646066
Comment 11 WebKit Review Bot 2010-03-31 13:56:54 PDT
Attachment 52206 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1552140
Comment 12 Dmitry Titov 2010-03-31 14:21:13 PDT
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.
Comment 13 Kinuko Yasuda 2010-03-31 14:40:29 PDT
(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.
Comment 14 Kinuko Yasuda 2010-06-22 22:07:35 PDT
Closing as it's been added.