Bug 163346 – GTK and EFL on Mac fail to compile WebTextChecker due to missing definition of WKTextCheckerClientBase

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jeremy Huddleston Sequoia]

With trunk r207192 and the buildfix for bug #163340, I'm hitting a compile failure now in WebTextChecker.cpp and WebTextCheckerClient.cpp regarding WKTextCheckerClientBase being unknown.

Given that WebTextChecker.cpp hasn't changed in almost two years and WebTextCheckerClient.cpp hasn't in almost three, it's likely a change to one of their header files 

Source/WebKit2/UIProcess/WebTextChecker.cpp:46:38: error: unknown type name 'WKTextCheckerClientBase'; did you mean 'WebCore::TextCheckerClient'?
void WebTextChecker::setClient(const WKTextCheckerClientBase* client)
                                     ^~~~~~~~~~~~~~~~~~~~~~~
                                     WebCore::TextCheckerClient


and similarly in WebTextCheckerClient.cpp:

Source/WebKit2/UIProcess/WebTextCheckerClient.cpp:27:
Source/WebKit2/UIProcess/WebTextCheckerClient.h:36:32: error: unknown type name 'WKTextCheckerClientBase'; did you mean 'WebCore::TextCheckerClient'?
template<> struct ClientTraits<WKTextCheckerClientBase> {
                               ^~~~~~~~~~~~~~~~~~~~~~~
                               WebCore::TextCheckerClient

---

It's defined in Source/WebKit2/UIProcess/API/C/WKTextChecker.h, and that was modified a few weeks ago as part of bug #161919, r206261.

Looking at that commit, it looks VERY wrong.  There's a ton of defined(__APPLE__) checks which are clearly incorrect.  The changes look related to toolkit, not platform.

[Jonathan Bedard]

What is the build configuration?

Bug #163340 adds access for the TextChecker for testing, which should be in a file named WKTextChecker.h.  What existed in WKTextChecker.h at the time should really be in a file named WKWebTextChecker.h, but renaming this file would be changing API.  WKTextChecker as of bug #163340 is essentially a combination of the two headers.  It breaks convention to preserve API.

In all of our builds, WebTextChecker is never used on Apple's systems, and this is the reason for the defined(__APPLE__).  I'm curious what you're doing to build WebTextChecker while on a system which would, at least to my understanding, usually not support it.

[Jeremy Huddleston Sequoia]

Those files are included unconditionally in PlatformGTK.cmake

The configuration in question is PLATFORM(GTK) && OS(DARWIN)

[Jonathan Bedard]

I didn't know that was valid platform combination.

Again, to preserve API, this patch will use #ifdefs.  Since we don't usually include our platform differentiation header, those macros will not be available.  This is also the method other API headers use when faced with this type of problem.

I don't have a GTK on Darwin build set up on any of my local machines, and I know our automation doesn't test this configuration either (otherwise we would have caught this bug weeks ago) so I would appreciate if you could confirm that this patch fixes your problem, I'm fairly certain it will.

I'll post the patch as soon as I confirm it at least builds locally for me.

[Jonathan Bedard]

Created attachment 291424 [details]
Patch

[Jeremy Huddleston Sequoia]

Thanks.  I'll test that and provide feedback or an updated patch.

[Jeremy Huddleston Sequoia]

Tested, looks good.  Thanks.

[Daniel Bates]

Comment on attachment 291424 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [GTK] trunk r207192 fails to compile WebTextChecker* due to missing definition of WKTextCheckerClientBase

Can we come up with a more descriptive title that explains that r207192 broke both GTK and EFL for Mac?

> Source/WebKit2/UIProcess/API/C/WKTextChecker.cpp:30
> +#if defined(BUILDING_EFL__) || defined(BUILDING_GTK__) || !defined(__APPLE__)

Is the disjunct !defined(__APPLE__) needed? It seems sufficient to omit this disjunct and it may be good to force new WebKit2 ports that make use of these functions to add to this list of defines in the hopes that when this happens they will consider separating the platform-independent functionality from the platform-specific functionality in this file.

> Source/WebKit2/UIProcess/API/C/WKTextChecker.h:37
> +#if defined(BUILDING_EFL__) || defined(BUILDING_GTK__) || !defined(__APPLE__)

Ditto.

[Jonathan Bedard]

Created attachment 291490 [details]
Patch

[Daniel Bates]

Comment on attachment 291490 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [GTK] trunk r207192 fails to compile WebTextChecker* due to missing definition of WKTextCheckerClientBase

Can we come up with a more descriptive title? See my remark in comment 7 for more details.

[Jonathan Bedard]

Created attachment 291491 [details]
Patch

[Jonathan Bedard]

Created attachment 291492 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 291492 [details]
Patch

Clearing flags on attachment: 291492

Committed r207296: <http://trac.webkit.org/changeset/207296>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.