This C API path is currently used on iOS. It is adding tons of complexity and bug surface for questionable benefits. We should remove it and switch to the CFNetwork ObjC API which we already use on OS X.
Created attachment 240227 [details] WIP patch
Created attachment 240231 [details] another
Created attachment 240232 [details] another
Attachment 240232 [details] did not pass style-queue: ERROR: Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:1: Could not find a newline character at the end of the file. [whitespace/ending_newline] [5] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246813 [details] rebased
Created attachment 246814 [details] rebased
I strongly oppose doing this at this time. While using the same code path on both platforms is so obviously the way to go, there couldn't be a worse time to do it.
The iOS build failure on EWS is due to NSURLDownload being a private header. Internal build seems to run fine.
Created attachment 267462 [details] Rebased onto r194115
(In reply to comment #9) > Created attachment 267462 [details] > Rebased onto r194115 This patch still has a scheduling bug where NSURLConnection callbacks are happening on the main thread (without the WebThread lock), but I have a fix for that that I'm testing locally. This patch is looking very promising now.
At the time we switched to the C API it was a significant speedup. Has this changed since?
(In reply to comment #11) > At the time we switched to the C API it was a significant speedup. Has this > changed since? Performance of this change has not been tested yet. However, I think the switch to CFURLConnection occurred prior to many CF types being [re-]implemented in terms of NS types. I plan to engage the perf team to test this change before landing it.
(In reply to comment #10) > (In reply to comment #9) > > Created attachment 267462 [details] > > Rebased onto r194115 > > This patch still has a scheduling bug where NSURLConnection callbacks are > happening on the main thread (without the WebThread lock), but I have a fix > for that that I'm testing locally. Fixed scheduling issue in Page::platformInitialize() in PageMac.mm. Also fixed a crash that happened on all quicklook tests due to the -clearHandle method being called on a class that didn't implement it. With these fixes, the NSURLConnection loader is experiencing roughly the same crashes as the exiting CFURLConnection code path (less than 10 total crashes on a full test run).
Created attachment 267532 [details] Patch to find build errors (iOS WK1 scheduling fixed; quickook tests fixed)
Created attachment 267533 [details] Patch to find build errors (iOS WK1 scheduling fixed; quickook tests fixed) v2
Attachment 267533 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/ios/QuickLook.mm:41: *SoftLink.h header should be included after all other headers. [build/include_order] [4] Total errors found: 1 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 267553 [details] Patch to find build errors (fixes for iOS and Windows)
Created attachment 267554 [details] Patch to find build errors (fixes for iOS and Windows) v2
Created attachment 267557 [details] Patch to find build errors (fix NSURLDownload on iOS)
Created attachment 267564 [details] Patch to find build errors (fix WebDownload on iOS)
Created attachment 267567 [details] Patch to find build errors (fix WebDownload on iOS) v2
Created attachment 267569 [details] Patch to find build errors (fix DownloadMac.mm on iOS)
One option would be to land https://bugs.webkit.org/show_bug.cgi?id=142540 first. It just switches the code paths (and makes them work) without deleting anything. That might be good for checking regressions.
Created attachment 267925 [details] Rebaseline onto r194421 (build is likely broken again)
(In reply to comment #23) > One option would be to land https://bugs.webkit.org/show_bug.cgi?id=142540 > first. It just switches the code paths (and makes them work) without > deleting anything. That might be good for checking regressions. Okay. I need to port some fixes from this patch to that one. Probably a better idea to land them separately anyway. :)
(In reply to comment #25) > (In reply to comment #23) > > One option would be to land https://bugs.webkit.org/show_bug.cgi?id=142540 > > first. It just switches the code paths (and makes them work) without > > deleting anything. That might be good for checking regressions. > > Okay. I need to port some fixes from this patch to that one. Probably a > better idea to land them separately anyway. :) There is now a patch up on Bug 142540 for review.