Bug 189257 - Resurrect WebKitTestRunner for Windows port
Summary: Resurrect WebKitTestRunner for Windows port
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: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-03 23:31 PDT by Fujii Hironori
Modified: 2018-11-12 10:59 PST (History)
11 users (show)

See Also:


Attachments
Patch (68.34 KB, patch)
2018-11-05 21:48 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (70.25 KB, patch)
2018-11-07 04:42 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (86.59 KB, patch)
2018-11-07 18:36 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (86.65 KB, patch)
2018-11-07 19:17 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (88.75 KB, patch)
2018-11-11 19:05 PST, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (88.75 KB, patch)
2018-11-11 19:46 PST, Takashi Komori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2018-09-03 23:31:45 PDT
Resurrect WebKitTestRunner for Windows port

It has been removed in Bug 123152.
Comment 1 Takashi Komori 2018-11-05 21:48:05 PST
Created attachment 353945 [details]
Patch
Comment 2 EWS Watchlist 2018-11-05 22:01:28 PST
Attachment 353945 [details] did not pass style-queue:


ERROR: Source/WebKit/PlatformWin.cmake:195:  Alphabetical sorting problem. "Shared/API/c/cairo" should be before "Shared/API/c/win".  [list/order] [5]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:30:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:120:  Missing space before {  [whitespace/braces] [5]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:183:  Missing spaces around =  [whitespace/operators] [4]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:183:  Missing spaces around <  [whitespace/operators] [3]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:183:  Missing space before {  [whitespace/braces] [5]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:199:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:200:  Use adoptGDIObject and GDIObject<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:219:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:220:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:221:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:222:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:223:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:224:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:225:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:226:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerInjectedBundlePrefix.cpp:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/win/TestControllerWin.cpp:180:  Missing space before {  [whitespace/braces] [5]
ERROR: Tools/WebKitTestRunner/win/TestControllerWin.cpp:180:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Tools/WebKitTestRunner/win/TestControllerWin.cpp:183:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Tools/WebKitTestRunner/win/TestControllerWin.cpp:185:  bundle_dir is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/WebKitTestRunner/win/TestControllerWin.cpp:189:  bundle_dir_utf is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/WebKitTestRunner/win/TestControllerWin.cpp:190:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Tools/WebKitTestRunner/win/WebKitTestRunnerPrefix.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 24 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Don Olmstead 2018-11-06 10:39:13 PST
Comment on attachment 353945 [details]
Patch

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

Overall looks about ready to go. Just some cleanup on the macros is needed.

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:73
> +#if !PLATFORM(GTK) && !PLATFORM(WPE) && !PLATFORM(WIN)

Could probably be #if PLATFORM(COCOA) at this point

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:82
> +#if (!PLATFORM(GTK) && !PLATFORM(WPE) && !PLATFORM(WIN)) || !HAVE(ACCESSIBILITY)

if PLATFORM(COCOA) || !HAVE(ACCESSIBILITY)

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:365
> +#if PLATFORM(COCOA) || PLATFORM(GTK) || PLATFORM(WPE) || PLATFORM(WIN)

Think this guard can be removed.

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:96
> +#if PLATFORM(GTK) || PLATFORM(WPE) || PLATFORM(WIN)

#if !PLATFORM(COCOA)

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:43
> +#elif PLATFORM(GTK) || PLATFORM(WPE) || PLATFORM(WIN)

I think this can probably just be an #else since it looks like there's a PLATFORM(COCOA) above.

> Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerInjectedBundlePrefix.h:45
> +#ifdef __OBJC__
> +#import <Foundation/Foundation.h>
> +#endif
> +
> +#include <WebKit/WebKit2_C.h>
> +#include <wtf/Platform.h>
> +
> +/* When C++ exceptions are disabled, the C++ library defines |try| and |catch|
> +* to allow C++ code that expects exceptions to build. These definitions
> +* interfere with Objective-C++ uses of Objective-C exception handlers, which
> +* use |@try| and |@catch|. As a workaround, undefine these macros. */
> +
> +#ifdef __cplusplus
> +#include <algorithm> // needed for exception_defines.h
> +#endif
> +
> +#ifdef __OBJC__
> +#undef try
> +#undef catch
> +#endif

Delete this part we don't have ObjC in Windows.

> Tools/WebKitTestRunner/PlatformWebView.h:65
> +#elif PLATFORM(WIN)
> +#include <cairo.h>
> +class TestRunnerWindow;
> +typedef HWND PlatformWindow;
> +typedef cairo_surface_t* PlatformImage;
> +typedef WKViewRef PlatformWKView;

It looks like it would be better to define PlatformImage in a #if USE(CAIRO) that way WPE/GTK/WinCairo all get it.

> Tools/WebKitTestRunner/TestController.cpp:881
> +#if !PLATFORM(COCOA) && !PLATFORM(WPE) && !PLATFORM(WIN)

#if PLATFORM(GTK)

> Tools/WebKitTestRunner/WebKitTestRunnerPrefix.h:50
> +#if OS(WINDOWS)
> +#undef WEBCORE_EXPORT
> +#define WEBCORE_EXPORT WTF_IMPORT_DECLARATION
> +#endif

There any WebKit exports that need to be defined?

> Tools/WebKitTestRunner/win/PlatformWebViewWin.cpp:171
> +static cairo_surface_t* generateCairoSurfaceFromBitmap(BITMAP bitmapTag)

Not a huge deal but it might make sense to have some #if USE(CAIRO) guards around these. Apple is looking at a Direct2D backend and is interested in having WebKit support.
Comment 4 Takashi Komori 2018-11-07 04:42:02 PST
Created attachment 354083 [details]
Patch
Comment 5 EWS Watchlist 2018-11-07 05:00:35 PST
Attachment 354083 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerInjectedBundlePrefix.cpp:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/win/TestControllerWin.cpp:189:  bundleDir_utf is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/WebKitTestRunner/win/WebKitTestRunnerPrefix.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Takashi Komori 2018-11-07 18:36:11 PST
Created attachment 354192 [details]
Patch
Comment 7 EWS Watchlist 2018-11-07 18:49:42 PST
Attachment 354192 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerInjectedBundlePrefix.cpp:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/win/TestControllerWin.cpp:189:  bundleDir_utf is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/WebKitTestRunner/win/WebKitTestRunnerPrefix.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Takashi Komori 2018-11-07 19:17:24 PST
Created attachment 354199 [details]
Patch

Fix style check.
Comment 9 EWS Watchlist 2018-11-07 19:20:58 PST
Attachment 354199 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerInjectedBundlePrefix.cpp:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/win/WebKitTestRunnerPrefix.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Takashi Komori 2018-11-07 19:29:08 PST
We can't include config.h into TestRunnerInjectedBundlePrefix.cpp and WebKitTestRunnerPrefix.cpp because these source files are used for precompiled header.

This no config.h style is also used in DumpRenderTreePrefix.cpp and TestWebKitAPIPrefix.cpp for the same purpose.
Comment 11 Fujii Hironori 2018-11-07 20:29:03 PST
Comment on attachment 354199 [details]
Patch

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

> ChangeLog:3
> +        Implement WebKitTestRunner for WinCairo

The summary doesn't match. Did you use prepare-ChangeLog? Change either the ChangeLog or Bugzilla summary.

> Source/cmake/OptionsWin.cmake:87
> +    WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCESSIBILITY PRIVATE ON)

I want to know the reason why ENABLE_ACCESSIBILITY is needed.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:-28
> -

Do not remove this blank line.
https://webkit.org/code-style-guidelines/#include-others

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:32
> +

Do not add a blank line here.
https://webkit.org/code-style-guidelines/#include-others

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:882
> +#if USE(CF) && !PLATFORM(WIN_CAIRO)

Why do you need this guard?

> Tools/WebKitTestRunner/PlatformWin.cmake:29
> +    ${DERIVED_SOURCES_DIR}/WebKit/Interfaces

Remove ${DERIVED_SOURCES_DIR}/WebKit/Interfaces. This is WK1.

> Tools/WebKitTestRunner/PlatformWin.cmake:52
> +    ${WEBKIT_TESTRUNNER_BINDINGS_DIR}/JSWrapper.cpp

I want to know the reason why you remove JSWrapper.cpp.

> Tools/WebKitTestRunner/PlatformWin.cmake:60
> +    "${WEBKIT_TESTRUNNER_DIR}/win/WebKitTestRunnerPrefix.cpp"

You don't need these double-quote here. CMake string quoting rule is different from shell scripts.

> Tools/WebKitTestRunner/TestController.cpp:77
> +#endif

Original code was sorted alphabetically.
Move these code bellow in this case.

> Tools/WebKitTestRunner/TestController.cpp:881
> +#if !PLATFORM(COCOA) && !PLATFORM(WPE) && !PLATFORM(WIN)

#if PLATFORM(GTK)
because WKTextCheckerContinuousSpellCheckingEnabledStateChanged is a GTK-only API.

> Tools/WebKitTestRunner/TestInvocation.cpp:50
> +#endif

Original code was sorted alphabetically.
Move these code bellow in this case.

> Tools/WebKitTestRunner/win/TestControllerWin.cpp:179
> +    TCHAR exePath[MAX_PATH];

Theoretically you are right, but TCHAR is not used nowadays. Simply use wchar_t. Don't include tchar.h. Don't use _t* API.
Comment 13 Takashi Komori 2018-11-08 19:36:03 PST
>> Source/cmake/OptionsWin.cmake:87
>> +    WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCESSIBILITY PRIVATE ON)
>
>I want to know the reason why ENABLE_ACCESSIBILITY is needed.

It it for appending AccessiblityController.cpp into TestRunnerInjectedBundle project.
Because Windows ports add HAVE_ACCESSIBILITY definition automatically in Platform.h, if we don't append it we can't build AccessibilityControllerWin.cpp


>> Tools/WebKitTestRunner/PlatformWin.cmake:52
>> +    ${WEBKIT_TESTRUNNER_BINDINGS_DIR}/JSWrapper.cpp
>
>I want to know the reason why you remove JSWrapper.cpp.


WEBKIT_ADD_PRECOMPILED_HEADER overwrites ForcedIncludeFiles attribute (precompiled header), so we can't use this macro twice.
Because JSWrapper.cpp is reffered from two projects WebKitTestRunnerLib and WebKitTestRunnerInjectedBundle, building overwritten precompiled headers have a conflict.

JSWrapper.cpp doesn't require the header, so it is easy way to avoid this conflict.
Comment 14 Fujii Hironori 2018-11-08 20:12:37 PST
Wow. I didn't know that Windows port has macros HAVE_ACCESSIBILITY=1 and ENABLE_ACCESSIBILITY=0.
This has been filed. I will do it after your patch will land.

  Bug 21802 – HAVE_ACCESSIBILITY should probably be ENABLE_ACCESSIBILITY
Comment 15 Takashi Komori 2018-11-08 23:28:07 PST
>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:882
>> +#if USE(CF) && !PLATFORM(WIN_CAIRO)
>
>Why do you need this guard?

Because WebCoreTestSupport::createXMLStringFromWebArchiveData is implemented in Objective-C, we can't use the code in InjectedBundlePage::dumpDOMAsWebArchive function.

 Source/WebCore/testing/cocoa/WebArchiveDumpSupport.mm
Comment 16 Don Olmstead 2018-11-09 09:32:56 PST
Comment on attachment 354199 [details]
Patch

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

> Tools/WebKitTestRunner/win/TestControllerWin.cpp:31
> +#include <WebKit/WKStringCF.h>

bfulgham has stated that Apple intends to remove CoreFoundation in Windows. We should not introduce any CF code in anything windows.
Comment 17 Takashi Komori 2018-11-11 19:05:36 PST
Created attachment 354525 [details]
Patch
Comment 18 EWS Watchlist 2018-11-11 19:08:20 PST
Attachment 354525 [details] did not pass style-queue:


ERROR: Tools/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerInjectedBundlePrefix.cpp:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/win/WebKitTestRunnerPrefix.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Takashi Komori 2018-11-11 19:46:28 PST
Created attachment 354527 [details]
Patch
Comment 20 EWS Watchlist 2018-11-11 19:48:30 PST
Attachment 354527 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerInjectedBundlePrefix.cpp:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/win/WebKitTestRunnerPrefix.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Fujii Hironori 2018-11-11 21:25:32 PST
Comment on attachment 354527 [details]
Patch

Looks good to me. Please wait for a day before landing this patch for more review or objections.
Comment 22 Brent Fulgham 2018-11-12 09:47:34 PST
Comment on attachment 354527 [details]
Patch

Looks fine to me, too, and the EWS bots seem happy. I say land away!
Comment 23 WebKit Commit Bot 2018-11-12 10:58:09 PST
Comment on attachment 354527 [details]
Patch

Clearing flags on attachment: 354527

Committed r238098: <https://trac.webkit.org/changeset/238098>
Comment 24 WebKit Commit Bot 2018-11-12 10:58:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Radar WebKit Bug Importer 2018-11-12 10:59:24 PST
<rdar://problem/45995584>