Bug 48027 – Add ability to test injected bundle API using TestWebKitAPI

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Sam Weinig]

Add ability to test injected bundle API using TestWebKitAPI

[Sam Weinig]

Created attachment 71361 [details]
Patch

[Sam Weinig]

Created attachment 71366 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 71366 [details]
Patch

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

> WebKitTools/TestWebKitAPI/InjectedBundleMain.cpp:33
> +#if defined(WIN32) || defined(_WIN32)
> +extern "C" __declspec(dllexport) 
> +#else
> +extern "C"
> +#endif

Maybe WKBundleInitialize.h should define a macro that clients can use to do this correctly.

> WebKitTools/TestWebKitAPI/InjectedBundleTest.h:41
> +    virtual void didCreatePage(WKBundleRef bundle, WKBundlePageRef page) { }
> +    virtual void willDestroyPage(WKBundleRef bundle, WKBundlePageRef page) { }
> +    virtual void didReceiveMessage(WKBundleRef bundle, WKStringRef messageName, WKTypeRef messageBody) { }

There are some unnecessary parameter names in here.

> WebKitTools/TestWebKitAPI/InjectedBundleTest.h:43
> +    std::string name() const { return m_identifier; }

Could use const std::string&.

> WebKitTools/TestWebKitAPI/InjectedBundleTest.h:45
> +    template<typename TestClassTy> class Register {

Registrar seems more accurate than Register. Does "Ty" really make the template parameter clearer?

> WebKitTools/TestWebKitAPI/PlatformUtilities.h:42
> +WKContextRef createContextForInjectedBundleTest(const std::string&);
> +
> +WKStringRef createInjectedBundlePath();
>  WKURLRef createURLForResource(const char* resource, const char* extension);
>  WKURLRef URLForNonExistentResource();

It would be nice to make these return WKRetainPtrs someday.

> WebKitTools/TestWebKitAPI/Tests/WebKit2/InjectedBundleBasic_Bundle.cpp:47
> +static InjectedBundleTest::Register<InjectedBundleBasicTest> injectedBundleBasicTestRegistrar("InjectedBundleBasicTest");

This variable could just be called "registrar".

> WebKitTools/TestWebKitAPI/Tests/WebKit2/InjectedBundleBasic_Bundle.cpp:50
> +} // namespace TestWebKitAPI
>  \ No newline at end of file

Please add a newline.

> WebKitTools/TestWebKitAPI/win/PlatformUtilitiesWin.cpp:65
> +WKStringRef createInjectedBundlePath()
> +{
> +    RetainPtr<CFURLRef> executableURL(AdoptCF, CFBundleCopyExecutableURL(CFBundleGetMainBundle()));
> +    RetainPtr<CFURLRef> executableContainerURL(AdoptCF, CFURLCreateCopyDeletingLastPathComponent(0, executableURL.get()));
> +    RetainPtr<CFStringRef> executableContainerPath(AdoptCF, CFURLCopyFileSystemPath(executableContainerURL.get(), kCFURLWindowsPathStyle));
> +    RetainPtr<CFMutableStringRef> bundlePath(AdoptCF, CFStringCreateMutableCopy(0, 0, executableContainerPath.get()));
> +    CFStringAppendCString(bundlePath.get(), injectedBundleDLL, kCFStringEncodingWindowsLatin1);
> +    return WKStringCreateWithCFString(bundlePath.get());
> +}

Using CFURLCreateCopyAppendingPathComponent would be nicer than using a mutable string. This could also be done with Win32 API calls instead.

[Sam Weinig]

Landed in r70194.