Add Objective-C API for the InjectedBundle

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Add Objective-C API for the InjectedBundle

[Sam Weinig]

Created attachment 166801 [details]
Patch

[Anders Carlsson]

Comment on attachment 166801 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugIn.mm:52
> ++ (void)_initializeSharedWithPrincipalClassInstance:(id<WKWebProcessPlugInDelegate>)principalClassInstance injectedBundle:(WebKit::InjectedBundle*)injectedBundle
> +{
> +    ASSERT_WITH_MESSAGE(!sharedInstance, "+[WKWebProcessPlugIn _initializeSharedWithPrincipalClassInstance:injectedBundle:] called multiple times");
> +
> +    sharedInstance = [[self alloc] _initWithPrincipalClassInstance:principalClassInstance injectedBundle:injectedBundle];
> +}

I think a better pattern for this singleton would be for the first call to the init instance method to set up the shared instance. Then you wouldn't need this method at all.

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInInternal.h:37
> +@interface WKWebProcessPlugIn ()
> +{
> +    RetainPtr<id<WKWebProcessPlugInDelegate> > _principalClassInstance;
> +    RefPtr<WebKit::InjectedBundle> _injectedBundle;
> +}

It looks like you're only using these from the .mm file, so you could just put them in the @implementation instead.

> Source/WebKit2/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:94
> +    // First check to see if the bundle has a WKBundleInitialize function.
> +    WKBundleInitializeFunctionPtr initializeFunction = reinterpret_cast<WKBundleInitializeFunctionPtr>(CFBundleGetFunctionPointerForName([m_platformBundle _cfBundle], CFSTR("WKBundleInitialize")));
> +    if (initializeFunction) {
> +        initializeFunction(toAPI(this), toAPI(initializationUserData));
> +        return true;
> +    }
> +    
> +    // Otherwise, look to see if the bundle has a principal class
> +    Class principalClass = [m_platformBundle principalClass];
> +    if (!principalClass) {
> +        WTFLogAlways("InjectedBundle::load failed - No initialize function or principal class found in the bundle executable.");
>          return false;
>      }
>  

I think we should prefer the class first and the WKBundleInitialize call second.

> Source/WebKit2/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:109
> +    if ([instance respondsToSelector:@selector(webProcessPlugInInitialize:)])
> +        [instance webProcessPlugInInitialize:[WKWebProcessPlugIn _shared]];

This makes me think that WKWebProcessPlugIn should really be called the WKWebProcessPlugInController, and the protocol should be the WKWebProcessPlugIn since that's what you actually implement.

[Sam Weinig]

(In reply to comment #2)
> (From update of attachment 166801 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166801&action=review
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugIn.mm:52
> > ++ (void)_initializeSharedWithPrincipalClassInstance:(id<WKWebProcessPlugInDelegate>)principalClassInstance injectedBundle:(WebKit::InjectedBundle*)injectedBundle
> > +{
> > +    ASSERT_WITH_MESSAGE(!sharedInstance, "+[WKWebProcessPlugIn _initializeSharedWithPrincipalClassInstance:injectedBundle:] called multiple times");
> > +
> > +    sharedInstance = [[self alloc] _initWithPrincipalClassInstance:principalClassInstance injectedBundle:injectedBundle];
> > +}
> 
> I think a better pattern for this singleton would be for the first call to the init instance method to set up the shared instance. Then you wouldn't need this method at all.

Is there another example of someone doing that?  It seems really weird to me.

> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInInternal.h:37
> > +@interface WKWebProcessPlugIn ()
> > +{
> > +    RetainPtr<id<WKWebProcessPlugInDelegate> > _principalClassInstance;
> > +    RefPtr<WebKit::InjectedBundle> _injectedBundle;
> > +}
> 
> It looks like you're only using these from the .mm file, so you could just put them in the @implementation instead.

Yeah, I actually can't do this since it will break the fragile base class build.  I'll move them to a WKWebProcessPlugInData object like the rest of the API.

> 
> > Source/WebKit2/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:94
> > +    // First check to see if the bundle has a WKBundleInitialize function.
> > +    WKBundleInitializeFunctionPtr initializeFunction = reinterpret_cast<WKBundleInitializeFunctionPtr>(CFBundleGetFunctionPointerForName([m_platformBundle _cfBundle], CFSTR("WKBundleInitialize")));
> > +    if (initializeFunction) {
> > +        initializeFunction(toAPI(this), toAPI(initializationUserData));
> > +        return true;
> > +    }
> > +    
> > +    // Otherwise, look to see if the bundle has a principal class
> > +    Class principalClass = [m_platformBundle principalClass];
> > +    if (!principalClass) {
> > +        WTFLogAlways("InjectedBundle::load failed - No initialize function or principal class found in the bundle executable.");
> >          return false;
> >      }
> >  
> 
> I think we should prefer the class first and the WKBundleInitialize call second.

I can't as that would break Safari.  principalClass almost always returns non-nil.

> 
> > Source/WebKit2/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:109
> > +    if ([instance respondsToSelector:@selector(webProcessPlugInInitialize:)])
> > +        [instance webProcessPlugInInitialize:[WKWebProcessPlugIn _shared]];
> 
> This makes me think that WKWebProcessPlugIn should really be called the WKWebProcessPlugInController, and the protocol should be the WKWebProcessPlugIn since that's what you actually implement.

I'll rename it.

[Sam Weinig]

Created attachment 166804 [details]
Patch

[Sam Weinig]

Committed r130245: <http://trac.webkit.org/changeset/130245>