Bug 208894 - Remove support for WebKitSystemInterface
Summary: Remove support for WebKitSystemInterface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Rollin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-10 16:38 PDT by Keith Rollin
Modified: 2020-03-16 14:50 PDT (History)
15 users (show)

See Also:


Attachments
Patch (209.30 KB, patch)
2020-03-10 16:40 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (209.49 KB, patch)
2020-03-16 13:05 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Rollin 2020-03-10 16:38:51 PDT
This project provided support for facilities that are now handled elsewhere and is no longer needed.
Comment 1 Radar WebKit Bug Importer 2020-03-10 16:38:59 PDT
<rdar://problem/60297590>
Comment 2 Keith Rollin 2020-03-10 16:40:42 PDT
Created attachment 393186 [details]
Patch
Comment 3 Keith Rollin 2020-03-10 16:42:05 PDT
To reviewers, I'm mostly worried about this change:

- # The static runtime is required for AppleWin due to WebKitSystemInterface.lib
- # being compiled with a static runtime.
- set(MSVC_STATIC_RUNTIME ON)

Is it appropriate to remove this settings?
Comment 4 Brent Fulgham 2020-03-11 10:54:18 PDT
Comment on attachment 393186 [details]
Patch

I love it. Delete delete delete! r=me
Comment 5 Konstantin Tokarev 2020-03-11 11:01:56 PDT
(In reply to Keith Rollin from comment #3)
> To reviewers, I'm mostly worried about this change:
> 
> - # The static runtime is required for AppleWin due to
> WebKitSystemInterface.lib
> - # being compiled with a static runtime.
> - set(MSVC_STATIC_RUNTIME ON)
> 
> Is it appropriate to remove this settings?

This depends on how do you build downstream projects and how you distribute their binaries.

1. Runtimes must match, so if you are building WebKit with static runtime (/MT) you should also build application with /MT, if you switch WebKit to shared runtime (/MD) application build settings need to be changed too.

2. If application is built with /MT, it's fully self-contained, while /MD application requires respective msvcrt dlls to be installed.
Comment 6 Keith Rollin 2020-03-11 12:50:35 PDT
On the one hand, it sounds like keeping the setting is good for backward compatibility with WebKit clients. On the other hand, allowing for a /MD style build to a shared runtime configuration would allow for smaller applications.

Who makes the decision on which route we want to take? If I don't hear anything, I'll revert this setting in the name of backward compatibility.
Comment 7 Keith Rollin 2020-03-16 13:05:19 PDT
Created attachment 393674 [details]
Patch
Comment 8 Per Arne Vollan 2020-03-16 13:30:31 PDT
(In reply to Keith Rollin from comment #6)
> On the one hand, it sounds like keeping the setting is good for backward
> compatibility with WebKit clients. On the other hand, allowing for a /MD
> style build to a shared runtime configuration would allow for smaller
> applications.
> 
> Who makes the decision on which route we want to take? If I don't hear
> anything, I'll revert this setting in the name of backward compatibility.

I think maintaining backward compatibility is a good call.
Comment 9 WebKit Commit Bot 2020-03-16 14:49:45 PDT
The commit-queue encountered the following flaky tests while processing attachment 393674 [details]:

http/tests/security/contentSecurityPolicy/report-status-code-zero-when-using-https.html bug 197297 (author: dbates@webkit.org)
The commit-queue is continuing to process your patch.
Comment 10 WebKit Commit Bot 2020-03-16 14:50:27 PDT
Comment on attachment 393674 [details]
Patch

Clearing flags on attachment: 393674

Committed r258520: <https://trac.webkit.org/changeset/258520>
Comment 11 WebKit Commit Bot 2020-03-16 14:50:29 PDT
All reviewed patches have been landed.  Closing bug.