Bug 43941 - Handle blob resource
Summary: Handle blob resource
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jian Li
URL:
Keywords:
Depends on: 43985
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-12 16:35 PDT by Jian Li
Modified: 2010-08-23 13:11 PDT (History)
6 users (show)

See Also:


Attachments
Proposed Patch (41.32 KB, patch)
2010-08-20 12:20 PDT, Jian Li
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (41.24 KB, patch)
2010-08-20 12:25 PDT, Jian Li
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (41.25 KB, patch)
2010-08-20 13:00 PDT, Jian Li
fishd: review+
jianli: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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