Bug 176069 - [WinCairo] Add Network Process files for wincairo webkit
Summary: [WinCairo] Add Network Process files for wincairo webkit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 174003
  Show dependency treegraph
 
Reported: 2017-08-29 14:49 PDT by Stephan Szabo
Modified: 2017-09-27 12:51 PDT (History)
8 users (show)

See Also:


Attachments
Add basic versions/stubs for Network Process files for wincairo webkit (25.43 KB, patch)
2017-08-30 16:58 PDT, Stephan Szabo
achristensen: review-
Details | Formatted Diff | Diff
Add basic versions/stubs for Network Process files for wincairo webkit (no LegacyCustomProtocolManager) (22.05 KB, patch)
2017-09-01 10:42 PDT, Stephan Szabo
achristensen: review-
Details | Formatted Diff | Diff
Add basic versions/stubs for Network Process files for wincairo webkit (21.97 KB, patch)
2017-09-01 15:37 PDT, Stephan Szabo
achristensen: review+
Details | Formatted Diff | Diff
Add basic versions/stubs for Network Process files for wincairo webkit (fixed styes errors) (22.06 KB, patch)
2017-09-04 23:10 PDT, Yousuke Kimoto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephan Szabo 2017-08-29 14:49:36 PDT
Basic versions of Network Process files for wincairo webkit2 for initial building.
Comment 1 Stephan Szabo 2017-08-30 16:58:41 PDT
Created attachment 319424 [details]
Add basic versions/stubs for Network Process files for wincairo webkit
Comment 2 Alex Christensen 2017-08-30 17:35:47 PDT
Comment on attachment 319424 [details]
Add basic versions/stubs for Network Process files for wincairo webkit

View in context: https://bugs.webkit.org/attachment.cgi?id=319424&action=review

> Source/WebKit/ChangeLog:8
> +        * NetworkProcess/CustomProtocols/curl/LegacyCustomProtocolManagerCurl.cpp: Added.

Let's not add an attempt to implement LegacyCustomProtocolManager.  Implement API::URLSchemeTask instead.
Comment 3 Yousuke Kimoto 2017-08-31 04:09:11 PDT
(In reply to Alex Christensen from comment #2)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319424&action=review
> 
> > Source/WebKit/ChangeLog:8
> > +        * NetworkProcess/CustomProtocols/curl/LegacyCustomProtocolManagerCurl.cpp: Added.
> 
> Let's not add an attempt to implement LegacyCustomProtocolManager. 
> Implement API::URLSchemeTask instead.

Thank you for your review. I'm working for this patch with Stephan Szabo.

Why should not an attempt implementation be added to LegacyCustomProtocolManager? If it is not implemented, many LINK errors occur.
For example,  DerivedSources/WebKit2/LegacyCustomProtocolManagerMessageReceiver.cpp requires them.
Those implementations are just to solve such LINK errors.
Comment 4 Alex Christensen 2017-08-31 19:01:31 PDT
We should instead remove LegacyCustomProtocolManager.messages.in from the common WebKit2_MESSAGES_IN_FILES and add them in PlatformMac and PlatformGTK.  We are moving in a direction of removing the LegacyCustomProtocolManager and we shouldn't make this even harder to do or tempt people to implement the wrong thing.
Comment 5 Stephan Szabo 2017-09-01 10:41:30 PDT
I just added https://bugs.webkit.org/show_bug.cgi?id=176230 for dealing with making it not required and will put up a new version of this without the LegacyCustomProtocolManager file.
Comment 6 Stephan Szabo 2017-09-01 10:42:19 PDT
Created attachment 319612 [details]
Add basic versions/stubs for Network Process files for wincairo webkit (no LegacyCustomProtocolManager)
Comment 7 Alex Christensen 2017-09-01 10:44:45 PDT
Comment on attachment 319612 [details]
Add basic versions/stubs for Network Process files for wincairo webkit (no LegacyCustomProtocolManager)

View in context: https://bugs.webkit.org/attachment.cgi?id=319612&action=review

> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:70
> +        goto exitGracefully;
> +
> +    wcstombs(buffer, tHost, bufferLen);
> +    result = true;
> +
> +exitGracefully:
> +    if (tRegBuffer)
> +        delete [] tRegBuffer;

This is what std::unique_ptr<TCHAR[]> is for.  Let's not add goto for this.
Comment 8 Stephan Szabo 2017-09-01 15:37:03 PDT
Created attachment 319655 [details]
Add basic versions/stubs for Network Process files for wincairo webkit

Cleanup for gotos, ensure wcstombs output ends up null terminated
Comment 9 Build Bot 2017-09-01 15:39:29 PDT
Attachment 319655 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:47:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:58:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Alex Christensen 2017-09-01 15:44:33 PDT
Comment on attachment 319655 [details]
Add basic versions/stubs for Network Process files for wincairo webkit

View in context: https://bugs.webkit.org/attachment.cgi?id=319655&action=review

> Source/WebKit/NetworkProcess/Downloads/curl/DownloadCurl.cpp:29
> +#if !USE(NETWORK_SESSION)

We should be planning to make a NETWORK_SESSION implementation for CURL soon.  Mac, iOS, and GTK have already switched.  The only reason we have this code in WebKit is because of El Capitan.
Luckily a NetworkSession implementation is quite similar to a ResourceHandle implementation using asynchronous client callbacks.
Comment 11 Yousuke Kimoto 2017-09-04 23:10:47 PDT
Created attachment 319878 [details]
Add basic versions/stubs for Network Process files for wincairo webkit (fixed styes errors)

(In reply to Build Bot from comment #9)
> ERROR: Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:47:  Tab found;
> better to use spaces  [whitespace/tab] [1]
> ERROR: Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:58:  Tab found;
> better to use spaces  [whitespace/tab] [1]
> Total errors found: 2 in 10 files

Fixed these two style check errors.
Comment 12 WebKit Commit Bot 2017-09-05 11:08:25 PDT
Comment on attachment 319878 [details]
Add basic versions/stubs for Network Process files for wincairo webkit (fixed styes errors)

Clearing flags on attachment: 319878

Committed r221625: <http://trac.webkit.org/changeset/221625>
Comment 13 Radar WebKit Bug Importer 2017-09-27 12:51:56 PDT
<rdar://problem/34694168>