Comment on attachment 268721[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=268721&action=review> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:68
> URL requestURL = [[m_avRequest.get() request] URL];
You should store the result of [m_avRequest.get() request] to avoid having to call it twice.
> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:162
> + ParsedContentRange contentRange = resource->response().contentRange();
resource should be a reference, or it should be null-checked before using it for the first time, which in this case was before setting data.
> Source/WebCore/platform/network/BlobResourceHandle.h:104
> long long m_rangeSuffixLength;
> + long long m_totalSize { 0 };
I think you should add the { 0 } for all of these, and remove the initializers in the constructors. As it is now, it just looks inconsistent.
> Source/WebCore/platform/network/ResourceResponseBase.cpp:456
> + m_haveParsedContentRangeHeader = true;
I think this needs to be set back to false in ResourceResponseBase::updateHeaderParsedState.
> Source/WebCore/xml/XMLHttpRequest.cpp:433
> + case HTTPHeaderName::ContentRange:
Why is this being added as a forbidden request header? https://fetch.spec.whatwg.org/#forbidden-header-name says it's ok, and it will probably be ignored by the server, right?
Created attachment 268727[details]
Archive of layout-test-results from ews103 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 268728[details]
Archive of layout-test-results from ews112 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 268729[details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 268721[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=268721&action=review
Make the current tests pass.
> Source/WebCore/platform/network/ParsedContentRange.cpp:92
> + instanceLength = headerValue.substring(instanceLengthSeparatorToken + 1).toUInt64();
What if there is no valid number or '*' here? I think toUInt64 would return 0, but it should fail if you have something like "bytes asdf-1/2" or "bytes 0-asdf/*" or "bytes 0-1/asdf". What should happen if there's whitespace, like "byte 0 - 1 / *"? I think right now we would succeed parsing most of that, but the * would not succeed if there is whitespace before it.
Please add tests with strange and invalid headers.
> Source/WebCore/platform/network/ParsedContentRange.cpp:94
> + return areContentRangeValuesValid(firstBytePosition, lastBytePosition, instanceLength);
I think this would be nicer if it only set any of the output variables if it succeeded. Then it would be impossible to get a firstBytePosition without a lastBytePosition, for example.
Comment on attachment 268721[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=268721&action=review>> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:68
>> URL requestURL = [[m_avRequest.get() request] URL];
>
> You should store the result of [m_avRequest.get() request] to avoid having to call it twice.
Not only that, but since requestURL is only used in a LOG() statement, this will throw a "unused variable" warning in release builds. Will fix.
>> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:162
>> + ParsedContentRange contentRange = resource->response().contentRange();
>
> resource should be a reference, or it should be null-checked before using it for the first time, which in this case was before setting data.
I think an ASSERT is fine here, as getting a null value here would indicate that the caller of CashedResourceClient was behaving badly.
>> Source/WebCore/platform/network/BlobResourceHandle.h:104
>> + long long m_totalSize { 0 };
>
> I think you should add the { 0 } for all of these, and remove the initializers in the constructors. As it is now, it just looks inconsistent.
Sure.
>> Source/WebCore/platform/network/ParsedContentRange.cpp:92
>> + instanceLength = headerValue.substring(instanceLengthSeparatorToken + 1).toUInt64();
>
> What if there is no valid number or '*' here? I think toUInt64 would return 0, but it should fail if you have something like "bytes asdf-1/2" or "bytes 0-asdf/*" or "bytes 0-1/asdf". What should happen if there's whitespace, like "byte 0 - 1 / *"? I think right now we would succeed parsing most of that, but the * would not succeed if there is whitespace before it.
>
> Please add tests with strange and invalid headers.
With this patch, both "0-asdf/*" and "asdf-1/*" will fail, but "0-1/asdf" will not. I'll fix it so that the latter one will fail.
I'll make the "*" check whitespace agnostic as well.
>> Source/WebCore/platform/network/ParsedContentRange.cpp:94
>> + return areContentRangeValuesValid(firstBytePosition, lastBytePosition, instanceLength);
>
> I think this would be nicer if it only set any of the output variables if it succeeded. Then it would be impossible to get a firstBytePosition without a lastBytePosition, for example.
I considered this, but thought that, for diagnostic purposes, it might be nice to see the values which caused the range to become invalid.
>> Source/WebCore/platform/network/ResourceResponseBase.cpp:456
>> + m_haveParsedContentRangeHeader = true;
>
> I think this needs to be set back to false in ResourceResponseBase::updateHeaderParsedState.
Ok.
>> Source/WebCore/xml/XMLHttpRequest.cpp:433
>> + case HTTPHeaderName::ContentRange:
>
> Why is this being added as a forbidden request header? https://fetch.spec.whatwg.org/#forbidden-header-name says it's ok, and it will probably be ignored by the server, right?
It will be ignored by the server. I'm guessing that, since Content-Length is a forbidden header, the lack of Content-Range in the spec is an oversight. But until they update the spec to add it, i'll remove it here.
Comment on attachment 268824[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=268824&action=review> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:79
> + LOG_ERROR("Failed to start load for media at url %s", URL([nsRequest URL]).string().ascii().data());
Since we have an NSURL here, may as well do [[nsurl absoluteString] UTF8String]
> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:118
> + ParsedContentRange contentRange = resource->response().contentRange();
There's no need to copy the whole data structure. Use ParsedContentRange&
Also, I don't think resource is null-checked.
> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:152
> void WebCoreAVFResourceLoader::fulfillRequestWithResource(CachedResource* resource)
> {
> + ASSERT(resource);
Maybe for a followup patch, but if resource is always non-null it should be a CachedResource&. Same with WebCoreAVFResourceLoader::responseReceived
> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:163
> + ParsedContentRange contentRange = resource->response().contentRange();
As the patch is right now, I think responseOffset should be left as 0 if resource is null.
> Source/WebCore/platform/network/BlobResourceHandle.h:105
> + long long m_totalSize { 0 };
> long long m_totalRemainingSize;
You didn't make all of these { 0 };
:(
> Source/WebCore/platform/network/ResourceResponseBase.h:43
> +class ParsedContentRange;
You shouldn't need to forward declare this if you include ParsedContentRange.h.
> Tools/ChangeLog:18
> +2016-01-11 Jer Noble <jer.noble@apple.com>
Too many changelog entries.
> Tools/TestWebKitAPI/Tests/WebCore/ParsedContentRange.cpp:35
> +TEST(WebCore, ParsedContentRangeFromString)
Add a test if -0 is the start of the range.
> Tools/TestWebKitAPI/Tests/WebCore/ParsedContentRange.cpp:39
> + ASSERT_TRUE(ParsedContentRange("bytes 0 - 1 / * ").isValid());
This is probably looser than the RFC standard.
Comment on attachment 268824[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=268824&action=review>> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:79
>> + LOG_ERROR("Failed to start load for media at url %s", URL([nsRequest URL]).string().ascii().data());
>
> Since we have an NSURL here, may as well do [[nsurl absoluteString] UTF8String]
Ok.
>> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:118
>> + ParsedContentRange contentRange = resource->response().contentRange();
>
> There's no need to copy the whole data structure. Use ParsedContentRange&
> Also, I don't think resource is null-checked.
Ok.
Will add an ASSERT(resource) (see below).
>> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:152
>> + ASSERT(resource);
>
> Maybe for a followup patch, but if resource is always non-null it should be a CachedResource&. Same with WebCoreAVFResourceLoader::responseReceived
Unfortunately, this would require re-writing every client of CachedResourceClient. (Which maybe we should do, but would absolutely need to be in a follow-up patch).
>> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:163
>> + ParsedContentRange contentRange = resource->response().contentRange();
>
> As the patch is right now, I think responseOffset should be left as 0 if resource is null.
We asserted above that resource will never be null.
>> Source/WebCore/platform/network/BlobResourceHandle.h:105
>> long long m_totalRemainingSize;
>
> You didn't make all of these { 0 };
> :(
Whoops!
>> Source/WebCore/platform/network/ResourceResponseBase.h:43
>> +class ParsedContentRange;
>
> You shouldn't need to forward declare this if you include ParsedContentRange.h.
Whoops!
>> Tools/ChangeLog:18
>> +2016-01-11 Jer Noble <jer.noble@apple.com>
>
> Too many changelog entries.
Whoops!
>> Tools/TestWebKitAPI/Tests/WebCore/ParsedContentRange.cpp:35
>> +TEST(WebCore, ParsedContentRangeFromString)
>
> Add a test if -0 is the start of the range.
Added. Fails correctly.
>> Tools/TestWebKitAPI/Tests/WebCore/ParsedContentRange.cpp:39
>> + ASSERT_TRUE(ParsedContentRange("bytes 0 - 1 / * ").isValid());
>
> This is probably looser than the RFC standard.
Will tighten.
Created attachment 268888[details]
Archive of layout-test-results from ews101 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 268889[details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 268892[details]
Archive of layout-test-results from ews112 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
(In reply to comment #24)
> Created attachment 268892[details]
> Archive of layout-test-results from ews112 for mac-yosemite
>
> The attached test failures were seen while running run-webkit-tests on the
> mac-debug-ews.
> Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Sigh. The expected.txt is missing an extra line-feed. Will fix.
Created attachment 270056[details]
Archive of layout-test-results from ews113 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
2016-01-11 10:25 PST, Jer Noble
2016-01-11 15:55 PST, Jer Noble
2016-01-11 16:56 PST, Build Bot
2016-01-11 16:59 PST, Build Bot
2016-01-11 17:00 PST, Build Bot
2016-01-12 15:38 PST, Jer Noble
2016-01-12 15:42 PST, Jer Noble
2016-01-12 15:54 PST, Jer Noble
2016-01-13 11:22 PST, Jer Noble
buildbot: commit-queue-
2016-01-13 12:23 PST, Build Bot
2016-01-13 12:26 PST, Build Bot
2016-01-13 12:32 PST, Build Bot
2016-01-13 13:19 PST, Jer Noble
2016-01-14 12:21 PST, Jer Noble
2016-01-14 14:28 PST, Jer Noble
2016-01-14 16:40 PST, Jer Noble
2016-01-14 16:41 PST, Jer Noble
2016-01-19 12:38 PST, Jer Noble
2016-01-19 13:22 PST, Jer Noble
2016-01-27 14:35 PST, Jer Noble
2016-01-27 14:37 PST, Jer Noble
2016-01-27 15:47 PST, Build Bot