Summary: | Remove USE(CFNETWORK)+PLATFORM(COCOA) code path | ||
---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> |
Component: | Page Loading | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> |
Status: | NEW --- | ||
Severity: | Normal | CC: | ap, barraclough, benjamin, cdumez, cmarcelo, commit-queue, darin, dbates, ddkilzer, eric.carlson, glenn, japhet, jer.noble, mitz, philipj, psolanki, sam, sergio |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Bug Depends on: | 142540 | ||
Bug Blocks: | |||
Attachments: |
Description
Antti Koivisto
2014-10-21 15:49:22 PDT
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. |