[webkit-reviews] review granted: [Bug 48027] Add ability to test injected bundle API using TestWebKitAPI : [Attachment 71366] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Sam Weinig
<sam at webkit.org>'s request for review:
Bug 48027: Add ability to test injected bundle API using TestWebKitAPI
https://bugs.webkit.org/show_bug.cgi?id=48027

Attachment 71366: Patch
https://bugs.webkit.org/attachment.cgi?id=71366&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
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.