Bug 125600 – Need ObjC APIs for some InjectedBundle classes.

• Component: WebKit2
• Status: RESOLVED FIXED

[Yongjun Zhang]

We will need Objective C APIs for some InjectBundle classes.  We can start with WebFrame, InjectedBundleScriptWorld, InjectedBundleHitTestResult and InjectedBundleNodeHandle.

[Yongjun Zhang]

Created attachment 219005 [details]
Add ObjC API classes for WebFrame, InjectedBundleScriptWorld, InjectedBundleHitTestResult and InjectedBundleNodeHandle.

[WebKit Commit Bot]

Attachment 219005 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/Cocoa/APIObject.mm', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFrame.h', u'Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFrame.mm', u'Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFrameInternal.h', u'Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInHitTestResult.h', u'Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInHitTestResult.mm', u'Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInHitTestResultInternal.h', u'Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandle.h', u'Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandle.mm', u'Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandleInternal.h', u'Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInScriptWorld.h', u'Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInScriptWorld.mm', u'Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInScriptWorldInternal.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandleInternal.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandleInternal.h:34:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInScriptWorldInternal.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInScriptWorldInternal.h:34:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInHitTestResultInternal.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInHitTestResultInternal.h:34:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFrameInternal.h:35:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 7 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yongjun Zhang]

Created attachment 219020 [details]
Fix style issue, and autorelease wrapper objects before returning to caller.

Comment on attachment 219020 [details]
Fix style issue, and autorelease wrapper objects before returning to caller.

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
> +@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

[Sam Weinig]

Comment on attachment 219020 [details]
Fix style issue, and autorelease wrapper objects before returning to caller.

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

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

Yeah, this should use the Objective-C JS API.

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFrame.h:42
> +- (WKWebProcessPlugInHitTestResult *)hitTest:(CGPoint)point;

Can we put HitTest on the browsing controller instead?

[Yongjun Zhang]

(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
> > +@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

[Yongjun Zhang]

(In reply to comment #5)
> (From update of attachment 219020 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219020&action=review
> 
> >> 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 *).
> 
> Yeah, this should use the Objective-C JS API.
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFrame.h:42
> > +- (WKWebProcessPlugInHitTestResult *)hitTest:(CGPoint)point;
> 
> Can we put HitTest on the browsing controller instead?

Looks like hitTest is for a frame, not a page.  If we put in browsing controller, we will lose the ability to do hitTest on a subframe unless we change the API to something like:

[WKWebProcessPlugInBrowsingContextController hitTest:inFrame:].

[Yongjun Zhang]

Created attachment 219103 [details]
address review comments, using JavaScript Obj C API intend of C API.

[WebKit Commit Bot]

Comment on attachment 219103 [details]
address review comments, using JavaScript Obj C API intend of C API.

Rejecting attachment 219103 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 219103, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
coa/WKWebProcessPlugInNodeHandleInternal.h
patching file Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInScriptWorld.h
patching file Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInScriptWorld.mm
patching file Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInScriptWorldInternal.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Dan Bernstein']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/48678025

[Yongjun Zhang]

Created attachment 219159 [details]
Fix a conflict in WebKit2 project file before landing.

[WebKit Commit Bot]

Comment on attachment 219159 [details]
Fix a conflict in WebKit2 project file before landing.

Clearing flags on attachment: 219159

Committed r160530: <http://trac.webkit.org/changeset/160530>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.