Adam Barth <abarth at webkit.org> has denied Dan Carney <dcarney at google.com>'s
request for review:
Bug 95735: Removed V8IsolatedContext
https://bugs.webkit.org/show_bug.cgi?id=95735
Attachment 162013: Patch
https://bugs.webkit.org/attachment.cgi?id=162013&action=review
------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=162013&action=review
This is a big step forward. Below are a bunch of minor comments. The only one
that's really important is the question about line 58 of
WorldContextHandle.cpp. I'd like to see another iteration of this patch, if
that ok with you.
> Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:98
> + if (worldId == mainWorldId) {
> + ASSERT_NOT_REACHED();
> + return mainThreadNormalWorld();
> + }
Why not leave this as an ASSERT?
> Source/WebCore/bindings/v8/ScriptController.cpp:329
> + if (UNLIKELY(worldId == DOMWrapperWorld::mainWorldId)) {
> + ASSERT_NOT_REACHED();
> + return m_windowShell;
> + }
Why not just ASSERT this?
> Source/WebCore/bindings/v8/ScriptController.cpp:352
> + if (UNLIKELY(!world)) {
> + ASSERT_NOT_REACHED();
> + return 0;
> + }
This should just be an ASSERT.
> Source/WebCore/bindings/v8/ScriptController.cpp:376
> + // Except in the test runner, worldID should be non 0 as it conflicts
with the mainWorldId.
> + if (UNLIKELY(!worldID))
> + worldID = DOMWrapperWorld::uninitializedWorldId;
Should we change the test runner so that the testing environment is more like
the production environment?
> Source/WebCore/bindings/v8/ScriptController.h:200
> + typedef HashMap<int, RefPtr<V8DOMWindowShell> > IsolatedWorldMap;
> + typedef HashMap<int, RefPtr<SecurityOrigin> >
IsolatedWorldSecurityOriginMap;
I wonder if we should move the SecurityOrigin pointer into V8DOMWindowShell.
We can do that in a separate patch though.
> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:234
> + if (UNLIKELY(m_context.get().IsWeak())) {
> + ASSERT_NOT_REACHED();
> + return;
> + }
ASSERT
> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:253
> + if (m_world->isMainWorld()) {
Do we ever call this function with weak and !m_world->isMainWorld() ?
> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:525
> + if (!m_world->isMainWorld()) {
> + ASSERT_NOT_REACHED();
> + return;
> + }
ASSERT
> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:560
> + if (!m_world->isMainWorld()) {
> + ASSERT_NOT_REACHED();
> + return;
> + }
ASSERT
> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:564
> + // FIXME: This shouldn't be possible anymore.
> + if (!m_frame->document())
> + return;
Is this a bad merge? I thought I deleted this code from trunk.
> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:594
> + if (!m_world->isMainWorld()) {
> + ASSERT_NOT_REACHED();
> + return;
> + }
ASSERT
> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:612
> + if (!m_world->isMainWorld()) {
> + ASSERT_NOT_REACHED();
> + return;
> + }
ASSERT
> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:633
> + if (!m_world->isMainWorld()) {
> + ASSERT_NOT_REACHED();
> + return;
> + }
ASSERT
> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:646
> + if (m_world->isMainWorld()) {
> + ASSERT_NOT_REACHED();
> + return;
> + }
ASSERT
> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:651
> + InspectorInstrumentation::didCreateIsolatedContext(m_frame,
scriptState, securityOrigin.get());
Why does setIsolatedWorldSecurityOrigin call didCreateIsolatedContext ? I
would assume that would be the responsibility of the create function, not of a
function that just sets the security origin.
> Source/WebCore/bindings/v8/V8DOMWindowShell.h:132
> + v8::Persistent<v8::Context> createNewContext(v8::Handle<v8::Object>
global);
createNewContext -> createContext
Why does this function return a v8::Persistent<v8::Context> ? It seems like
the result could just be stored in m_context.
> Source/WebCore/bindings/v8/V8DOMWindowShell.h:133
> + bool installDOMWindow(v8::Handle<v8::Context>, DOMWindow*);
Now that this function isn't static, it doesn't need to take a
v8::Handle<v8::Context> parameter. It can just use m_context.
> Source/WebCore/bindings/v8/V8DOMWindowShell.h:146
> + RefPtr<SecurityOrigin> m_isolatedWorldShellSecurityOrigin;
So, we keep a SecurityOrigin pointer here and in ScriptController? Do we need
to have both of them? Do they play different roles?
> Source/WebCore/bindings/v8/WorldContextHandle.cpp:53
> + ASSERT_NOT_REACHED();
Please just use ASSERT rather than trying to handle these impossible cases.
> Source/WebCore/bindings/v8/WorldContextHandle.cpp:58
> + m_context =
SharedPersistent<v8::Context>::create(v8::Persistent<v8::Context>::New(context)
);
Here is the v8::Persistent<v8::Context>::Dispose() that balances this New() ?
> Source/WebCore/bindings/v8/WorldContextHandle.cpp:70
> + if (m_context->get().IsEmpty()) {
> + ASSERT_NOT_REACHED();
> + // FIXME: This is problematic, but an empty context is worse.
> return script->mainWorldContext();
> + }
Please just ASSERT