Bug 148317 – [Cocoa] API tests using the Modern WebKit API should be able to create web process plug-ins

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

[Cocoa] API tests using the Modern WebKit API should be able to create web process plug-ins

[Andy Estes]

Created attachment 259644 [details]
Patch

[WebKit Commit Bot]

Attachment 259644 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:59:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:65:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Created attachment 259651 [details]
Patch

[WebKit Commit Bot]

Attachment 259651 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:59:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:65:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Created attachment 259687 [details]
Patch

[WebKit Commit Bot]

Attachment 259687 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:56:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:67:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Created attachment 259736 [details]
Patch

[WebKit Commit Bot]

Attachment 259736 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:44:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:54:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:65:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:75:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 259736 [details]
Patch

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

> Tools/ChangeLog:10
> +        WKProcessPool returned by processPoolWithTestPlugInPrincipalClassName() when creating WKWebViews.

This comment is outdated. processPoolWithTestPlugInPrincipalClassName() doesn’t exist in this patch.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:35
> +static bool isDone = false;

No need to initialize statics to false, 0, etc.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:45
> +            EXPECT_FALSE(!!result);

Could use EXPECT_NULL here.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:56
> +            EXPECT_TRUE([result isEqualToString:testString]);

Could use EXPECT_STREQ here.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:76
> +            EXPECT_FALSE(!!result);

EXPECT_NULL?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:83
> +

Another thing you could test here is that setting a bundle parameter before creating the WKWebView also works.

> Tools/TestWebKitAPI/cocoa/WebProcessPlugIn/WebProcessPlugIn.mm:68
> +- (void)webProcessPlugIn:(WKWebProcessPlugInController *)plugInController didCreateBrowserContextController:(WKWebProcessPlugInBrowserContextController *)browserContextController
> +{
> +    if ([_testPlugIn respondsToSelector:@selector(webProcessPlugIn:didCreateBrowserContextController:)])
> +        [_testPlugIn webProcessPlugIn:plugInController didCreateBrowserContextController:browserContextController];
> +}
> +
> +- (void)webProcessPlugIn:(WKWebProcessPlugInController *)plugInController willDestroyBrowserContextController:(WKWebProcessPlugInBrowserContextController *)browserContextController
> +{
> +    if ([_testPlugIn respondsToSelector:@selector(webProcessPlugIn:willDestroyBrowserContextController:)])
> +        [_testPlugIn webProcessPlugIn:plugInController willDestroyBrowserContextController:browserContextController];
> +}

You might be able to use a generic forwarding mechanism like -forwardingTargetForSelector: instead of implementing every present and future method in the protocol.

[Andy Estes]

(In reply to comment #9)
> Comment on attachment 259736 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259736&action=review
> 
> > Tools/ChangeLog:10
> > +        WKProcessPool returned by processPoolWithTestPlugInPrincipalClassName() when creating WKWebViews.
> 
> This comment is outdated. processPoolWithTestPlugInPrincipalClassName()
> doesn’t exist in this patch.
> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:35
> > +static bool isDone = false;
> 
> No need to initialize statics to false, 0, etc.
> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:45
> > +            EXPECT_FALSE(!!result);
> 
> Could use EXPECT_NULL here.
> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:56
> > +            EXPECT_TRUE([result isEqualToString:testString]);
> 
> Could use EXPECT_STREQ here.
> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:76
> > +            EXPECT_FALSE(!!result);
> 
> EXPECT_NULL?
> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleParameters.mm:83
> > +
> 
> Another thing you could test here is that setting a bundle parameter before
> creating the WKWebView also works.
> 
> > Tools/TestWebKitAPI/cocoa/WebProcessPlugIn/WebProcessPlugIn.mm:68
> > +- (void)webProcessPlugIn:(WKWebProcessPlugInController *)plugInController didCreateBrowserContextController:(WKWebProcessPlugInBrowserContextController *)browserContextController
> > +{
> > +    if ([_testPlugIn respondsToSelector:@selector(webProcessPlugIn:didCreateBrowserContextController:)])
> > +        [_testPlugIn webProcessPlugIn:plugInController didCreateBrowserContextController:browserContextController];
> > +}
> > +
> > +- (void)webProcessPlugIn:(WKWebProcessPlugInController *)plugInController willDestroyBrowserContextController:(WKWebProcessPlugInBrowserContextController *)browserContextController
> > +{
> > +    if ([_testPlugIn respondsToSelector:@selector(webProcessPlugIn:willDestroyBrowserContextController:)])
> > +        [_testPlugIn webProcessPlugIn:plugInController willDestroyBrowserContextController:browserContextController];
> > +}
> 
> You might be able to use a generic forwarding mechanism like
> -forwardingTargetForSelector: instead of implementing every present and
> future method in the protocol.

This worked, but I also had to override -respondsToSelector:

- (BOOL)respondsToSelector:(SEL)aSelector
{
    if ([_testPlugIn respondsToSelector:aSelector])
        return YES;
    return [super respondsToSelector:aSelector];
}

Thanks for the review!

[Andy Estes]

Committed r188844: <http://trac.webkit.org/changeset/188844>

[Csaba Osztrogonác]

(In reply to comment #11)
> Committed r188844: <http://trac.webkit.org/changeset/188844>

It broke the build on the Apple Yosemite 32 bit bot:
https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%2832-bit%20Build%29/builds/5451

[Andy Estes]

(In reply to comment #12)
> (In reply to comment #11)
> > Committed r188844: <http://trac.webkit.org/changeset/188844>
> 
> It broke the build on the Apple Yosemite 32 bit bot:
> https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%2832-
> bit%20Build%29/builds/5451

Should be fixed in r188845: http://trac.webkit.org/changeset/188845