Summary: | [Mac] Wrap a resource and resource loader in a NSURLSession-like object for use by lower level frameworks | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Jer Noble <jer.noble> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, buildbot, commit-queue, jonlee, mkwst, rniwa, webkit-bug-importer | ||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||
Bug Blocks: | 189742 | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Jer Noble
2016-01-29 13:34:37 PST
Created attachment 270257 [details]
Patch
Attachment 270257 [details] did not pass style-queue:
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0: Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py. [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:45: Place brace on its own line for function definitions. [whitespace/braces] [4]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:58: Place brace on its own line for function definitions. [whitespace/braces] [4]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:471: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 4 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 270423 [details]
Patch
Attachment 270423 [details] did not pass style-queue:
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0: Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py. [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:460: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 270427 [details]
Patch
Attachment 270427 [details] did not pass style-queue:
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0: Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py. [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:453: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 270434 [details]
Patch
Attachment 270434 [details] did not pass style-queue:
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0: Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py. [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:468: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 270447 [details]
Patch
Attachment 270447 [details] did not pass style-queue:
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0: Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py. [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:469: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 270452 [details]
Patch
Attachment 270452 [details] did not pass style-queue:
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0: Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py. [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:469: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 270454 [details]
Patch
Attachment 270454 [details] did not pass style-queue:
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0: Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py. [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:469: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 270500 [details]
Patch
Attachment 270500 [details] did not pass style-queue:
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0: Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py. [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:479: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 270500 [details] Patch Attachment 270500 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/773584 Number of test failures exceeded the failure limit. Created attachment 270504 [details]
Archive of layout-test-results from ews116 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 270500 [details] Patch Attachment 270500 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/773600 Number of test failures exceeded the failure limit. Created attachment 270505 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 270500 [details] Patch Attachment 270500 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/773602 Number of test failures exceeded the failure limit. Created attachment 270506 [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
Created attachment 270509 [details]
Patch
Attachment 270509 [details] did not pass style-queue:
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0: Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py. [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:486: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 270509 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270509&action=review > Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h:59 > + CacheData = 2 << 0, This isn't really used in this patch. > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:56 > + Vector<RetainPtr<WebCoreNSURLSessionDataTask>> _dataTasks; I think we should use a HashSet to avoid removing from the middle of a Vector. > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:107 > + int64_t _countOfBytesReceived; > + int64_t _countOfBytesSent; > + int64_t _countOfBytesExpectedToSend; > + int64_t _countOfBytesExpectedToReceive; Do these really need to be signed? Can they ever be <= 0? > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:110 > + [self.delegateQueue addOperationWithBlock:[strongSelf] { Sometimes you use _queue directly. I think this should be consistent. > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:421 > + callOnMainThread([strongSelf] { Are these and the ASSERT(isMainThread())'s correct when the delegate queue is not the [NSOperationQueue mainQueue]? > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:489 > + if (disposition == NSURLSessionResponseAllow) else if Also, what should be done if disposition is NSURLSessionResponseBecomeDownload or NSURLSessionResponseBecomeStream? > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:503 > + // FIXME: try to avoid a copy, if possible. This had better be possible. Otherwise, we have done something terribly wrong and it will kill media performance. > Source/WebCore/platform/spi/mac/AVFoundationSPI.h:78 > +#import <AVFoundation/AVAssetResourceLoader.h> This isn't used in this patch. > Tools/TestWebKitAPI/Tests/WebCore/WebCoreNSURLSession.mm:79 > + didRecieveData = true; It would be nice if we could check to make sure the data is actually what we expect it to be. Comment on attachment 270509 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270509&action=review >> Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h:59 >> + CacheData = 2 << 0, > > This isn't really used in this patch. It's for a follow up patch; I'll remove it. >> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:56 >> + Vector<RetainPtr<WebCoreNSURLSessionDataTask>> _dataTasks; > > I think we should use a HashSet to avoid removing from the middle of a Vector. OK. >> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:107 >> + int64_t _countOfBytesExpectedToReceive; > > Do these really need to be signed? Can they ever be <= 0? The protocol I'm re-implementing requires these to be int64_t. We can implement the ivars as uint64_t and have the properties be uint64_t, but we could no longer use @synthesize, and it would require potentially confusing casts everywhere. >> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:110 >> + [self.delegateQueue addOperationWithBlock:[strongSelf] { > > Sometimes you use _queue directly. I think this should be consistent. Ok. >> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:421 >> + callOnMainThread([strongSelf] { > > Are these and the ASSERT(isMainThread())'s correct when the delegate queue is not the [NSOperationQueue mainQueue]? Yes. >> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:489 >> + if (disposition == NSURLSessionResponseAllow) > > else if > > Also, what should be done if disposition is NSURLSessionResponseBecomeDownload or NSURLSessionResponseBecomeStream? Since we don't support Download tasks or Stream tasks, we don't handle those dispositions. I suppose we could add an ASSERT_NOT_REACHED() to catch if any of our clients start requesting those tasks. >> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:503 >> + // FIXME: try to avoid a copy, if possible. > > This had better be possible. Otherwise, we have done something terribly wrong and it will kill media performance. It's not currently possible to get a reference-counted handle on the memory. That's an improvement we have to look into, hence the FIXME. >> Source/WebCore/platform/spi/mac/AVFoundationSPI.h:78 >> +#import <AVFoundation/AVAssetResourceLoader.h> > > This isn't used in this patch. I'll remove it. >> Tools/TestWebKitAPI/Tests/WebCore/WebCoreNSURLSession.mm:79 >> + didRecieveData = true; > > It would be nice if we could check to make sure the data is actually what we expect it to be. I'll add some logic to assert that the data returned matches the data in the file. Comment on attachment 270509 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270509&action=review >> Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h:59 >> + CacheData = 2 << 0, > > This isn't really used in this patch. It's for a follow up patch; I'll remove it. >> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:56 >> + Vector<RetainPtr<WebCoreNSURLSessionDataTask>> _dataTasks; > > I think we should use a HashSet to avoid removing from the middle of a Vector. OK. >> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:107 >> + int64_t _countOfBytesExpectedToReceive; > > Do these really need to be signed? Can they ever be <= 0? The protocol I'm re-implementing requires these to be int64_t. We can implement the ivars as uint64_t and have the properties be uint64_t, but we could no longer use @synthesize, and it would require potentially confusing casts everywhere. >> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:110 >> + [self.delegateQueue addOperationWithBlock:[strongSelf] { > > Sometimes you use _queue directly. I think this should be consistent. Ok. >> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:421 >> + callOnMainThread([strongSelf] { > > Are these and the ASSERT(isMainThread())'s correct when the delegate queue is not the [NSOperationQueue mainQueue]? Yes. >> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:489 >> + if (disposition == NSURLSessionResponseAllow) > > else if > > Also, what should be done if disposition is NSURLSessionResponseBecomeDownload or NSURLSessionResponseBecomeStream? Since we don't support Download tasks or Stream tasks, we don't handle those dispositions. I suppose we could add an ASSERT_NOT_REACHED() to catch if any of our clients start requesting those tasks. >> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:503 >> + // FIXME: try to avoid a copy, if possible. > > This had better be possible. Otherwise, we have done something terribly wrong and it will kill media performance. It's not currently possible to get a reference-counted handle on the memory. That's an improvement we have to look into, hence the FIXME. >> Source/WebCore/platform/spi/mac/AVFoundationSPI.h:78 >> +#import <AVFoundation/AVAssetResourceLoader.h> > > This isn't used in this patch. I'll remove it. >> Tools/TestWebKitAPI/Tests/WebCore/WebCoreNSURLSession.mm:79 >> + didRecieveData = true; > > It would be nice if we could check to make sure the data is actually what we expect it to be. I'll add some logic to assert that the data returned matches the data in the file. Created attachment 270589 [details]
Patch
Attachment 270589 [details] did not pass style-queue:
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0: Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py. [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:486: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 2 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 270589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270589&action=review > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:40 > +SOFT_LINK_CONSTANT_MAY_FAIL(Foundation, NSURLSessionTaskPriorityDefault, float) We should put a comment saying this is soft linked to link successfully in Yosemite so we can remember to clean up once we drop support for Yosemite. > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:466 > + // No-op. Shouldn't we call didSendBodyData on the delegate here? > Tools/TestWebKitAPI/Tests/WebCore/WebCoreNSURLSession.mm:128 > + [session finishTasksAndInvalidate]; This does not test finishTasksAndInvalidate with _dataTasks not empty, which uses a different code path. Please add such a test. (In reply to comment #30) > Comment on attachment 270589 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270589&action=review > > > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:40 > > +SOFT_LINK_CONSTANT_MAY_FAIL(Foundation, NSURLSessionTaskPriorityDefault, float) > > We should put a comment saying this is soft linked to link successfully in > Yosemite so we can remember to clean up once we drop support for Yosemite. Ok. > > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:466 > > + // No-op. > > Shouldn't we call didSendBodyData on the delegate here? No, we are not a Upload task. > > Tools/TestWebKitAPI/Tests/WebCore/WebCoreNSURLSession.mm:128 > > + [session finishTasksAndInvalidate]; > > This does not test finishTasksAndInvalidate with _dataTasks not empty, which > uses a different code path. Please add such a test. Ok. Created attachment 270596 [details]
Patch for landing
Attachment 270596 [details] did not pass style-queue:
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0: Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py. [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:487: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 2 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 270596 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=270596&action=review > Tools/TestWebKitAPI/Tests/WebCore/WebCoreNSURLSession.mm:151 > +TEST_F(WebCoreNSURLSessionTest, InvalidateEmpty) I don't think this tests what I was thinking. I think we need a test that calls dataTaskWithURL on the session and calls finishTasksAndInvalidate without waiting for the task to complete first. Comment on attachment 270596 [details] Patch for landing Clearing flags on attachment: 270596 Committed r196082: <http://trac.webkit.org/changeset/196082> Comment on attachment 270596 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=270596&action=review > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:206 > + WebCoreNSURLSessionDataTask *task = [[WebCoreNSURLSessionDataTask alloc] initWithSession:self identifier:_nextTaskIdentifier++ request:request]; Is the first task identifier 0? I think the first task identifier of a real NSURLSession is 1, and that might cause some problems. It might be worth using the prefix ++ instead. (In reply to comment #34) > Comment on attachment 270596 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270596&action=review > > > Tools/TestWebKitAPI/Tests/WebCore/WebCoreNSURLSession.mm:151 > > +TEST_F(WebCoreNSURLSessionTest, InvalidateEmpty) > > I don't think this tests what I was thinking. I think we need a test that > calls dataTaskWithURL on the session and calls finishTasksAndInvalidate > without waiting for the task to complete first. We have one of those, in BasicOperation: + didInvalidate = false; + + task = [session dataTaskWithURL:resourceURL]; + [task resume]; + [session finishTasksAndInvalidate]; + + TestWebKitAPI::Util::run(&didInvalidate); |