Summary: | Handle blob resource | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jian Li <jianli> | ||||||||
Component: | WebCore JavaScript | Assignee: | 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
Jian Li
2010-08-12 16:35:22 PDT
Created attachment 64971 [details]
Proposed Patch
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.
Created attachment 64972 [details]
Proposed Patch
Fix style errors.
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.
Attachment 64971 [details] did not build on mac: Build output: http://queues.webkit.org/results/3722453 Attachment 64972 [details] did not build on mac: Build output: http://queues.webkit.org/results/3719462 Created attachment 64976 [details]
Proposed Patch
Fix mac warnings.
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 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
Committed as http://trac.webkit.org/changeset/65827. (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 |