Bug 193161 - Remove or deprecate unused C SPI
Summary: Remove or deprecate unused C SPI
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-01-04 15:01 PST by Alex Christensen
Modified: 2019-04-17 11:17 PDT (History)
4 users (show)

See Also:


Attachments
Patch (53.91 KB, patch)
2019-01-04 15:03 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (50.10 KB, patch)
2019-01-08 20:45 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-01-04 15:01:17 PST
Remove or deprecate unused C SPI
Comment 1 Alex Christensen 2019-01-04 15:03:49 PST
Created attachment 358379 [details]
Patch
Comment 2 Alex Christensen 2019-01-04 17:06:16 PST
Note: IconDB code use was removed in rdar://problem/47057658
Comment 3 Alex Christensen 2019-01-08 10:14:06 PST
Could someone fix the gtk build with this patch?
Comment 4 Michael Catanzaro 2019-01-08 11:42:18 PST
In WKPage.cpp, change:

namespace API {

using namespace WebCore;
using namespace WebKit;

to:

using namespace WebCore;
using namespace WebKit;

namespace API {

That fixes it for me.
Comment 5 Alex Christensen 2019-01-08 11:44:12 PST
We don't want to do that.  This means we need to add WebKit::, WebCore::, or using namespace ...; inside a scope somewhere.  Could you upload a patch that does that?
Comment 6 Michael Catanzaro 2019-01-08 11:45:49 PST
Looks like this will break TestController:

[227/431] Building CXX object Tools/WebKitTestRunne...MakeFiles/WebKitTestRunner.dir/TestController.cpp.o
../../Tools/WebKitTestRunner/TestController.cpp: In destructor ‘WTR::TestController::~TestController()’:
../../Tools/WebKitTestRunner/TestController.cpp:161:69: warning: ‘const OpaqueWKIconDatabase* WKContextGetIconDatabase(WKContextRef)’ is deprecated: No longer supported [-Wdeprecated-declarations]
         WKIconDatabaseClose(WKContextGetIconDatabase(m_context.get()));
                                                                     ^
In file included from DerivedSources/ForwardingHeaders/WebKit/WKContext.h:1,
                 from ../../Source/WebKit/UIProcess/API/C/WebKit2_C.h:36,
                 from DerivedSources/ForwardingHeaders/WebKit/WebKit2_C.h:1,
                 from ../../Tools/WebKitTestRunner/config.h:31,
                 from ../../Tools/WebKitTestRunner/TestController.cpp:26:
../../Source/WebKit/UIProcess/API/C/WKContext.h:141:29: note: declared here
 WK_EXPORT WKIconDatabaseRef WKContextGetIconDatabase(WKContextRef context) WK_C_API_DEPRECATED;
                             ^~~~~~~~~~~~~~~~~~~~~~~~
../../Tools/WebKitTestRunner/TestController.cpp:161:69: warning: ‘const OpaqueWKIconDatabase* WKContextGetIconDatabase(WKContextRef)’ is deprecated: No longer supported [-Wdeprecated-declarations]
         WKIconDatabaseClose(WKContextGetIconDatabase(m_context.get()));
                                                                     ^
In file included from DerivedSources/ForwardingHeaders/WebKit/WKContext.h:1,
                 from ../../Source/WebKit/UIProcess/API/C/WebKit2_C.h:36,
                 from DerivedSources/ForwardingHeaders/WebKit/WebKit2_C.h:1,
                 from ../../Tools/WebKitTestRunner/config.h:31,
                 from ../../Tools/WebKitTestRunner/TestController.cpp:26:
../../Source/WebKit/UIProcess/API/C/WKContext.h:141:29: note: declared here
 WK_EXPORT WKIconDatabaseRef WKContextGetIconDatabase(WKContextRef context) WK_C_API_DEPRECATED;
                             ^~~~~~~~~~~~~~~~~~~~~~~~
../../Tools/WebKitTestRunner/TestController.cpp:161:70: warning: ‘void WKIconDatabaseClose(WKIconDatabaseRef)’ is deprecated: No longer supported [-Wdeprecated-declarations]
         WKIconDatabaseClose(WKContextGetIconDatabase(m_context.get()));
                                                                      ^
In file included from DerivedSources/ForwardingHeaders/WebKit/WKIconDatabase.h:1,
                 from ../../Tools/WebKitTestRunner/TestController.cpp:45:
../../Source/WebKit/UIProcess/API/C/WKIconDatabase.h:81:16: note: declared here
 WK_EXPORT void WKIconDatabaseClose(WKIconDatabaseRef iconDatabase) WK_C_API_DEPRECATED;
                ^~~~~~~~~~~~~~~~~~~
../../Tools/WebKitTestRunner/TestController.cpp:161:70: warning: ‘void WKIconDatabaseClose(WKIconDatabaseRef)’ is deprecated: No longer supported [-Wdeprecated-declarations]
         WKIconDatabaseClose(WKContextGetIconDatabase(m_context.get()));
                                                                      ^
In file included from DerivedSources/ForwardingHeaders/WebKit/WKIconDatabase.h:1,
                 from ../../Tools/WebKitTestRunner/TestController.cpp:45:
../../Source/WebKit/UIProcess/API/C/WKIconDatabase.h:81:16: note: declared here
 WK_EXPORT void WKIconDatabaseClose(WKIconDatabaseRef iconDatabase) WK_C_API_DEPRECATED;
                ^~~~~~~~~~~~~~~~~~~
Comment 7 Michael Catanzaro 2019-01-08 11:46:37 PST
(In reply to Alex Christensen from comment #5)
> We don't want to do that.  This means we need to add WebKit::, WebCore::, or
> using namespace ...; inside a scope somewhere.  Could you upload a patch
> that does that?

Hm, sorry I got confused. Usual C++ rule is to avoid using statements in global scope of header files, not source files, but of course we don't want to do that for unified builds. I'll try to make it work.
Comment 8 Michael Catanzaro 2019-01-08 11:59:21 PST
I think you should just pull all the C API files out of unified build using @no-unify (like we do for WPE/GTK API files). When they were added to the unified build, the using statements weren't removed from the global namespace like we did for normal sources (probably because this is quite hard to do for a C API file where all the API functions have to be in the global namespace!). It's not just a problem with WKPage.cpp -- it's coincidence that's the one we're fighting now -- but with all the files in this directory. With some quick search/replaces I've added the WebKit:: namespace in several hundred places where it's missing, but that just gets me error messages exceeding  my terminal scrollback, so I don't anticipate success this way.
Comment 9 Alex Christensen 2019-01-08 20:45:53 PST
Created attachment 358666 [details]
Patch
Comment 10 Alex Christensen 2019-01-09 11:43:53 PST
Michael, that didn't fix the Linux build.  I could fix the Mac build with that, but it looks like we don't want to do that.
Comment 11 Michael Catanzaro 2019-01-10 20:12:11 PST
The problem is some other file has these using statements in the global scope, and that file is included in the unified source bundle on all platforms except Linux.

Just un-unifying the file is not enough. Doing that alone will only surface the problem on all platforms (as we see on EWS). It's still not going to build unless you also move the using statements:

(In reply to Michael Catanzaro from comment #4)
> In WKPage.cpp, change:
> 
> namespace API {
> 
> using namespace WebCore;
> using namespace WebKit;
> 
> to:
> 
> using namespace WebCore;
> using namespace WebKit;
> 
> namespace API {
> 
> That fixes it for me.

Removing the file from the unified build just guarantees you can move the using statements without mucking up unrelated files.

Alternatively, once the file is removed from unified build, you can try to make it build standalone by adding WebKit:: and WebCore:: qualifications everywhere, then add it back to unified build once it works if you really want to. But I briefly tried this the other day, and I did not enjoy what I was getting myself into. ;)