Bug 98222 - Add Objective-C API for the InjectedBundle
Summary: Add Objective-C API for the InjectedBundle
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-02 18:18 PDT by Sam Weinig
Modified: 2012-10-02 21:11 PDT (History)
0 users

See Also:


Attachments
Patch (30.20 KB, patch)
2012-10-02 20:28 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (30.23 KB, patch)
2012-10-02 21:07 PDT, Sam Weinig
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2012-10-02 18:18:41 PDT
Add Objective-C API for the InjectedBundle
Comment 1 Sam Weinig 2012-10-02 20:28:05 PDT
Created attachment 166801 [details]
Patch
Comment 2 Anders Carlsson 2012-10-02 20:39:01 PDT
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.
Comment 3 Sam Weinig 2012-10-02 20:42:26 PDT
(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.
Comment 4 Sam Weinig 2012-10-02 21:07:35 PDT
Created attachment 166804 [details]
Patch
Comment 5 Sam Weinig 2012-10-02 21:11:59 PDT
Committed r130245: <http://trac.webkit.org/changeset/130245>