Bug 36568 - Support BlobBuilder defined in FileAPI/FileWriter
: Support BlobBuilder defined in FileAPI/FileWriter
Status: RESOLVED FIXED
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
:
: 36903 40593
: 36567
  Show dependency treegraph
 
Reported: 2010-03-24 18:07 PST by
Modified: 2010-06-22 22:07 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


------- Comment #1 From 2010-03-30 12:19:43 PST -------
Created an attachment (id=52068) [details]
Patch
------- Comment #2 From 2010-03-30 12:31:43 PST -------
Uploaded the first patch for adding BlobBuilder.
------- Comment #3 From 2010-03-30 12:44:11 PST -------
Why does Blob get a stringValue accessor in debug builds?
------- Comment #4 From 2010-03-30 13:12:29 PST -------
(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 From 2010-03-30 14:07:09 PST -------
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 From 2010-03-30 16:12:46 PST -------
(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 From 2010-03-30 18:40:05 PST -------
A note for me: need to merge with this one https://bugs.webkit.org/show_bug.cgi?id=36678
------- Comment #8 From 2010-03-30 22:30:34 PST -------
> 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 From 2010-03-31 13:22:02 PST -------
Created an attachment (id=52206) [details]
Patch
------- Comment #10 From 2010-03-31 13:33:15 PST -------
Attachment 52206 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1646066
------- Comment #11 From 2010-03-31 13:56:54 PST -------
Attachment 52206 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1552140
------- Comment #12 From 2010-03-31 14:21:13 PST -------
(From update of attachment 52206 [details])
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 From 2010-03-31 14:40:29 PST -------
(In reply to comment #12)
> (From update of attachment 52206 [details] [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 From 2010-06-22 22:07:35 PST -------
Closing as it's been added.