Bug 146358 - ScriptMessageHandlerDelegate::didPostMessage() should reuse its JSContext instance
Summary: ScriptMessageHandlerDelegate::didPostMessage() should reuse its JSContext ins...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-06-26 12:36 PDT by Mark Lam
Modified: 2015-06-26 16:47 PDT (History)
5 users (show)

See Also:


Attachments
the patch. (3.45 KB, patch)
2015-06-26 13:07 PDT, Mark Lam
darin: review+
Details | Formatted Diff | Diff
patch 2: use a static JSContext. (3.40 KB, patch)
2015-06-26 14:22 PDT, Mark Lam
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2015-06-26 12:36:37 PDT
Currently, ScriptMessageHandlerDelegate::didPostMessage() creates a new JSContext on every invocation.  It would be more efficient to just create it once and reuse it for all subsequent calls.
Comment 1 Mark Lam 2015-06-26 12:37:09 PDT
<rdar://problem/21349359>
Comment 2 Mark Lam 2015-06-26 13:07:06 PDT
Created attachment 255662 [details]
the patch.
Comment 3 Mark Lam 2015-06-26 13:08:42 PDT
Comment on attachment 255662 [details]
the patch.

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

> Source/WebKit2/ChangeLog:9
> +        time it is called.  This is JSContext is used only once to deserialized a JSON object

typo: extra "is" before JSContext.  Will fix this before landing.
Comment 4 Mark Lam 2015-06-26 14:22:34 PDT
Created attachment 255665 [details]
patch 2: use a static JSContext.

Sam pointed out offline that didPostMessage() is always called from the UI thread, and we only need a static instance if the JSContext.  Updated the patch to reflect this.
Comment 5 Anders Carlsson 2015-06-26 14:47:48 PDT
Comment on attachment 255665 [details]
patch 2: use a static JSContext.

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

> Source/WebKit2/UIProcess/API/Cocoa/WKUserContentController.mm:97
> +            static JSContext* context = nil;
> +            if (!context)
> +                context = [[JSContext alloc] init];

You can just assign to the variable directly.

> Source/WebKit2/UIProcess/API/Cocoa/WKUserContentController.mm:101
> +            id body = [value toObject];

value.toObject?

> Source/WebKit2/UIProcess/API/Cocoa/WKUserContentController.mm:103
> +
> +            RetainPtr<WKScriptMessage> message = adoptNS([[WKScriptMessage alloc] _initWithBody:body webView:fromWebPageProxy(page) frameInfo:frameInfo.get() name:m_name.get()]);

I'd use auto here.
Comment 6 Mark Lam 2015-06-26 15:17:21 PDT
Thanks for the reviews.  Ander's feedback as been applied.  Landed in r186010: <http://trac.webkit.org/r186010>.
Comment 7 Mark Lam 2015-06-26 16:16:41 PDT
Some build bots were not liking the #import <WTF/MainThread.h>.  I removed it for now in r186014: <http://trac.webkit.org/r186014> to green the bots while I investigate.
Comment 8 Mark Lam 2015-06-26 16:47:47 PDT
#inport and assertion re-added in r186017: <http://trac.webkit.org/r186017>.