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

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Yongjun Zhang <yongjun_zhang at apple.com>  2013-12-12 11:00:38 PST ---
(In reply to comment #4)
> (From update of attachment 219020 [details])
> 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 *).

Good point!  I will change to use JavaScript Obj C API.

Currently the caller site of jsContextForWorld still expects JavaScript C API.  For now, I will convert JSContext* to JSContextRef in there, and do the whole conversion later.

> 
> > 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.