| Summary: | [Chromium] We need an API to expose blob creation to Chromium | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jay Civelli <jcivelli> | ||||||||||||
| Component: | WebKit Misc. | Assignee: | Jay Civelli <jcivelli> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | fishd, michaeln, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Jay Civelli
2011-09-16 15:08:27 PDT
Created attachment 107728 [details]
Patch
Created attachment 107729 [details]
Patch
Drive-by-comments only... Nice to see this doesn't require bunches of code. v8::Handle<v8::Value> createBlobFromFile(...) Since an opaque v8 handle is the return value, i wonder if the method sig should be tweeked... createBlobFromFileAsV8Handle()? Comment on attachment 107729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107729&action=review > Source/WebKit/chromium/public/WebBlob.h:48 > + static v8::Handle<v8::Value> createBlobFromFile(const WebString& path, long long size); nit: no need to mention "Blob" in the method name. just go with WebBlob::createFromFile. though it may seem heavier than you require, i think it would be better to make WebBlob be a proper wrapper of WebCore::Blob. WebCore::Blob is reference counted, so you can use WebPrivatePtr<T>. follow the example of WebNode. then, you can provide a WEBKIT_USING_V8 method name toV8Value(). Created attachment 108056 [details]
Patch
Comment on attachment 108056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108056&action=review > Source/WebKit/chromium/public/WebBlob.h:51 > + virtual ~WebBlob() { reset(); } nit: this can be non-virtual > Source/WebKit/chromium/public/WebBlob.h:54 > + WebBlob(const WebBlob& n) { assign(n); } nit: the "n" parameter name used in WebNode.h was chosen because "n" stands for "node" here, perhaps "b" would be a better choice. > Source/WebKit/chromium/public/WebBlob.h:66 > + WEBKIT_EXPORT bool equals(const WebBlob&) const; nit: it's okay to define equals here, but you could also leave it out if you like. we don't universally provide this for simple WebCore wrappers. > Source/WebKit/chromium/public/WebBlob.h:69 > + nit: only one new line here > Source/WebKit/chromium/public/WebBlob.h:77 > +#if WEBKIT_USING_V8 nit: we normally put WEBKIT_IMPLEMENTATION stuff last in the public section Created attachment 108075 [details]
Patch for landing
Comment on attachment 108075 [details] Patch for landing Rejecting attachment 108075 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 Last 500 characters of output: 85e273c316e0d416a2f995b30baa2e9a5ea2dc5a r95585 = 6e9b58622449856b0904ebe3d271c5190e568d0e Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Updating chromium port dependencies using gclient... ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/9767473 Created attachment 108089 [details]
Patch for landing
Comment on attachment 108089 [details] Patch for landing Clearing flags on attachment: 108089 Committed r95590: <http://trac.webkit.org/changeset/95590> All reviewed patches have been landed. Closing bug. |