[Webkit-unassigned] [Bug 125600] Need ObjC APIs for some InjectedBundle classes.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Dec 11 19:14:33 PST 2013


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





--- Comment #4 from mitz at webkit.org <mitz at webkit.org>  2013-12-11 19:12:45 PST ---
(From update of attachment 219020)
View in context: https://bugs.webkit.org/attachment.cgi?id=219020&action=review

Looks good! I have one substantial comment so I won’t r+, and a few style comments.

> Source/WebKit2/Shared/Cocoa/APIObject.mm:157
> +    case Type::BundleFrame:
> +        wrapper = [WKWebProcessPlugInFrame alloc];
> +        break;
> +
> +    case Type::BundleScriptWorld:
> +        wrapper = [WKWebProcessPlugInScriptWorld alloc];
> +        break;
> +
> +    case Type::BundleHitTestResult:
> +        wrapper = [WKWebProcessPlugInHitTestResult alloc];
> +        break;
> +
> +    case Type::BundleNodeHandle:
> +        wrapper = [WKWebProcessPlugInNodeHandle alloc];
> +        break;
> +

We should try to keep these sorted by the API type (so BundleScriptWorld should be last in the group)

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFrame.h:32
> +#import <Foundation/Foundation.h>
> +#import <CoreGraphics/CoreGraphics.h>
> +#import <JavaScriptCore/JSContextRef.h>

These can also be sorted.

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFrame.h:41
> +- (JSGlobalContextRef)jsContextForWorld:(WKWebProcessPlugInScriptWorld *)world;

I believe that we should avoid the C JavaScript API types in our Cocoa API, so this method should return a (JSContext *).

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFrame.h:43
> +- (JSValueRef)jsWrapperForNodeHandle:(WKWebProcessPlugInNodeHandle *)nodeHandle inWorld:(WKWebProcessPlugInScriptWorld *)world;

Similarly, this should return a (JSValue *).

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFrame.mm:27
> +#import "config.h"
> +#import "WKWebProcessPlugInFrameInternal.h"

We normally leave a blank line after the first two imports.

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFrame.mm:34
> +#if WK_API_ENABLED
> +

I usually put this before the other imports (i.e. just after the blank line I mentioned). See for example WKRemoteObjectCoder.mm.

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFrame.mm:62
> +    InjectedBundleNodeHandle *injectedNodeHandle = static_cast<InjectedBundleNodeHandle*>(&[nodeHandle _apiObject]);
> +    InjectedBundleScriptWorld *injectedWorld = static_cast<InjectedBundleScriptWorld*>(&[world _apiObject]);

We should add internal methods to WKWebProcessPlugInNodeHandle and WKWebProcessPlugInScriptWorld that return an InjectedBundleNodeHandle& and an InjectedBundleScriptWorld&, respectively, instead of doing the casting here.

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInHitTestResult.h:37
> + at property (readonly) WKWebProcessPlugInNodeHandle* nodeHandle;

The space should go on the other hand of the star.

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInHitTestResult.mm:30
> +#import "config.h"
> +#import "WKWebProcessPlugInHitTestResultInternal.h"
> +#import "WKWebProcessPlugInNodeHandleInternal.h"
> +
> +#if WK_API_ENABLED

Same comments about the import block.

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInHitTestResultInternal.h:31
> +#import "WKObject.h"
> +#import "InjectedBundleHitTestResult.h"

I < W

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandle.h:34
> +

Probably don’t need this blank line.

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandleInternal.h:31
> +#import "WKObject.h"
> +#import "InjectedBundleNodeHandle.h"

W > I

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInScriptWorld.h:36
> ++ (WKWebProcessPlugInScriptWorld*)world;
> ++ (WKWebProcessPlugInScriptWorld*)normalWorld;

Missing spaces before the stars.

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInScriptWorldInternal.h:31
> +#import "WKObject.h"
> +#import "InjectedBundleScriptWorld.h"

I < W

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


More information about the webkit-unassigned mailing list