Bug 163346 - GTK and EFL on Mac fail to compile WebTextChecker due to missing definition of WKTextCheckerClientBase
Summary: GTK and EFL on Mac fail to compile WebTextChecker due to missing definition o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-12 11:30 PDT by Jeremy Huddleston Sequoia
Modified: 2016-10-13 11:56 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.06 KB, patch)
2016-10-12 16:58 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (1.99 KB, patch)
2016-10-13 10:21 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (1.99 KB, patch)
2016-10-13 10:26 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (1.99 KB, patch)
2016-10-13 10:28 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Huddleston Sequoia 2016-10-12 11:30:48 PDT
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.
Comment 1 Jonathan Bedard 2016-10-12 13:57:59 PDT
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.
Comment 2 Jeremy Huddleston Sequoia 2016-10-12 14:06:43 PDT
Those files are included unconditionally in PlatformGTK.cmake

The configuration in question is PLATFORM(GTK) && OS(DARWIN)
Comment 3 Jonathan Bedard 2016-10-12 14:55:49 PDT
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.
Comment 4 Jonathan Bedard 2016-10-12 16:58:21 PDT
Created attachment 291424 [details]
Patch
Comment 5 Jeremy Huddleston Sequoia 2016-10-12 17:05:22 PDT
Thanks.  I'll test that and provide feedback or an updated patch.
Comment 6 Jeremy Huddleston Sequoia 2016-10-12 19:41:12 PDT
Tested, looks good.  Thanks.
Comment 7 Daniel Bates 2016-10-13 10:20:01 PDT
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.
Comment 8 Jonathan Bedard 2016-10-13 10:21:43 PDT
Created attachment 291490 [details]
Patch
Comment 9 Daniel Bates 2016-10-13 10:23:01 PDT
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.
Comment 10 Jonathan Bedard 2016-10-13 10:26:59 PDT
Created attachment 291491 [details]
Patch
Comment 11 Jonathan Bedard 2016-10-13 10:28:02 PDT
Created attachment 291492 [details]
Patch
Comment 12 WebKit Commit Bot 2016-10-13 11:56:22 PDT
Comment on attachment 291492 [details]
Patch

Clearing flags on attachment: 291492

Committed r207296: <http://trac.webkit.org/changeset/207296>
Comment 13 WebKit Commit Bot 2016-10-13 11:56:28 PDT
All reviewed patches have been landed.  Closing bug.