Bug 94593 – [V8] Move context() from V8Proxy to ScriptController

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

To kill V8Proxy, we can move context() from V8Proxy to ScriptController.

[Kentaro Hara]

Created attachment 159677 [details]
Patch

[Adam Barth]

Comment on attachment 159677 [details]
Patch

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

Let's iterate on this patch for a bit.  I wonder if we can get rid of ScriptController::context entirely and use the less-magical frame->script()->context(), which we might want to rename to contextForCurrentWorld() to mirror contextForMainWorld().

> Source/WebCore/bindings/scripts/test/V8/V8TestActiveDOMObject.cpp:194
> -        context = proxy->context();
> +        context = proxy->frame()->script()->context();

We should re-write this function to not use V8Proxy anymore.

> Source/WebCore/bindings/v8/ScriptController.cpp:379
> +v8::Local<v8::Context> ScriptController::context(Frame* frame)

How is this function different from:

return frame ? frame->script()->context() : v8::Local<v8::Context>();

?

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:138
> -    v8::Handle<v8::Context> context = V8Proxy::context(frame);
> +    v8::Handle<v8::Context> context = ScriptController::context(frame);

We already know that frame is non-0 at this point, so can't we just use frame->script()->context() ?

> Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:156
> -    v8::Local<v8::Context> context = V8Proxy::context(frame);
> +    v8::Local<v8::Context> context = ScriptController::context(frame);

Here we also know that frame is non-0 because shouldAllowAccessToFrame returns false for 0 frames.

> Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:177
> -    v8::Local<v8::Context> context = V8Proxy::context(frame);
> +    v8::Local<v8::Context> context = ScriptController::context(frame);

Ditto

> Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:623
> -    v8::Handle<v8::Context> context = V8Proxy::context(frame);
> +    v8::Handle<v8::Context> context = ScriptController::context(frame);

Here also frame is non-0

[Kentaro Hara]

Created attachment 159849 [details]
Patch

[Kentaro Hara]

(In reply to comment #2)
> > Source/WebCore/bindings/v8/ScriptController.cpp:379
> > +v8::Local<v8::Context> ScriptController::context(Frame* frame)
> 
> How is this function different from:
> 
> return frame ? frame->script()->context() : v8::Local<v8::Context>();

Same.

- I removed ScriptController::context(Frame*).

- I renamed ScriptController::context() to ScriptController::currentWorldContext().

- I used frame->script()->currentWorldContext() where frame exists. I used frame ? frame->script()->currentWorldContext() : v8::Local<v8::Context>() where frame might not exist.


> > Source/WebCore/bindings/scripts/test/V8/V8TestActiveDOMObject.cpp:194
> > -        context = proxy->context();
> > +        context = proxy->frame()->script()->context();
> 
> We should re-write this function to not use V8Proxy anymore.

I found that simple rewriting crashes a couple of tests. Let me work on it in a follow-up patch.

[Adam Barth]

> I found that simple rewriting crashes a couple of tests. Let me work on it in a follow-up patch.

Sounds good.  Thanks.

[Adam Barth]

Comment on attachment 159849 [details]
Patch

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

> Source/WebCore/bindings/v8/ScriptController.h:159
>      static v8::Local<v8::Context> mainWorldContext(Frame*);

This is another function we should consider removing in a similar way.  (In a later patch.)

[WebKit Review Bot]

Comment on attachment 159849 [details]
Patch

Rejecting attachment 159849 [details] from commit-queue.

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

Last 500 characters of output:
): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Reduce Font.h includes across project -- improves RenderObject.h compile time

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/13571386

[WebKit Review Bot]

Comment on attachment 159849 [details]
Patch

Clearing flags on attachment: 159849

Committed r126363: <http://trac.webkit.org/changeset/126363>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.