[Webkit-unassigned] [Bug 48027] Add ability to test injected bundle API using TestWebKitAPI

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48027


Adam Roben (aroben) <aroben at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #71366|review?                     |review+
               Flag|                            |




--- Comment #3 from Adam Roben (aroben) <aroben at apple.com>  2010-10-20 17:10:18 PST ---
(From update of attachment 71366)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.