Bug 189257 – Resurrect WebKitTestRunner for Windows port

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Fujii Hironori]

Resurrect WebKitTestRunner for Windows port

It has been removed in Bug 123152.

[Takashi Komori]

Created attachment 353945 [details]
Patch

[EWS Watchlist]

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.

[Don Olmstead]

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.

[Takashi Komori]

Created attachment 354083 [details]
Patch

[EWS Watchlist]

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.

[Takashi Komori]

Created attachment 354192 [details]
Patch

[EWS Watchlist]

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.

[Takashi Komori]

Created attachment 354199 [details]
Patch

Fix style check.

[EWS Watchlist]

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.

[Takashi Komori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

A link for the original code.
https://github.com/WebKit/webkit/tree/a7fa8412b48e21125b826e47261540a2243ba8a5/Tools/WebKitTestRunner

[Takashi Komori]

>> 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.

[Fujii Hironori]

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

[Takashi Komori]

>> 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

[Don Olmstead]

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.

[Takashi Komori]

Created attachment 354525 [details]
Patch

[EWS Watchlist]

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.

[Takashi Komori]

Created attachment 354527 [details]
Patch

[EWS Watchlist]

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.

[Fujii Hironori]

Comment on attachment 354527 [details]
Patch

Looks good to me. Please wait for a day before landing this patch for more review or objections.

[Brent Fulgham]

Comment on attachment 354527 [details]
Patch

Looks fine to me, too, and the EWS bots seem happy. I say land away!

[WebKit Commit Bot]

Comment on attachment 354527 [details]
Patch

Clearing flags on attachment: 354527

Committed r238098: <https://trac.webkit.org/changeset/238098>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45995584>