Bug 103376 - [V8] add contextForWorld helper function to ScriptController
Summary: [V8] add contextForWorld helper function to ScriptController
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-27 01:09 PST by Dan Carney
Modified: 2012-11-27 09:22 PST (History)
4 users (show)

See Also:


Attachments
Patch (3.34 KB, patch)
2012-11-27 01:10 PST, Dan Carney
no flags Details | Formatted Diff | Diff
Patch (3.18 KB, patch)
2012-11-27 01:50 PST, Dan Carney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Carney 2012-11-27 01:09:18 PST
[V8] add contextForWorld helper function to ScriptController
Comment 1 Dan Carney 2012-11-27 01:10:42 PST
Created attachment 176195 [details]
Patch
Comment 2 Kentaro Hara 2012-11-27 01:16:53 PST
Comment on attachment 176195 [details]
Patch

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

> Source/WebCore/bindings/v8/ScriptController.cpp:437
> +inline v8::Local<v8::Context> ScriptController::contextForWorld(DOMWrapperWorld* world)

This could be in a header file. Maybe you couldn't write it in a header file due to some #include dependency problem ?
Comment 3 Dan Carney 2012-11-27 01:31:46 PST
(In reply to comment #2)
> (From update of attachment 176195 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176195&action=review
> 
> > Source/WebCore/bindings/v8/ScriptController.cpp:437
> > +inline v8::Local<v8::Context> ScriptController::contextForWorld(DOMWrapperWorld* world)
> 
> This could be in a header file. Maybe you couldn't write it in a header file due to some #include dependency problem ?

I will move it to the header, i just wanted all the uses of the function in the same spot as the function.
Comment 4 Kentaro Hara 2012-11-27 01:32:50 PST
ok, if performance is not critical, it would make sense to put it in a cpp file as a static function.
Comment 5 Dan Carney 2012-11-27 01:50:23 PST
Created attachment 176201 [details]
Patch
Comment 6 Dan Carney 2012-11-27 01:50:55 PST
Comment on attachment 176201 [details]
Patch

static inline version
Comment 7 WebKit Review Bot 2012-11-27 02:28:52 PST
Comment on attachment 176201 [details]
Patch

Rejecting attachment 176201 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

/mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/15016026
Comment 8 WebKit Review Bot 2012-11-27 03:53:37 PST
Comment on attachment 176201 [details]
Patch

Clearing flags on attachment: 176201

Committed r135846: <http://trac.webkit.org/changeset/135846>
Comment 9 WebKit Review Bot 2012-11-27 03:53:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Adam Barth 2012-11-27 09:22:33 PST
Comment on attachment 176201 [details]
Patch

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

> Source/WebCore/bindings/v8/ScriptController.cpp:437
> +static inline v8::Local<v8::Context> contextForWorld(ScriptController* scriptController, DOMWrapperWorld* world)

Minor style nit: We usually put functions like this at the top of the file, just after the namespace WebCore.

Also, my understanding is that static and inline are redundant here.