Bug 137936

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

142540

Bug Blocks:

Attachments:

Description	Flags
WIP patch	none
another	none
another	none
rebased	none
rebased	none
Rebased onto r194115	none
Patch to find build errors (iOS WK1 scheduling fixed; quickook tests fixed)	none
Patch to find build errors (iOS WK1 scheduling fixed; quickook tests fixed) v2	none
Patch to find build errors (fixes for iOS and Windows)	none
Patch to find build errors (fixes for iOS and Windows) v2	none
Patch to find build errors (fix NSURLDownload on iOS)	none
Patch to find build errors (fix WebDownload on iOS)	none
Patch to find build errors (fix WebDownload on iOS) v2	none
Patch to find build errors (fix DownloadMac.mm on iOS)	none
Rebaseline onto r194421 (build is likely broken again)	none

Antti Koivisto

Reported 2014-10-21 15:49:22 PDT

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.

Attachments
WIP patch (74.14 KB, patch) 2014-10-21 15:50 PDT, Antti Koivisto	no flags	Details Formatted Diff Diff
another (80.34 KB, patch) 2014-10-21 17:29 PDT, Antti Koivisto	no flags	Details Formatted Diff Diff
another (80.50 KB, patch) 2014-10-21 17:33 PDT, Antti Koivisto	no flags	Details Formatted Diff Diff
rebased (89.64 KB, patch) 2015-02-18 06:02 PST, Antti Koivisto	no flags	Details Formatted Diff Diff
rebased (91.69 KB, patch) 2015-02-18 07:09 PST, Antti Koivisto	no flags	Details Formatted Diff Diff
Rebased onto r194115 (107.55 KB, patch) 2015-12-16 09:49 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch to find build errors (iOS WK1 scheduling fixed; quickook tests fixed) (123.72 KB, patch) 2015-12-16 21:33 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch to find build errors (iOS WK1 scheduling fixed; quickook tests fixed) v2 (123.43 KB, patch) 2015-12-16 21:40 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch to find build errors (fixes for iOS and Windows) (125.36 KB, patch) 2015-12-17 03:14 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch to find build errors (fixes for iOS and Windows) v2 (125.32 KB, patch) 2015-12-17 03:20 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch to find build errors (fix NSURLDownload on iOS) (127.45 KB, patch) 2015-12-17 04:45 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch to find build errors (fix WebDownload on iOS) (128.10 KB, patch) 2015-12-17 09:00 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch to find build errors (fix WebDownload on iOS) v2 (128.09 KB, patch) 2015-12-17 09:49 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch to find build errors (fix DownloadMac.mm on iOS) (128.16 KB, patch) 2015-12-17 10:47 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Rebaseline onto r194421 (build is likely broken again) (123.36 KB, patch) 2015-12-26 04:21 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Show Obsolete (14) View All Add attachment proposed patch, testcase, etc.

Antti Koivisto

Comment 1 2014-10-21 15:50:37 PDT

Created attachment 240227 [details] WIP patch

Antti Koivisto

Comment 2 2014-10-21 17:29:45 PDT

Created attachment 240231 [details] another

Antti Koivisto

Comment 3 2014-10-21 17:33:16 PDT

Created attachment 240232 [details] another

WebKit Commit Bot

Comment 4 2014-10-21 17:35:59 PDT

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.

Antti Koivisto

Comment 5 2015-02-18 06:02:55 PST

Created attachment 246813 [details] rebased

Antti Koivisto

Comment 6 2015-02-18 07:09:12 PST

Created attachment 246814 [details] rebased

Alexey Proskuryakov

Comment 7 2015-02-18 12:03:23 PST

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.

Antti Koivisto

Comment 8 2015-02-18 13:15:36 PST

The iOS build failure on EWS is due to NSURLDownload being a private header. Internal build seems to run fine.

David Kilzer (:ddkilzer)

Comment 9 2015-12-16 09:49:47 PST

Created attachment 267462 [details] Rebased onto r194115

David Kilzer (:ddkilzer)

Comment 10 2015-12-16 11:10:51 PST

(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.

Darin Adler

Comment 11 2015-12-16 19:32:42 PST

At the time we switched to the C API it was a significant speedup. Has this changed since?

David Kilzer (:ddkilzer)

Comment 12 2015-12-16 21:22:31 PST

(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.

David Kilzer (:ddkilzer)

Comment 13 2015-12-16 21:26:27 PST

(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).

David Kilzer (:ddkilzer)

Comment 14 2015-12-16 21:33:22 PST

Created attachment 267532 [details] Patch to find build errors (iOS WK1 scheduling fixed; quickook tests fixed)

David Kilzer (:ddkilzer)

Comment 15 2015-12-16 21:40:11 PST

Created attachment 267533 [details] Patch to find build errors (iOS WK1 scheduling fixed; quickook tests fixed) v2

WebKit Commit Bot

Comment 16 2015-12-16 21:41:34 PST

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.

David Kilzer (:ddkilzer)

Comment 17 2015-12-17 03:14:38 PST

Created attachment 267553 [details] Patch to find build errors (fixes for iOS and Windows)

David Kilzer (:ddkilzer)

Comment 18 2015-12-17 03:20:10 PST

Created attachment 267554 [details] Patch to find build errors (fixes for iOS and Windows) v2

David Kilzer (:ddkilzer)

Comment 19 2015-12-17 04:45:51 PST

Created attachment 267557 [details] Patch to find build errors (fix NSURLDownload on iOS)

David Kilzer (:ddkilzer)

Comment 20 2015-12-17 09:00:51 PST

Created attachment 267564 [details] Patch to find build errors (fix WebDownload on iOS)

David Kilzer (:ddkilzer)

Comment 21 2015-12-17 09:49:59 PST

Created attachment 267567 [details] Patch to find build errors (fix WebDownload on iOS) v2

David Kilzer (:ddkilzer)

Comment 22 2015-12-17 10:47:42 PST

Created attachment 267569 [details] Patch to find build errors (fix DownloadMac.mm on iOS)

Antti Koivisto

Comment 23 2015-12-18 11:13:45 PST

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.

David Kilzer (:ddkilzer)

Comment 24 2015-12-26 04:21:18 PST

Created attachment 267925 [details] Rebaseline onto r194421 (build is likely broken again)

David Kilzer (:ddkilzer)

Comment 25 2015-12-27 17:38:52 PST

(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. :)

David Kilzer (:ddkilzer)

Comment 26 2015-12-31 01:22:32 PST

(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.

Note You need to log in before you can comment on or make changes to this bug.