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

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

Summary: [Cocoa] API tests using the Modern WebKit API should be able to create web pr...

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	New Bugs (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Andy Estes

URL:
Keywords:

Depends on:
Blocks:	147872
	Show dependency tree / graph

Reported:	2015-08-21 11:56 PDT by Andy Estes
Modified:	2015-08-23 15:52 PDT (History)
CC List:	7 users (show)

See Also:

Attachments
Patch (34.15 KB, patch) 2015-08-21 12:40 PDT, Andy Estes	no flags	Details \| Formatted Diff \| Diff
Patch (34.92 KB, patch) 2015-08-21 13:40 PDT, Andy Estes	no flags	Details \| Formatted Diff \| Diff
Patch (35.28 KB, patch) 2015-08-21 17:29 PDT, Andy Estes	no flags	Details \| Formatted Diff \| Diff
Patch (35.33 KB, patch) 2015-08-22 22:25 PDT, Andy Estes	mitz: review+	Details \| Formatted Diff \| Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Andy Estes 2015-08-21 11:56:59 PDT

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

Comment 1 Andy Estes 2015-08-21 12:40:08 PDT

Created attachment 259644 [details]
Patch

Comment 2 WebKit Commit Bot 2015-08-21 12:42:01 PDT

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.

Comment 3 Andy Estes 2015-08-21 13:40:46 PDT

Created attachment 259651 [details]
Patch

Comment 4 WebKit Commit Bot 2015-08-21 13:43:39 PDT

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.

Comment 5 Andy Estes 2015-08-21 17:29:31 PDT

Created attachment 259687 [details]
Patch

Comment 6 WebKit Commit Bot 2015-08-21 17:31:57 PDT

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.

Comment 7 Andy Estes 2015-08-22 22:25:24 PDT

Created attachment 259736 [details]
Patch

Comment 8 WebKit Commit Bot 2015-08-22 22:27:01 PDT

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 9 mitz 2015-08-23 08:42:31 PDT

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.

Comment 10 Andy Estes 2015-08-23 15:24:57 PDT

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

Comment 11 Andy Estes 2015-08-23 15:31:43 PDT

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

Comment 12 Csaba Osztrogonác 2015-08-23 15:39:24 PDT

(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

Comment 13 Andy Estes 2015-08-23 15:52:09 PDT

(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