Bug 43941

Summary: Handle blob resource
Product: WebKit Reporter: Jian Li <jianli>
Component: WebCore JavaScriptAssignee: Jian Li <jianli>
Status: RESOLVED FIXED    
Severity: Normal CC: dimich, eric, fishd, kinuko, levin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 43985    
Bug Blocks:    
Attachments:
Description Flags
Proposed Patch
jianli: commit-queue-
Proposed Patch
jianli: commit-queue-
Proposed Patch fishd: review+, jianli: commit-queue-

Description Jian Li 2010-08-12 16:35:22 PDT
Need to add the support to handle blob resource.
Comment 1 Jian Li 2010-08-20 12:20:57 PDT
Created attachment 64971 [details]
Proposed Patch
Comment 2 WebKit Review Bot 2010-08-20 12:22:22 PDT
Attachment 64971 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/network/ResourceHandleClient.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/network/ResourceHandle.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/network/BlobResourceHandle.cpp:209:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/platform/network/BlobResourceHandle.cpp:237:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/network/BlobResourceHandle.cpp:398:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/network/BlobResourceHandle.cpp:443:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/platform/network/BlobResourceHandle.cpp:446:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 7 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Jian Li 2010-08-20 12:25:40 PDT
Created attachment 64972 [details]
Proposed Patch

Fix style errors.
Comment 4 WebKit Review Bot 2010-08-20 12:28:14 PDT
Attachment 64972 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/network/ResourceHandleClient.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/network/ResourceHandle.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Eric Seidel (no email) 2010-08-20 12:40:29 PDT
Attachment 64971 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3722453
Comment 6 Eric Seidel (no email) 2010-08-20 12:51:18 PDT
Attachment 64972 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3719462
Comment 7 Jian Li 2010-08-20 13:00:38 PDT
Created attachment 64976 [details]
Proposed Patch

Fix mac warnings.
Comment 8 WebKit Review Bot 2010-08-20 13:03:16 PDT
Attachment 64976 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/network/ResourceHandleClient.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/network/ResourceHandle.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Darin Fisher (:fishd, Google) 2010-08-20 14:43:42 PDT
Comment on attachment 64976 [details]
Proposed Patch

WebCore/page/SecurityOrigin.cpp:128
 +      if (url.protocolIs("blob"))
I thought we were using the "blobdata" scheme.

WebCore/platform/network/BlobResourceHandle.cpp:53
 +  static const int ok = 200;
nit: might be nice to name these http status codes with a prefix like httpOK, httpPartialContent, etc.

WebCore/platform/network/BlobResourceHandle.cpp:74
 +  class BlobResourceSynchronousLoader : public ResourceHandleClient {
can this class be in an anonymous namespace?

WebCore/platform/network/BlobResourceHandle.cpp:108
 +      m_data.resize(static_cast<size_t>(response.expectedContentLength()));
do you need to worry if expected content length is outrageously large?  it is a 64-bit value, so this could be large.  is there a security concern?

WebCore/platform/network/BlobResourceHandle.cpp:216
 +  bool BlobResourceHandle::parseRangeHeader(const String& range)
should this code be in HTTPParsers.{h,cpp}?

those are just nits, so R=me
Comment 10 Jian Li 2010-08-23 13:10:35 PDT
Committed as http://trac.webkit.org/changeset/65827.
Comment 11 Jian Li 2010-08-23 13:11:42 PDT
(In reply to comment #9)
> (From update of attachment 64976 [details])
> WebCore/page/SecurityOrigin.cpp:128
>  +      if (url.protocolIs("blob"))
> I thought we were using the "blobdata" scheme.
The latest version of File API uses "blob" scheme.

> 
> WebCore/platform/network/BlobResourceHandle.cpp:53
>  +  static const int ok = 200;
> nit: might be nice to name these http status codes with a prefix like httpOK, httpPartialContent, etc.
Fixed.

> 
> WebCore/platform/network/BlobResourceHandle.cpp:74
>  +  class BlobResourceSynchronousLoader : public ResourceHandleClient {
> can this class be in an anonymous namespace?
Fixed.

> 
> WebCore/platform/network/BlobResourceHandle.cpp:108
>  +      m_data.resize(static_cast<size_t>(response.expectedContentLength()));
> do you need to worry if expected content length is outrageously large?  it is a 64-bit value, so this could be large.  is there a security concern?
Yes. I've already handled it with a check right before this.

> 
> WebCore/platform/network/BlobResourceHandle.cpp:216
>  +  bool BlobResourceHandle::parseRangeHeader(const String& range)
> should this code be in HTTPParsers.{h,cpp}?
Fixed.

> 
> those are just nits, so R=me