[webkit-reviews] review granted: [Bug 67828] Rework script context creation/release notifications : [Attachment 107754] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Aaron Boodman <aa at chromium.org>'s
request for review:
Bug 67828: Rework script context creation/release notifications
https://bugs.webkit.org/show_bug.cgi?id=67828

Attachment 107754: Patch
https://bugs.webkit.org/attachment.cgi?id=107754&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107754&action=review


This looks great.  You might want to check over your indentation in the tests.

> Source/WebCore/ChangeLog:8
> +	   No new tests. (OOPS!)

Lies!  There are tests!

> Source/WebCore/bindings/v8/V8IsolatedContext.cpp:52
> -    : m_world(IsolatedWorld::create(worldId))
> +    : m_world(IsolatedWorld::create(worldId)), m_frame(proxy->frame())

This should be on two separate lines.

Is there any risk that the V8IsolatedContext could outlive the frame?  If so,
we'll need a mechanism to make sure we don't hold a dangling pointer. 
V8IsolatedContext's lifetime is managed by the garbage collector, right?  That
means it can live arbitrarily long.

> Source/WebCore/bindings/v8/V8IsolatedContext.cpp:84
> +    m_frame->loader()->client()->willReleaseScriptContext(context(),
m_world->id());
>      m_context->get().MakeWeak(this, &contextWeakReferenceCallback);
> +    m_frame = 0;

Ah, I see that we clear out the pointer before turning our fortunes over to the
garbage collector.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:304
> +    std::vector<Notification*> createNotifications;
> +    std::vector<Notification*> releaseNotifications;

These names are confusing.  They feel like method names (verbs).  Maybe
createNotificationsReceived?  logOfCreateNotifications ?

I asume we aren't allowed to depend upon base here to avoid the manual memory
management.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:385
> +    for (size_t i = 0; i < webFrameClient.releaseNotifications.size(); ++i)
{
> +	 EXPECT_TRUE(webFrameClient.releaseNotifications[i]->Equals(
> +	    
webFrameClient.createNotifications[webFrameClient.createNotifications.size() -
3 - i]));
> +    }

- 3 ?  Looks odd, but ok.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:446
> +    for (size_t i = 0; i < webFrameClient.releaseNotifications.size(); ++i)
{
> +	 if
(webFrameClient.releaseNotifications[i]->Equals(webFrameClient.createNotificati
ons[0]))
> +	   ++matchCount;
> +    }

This file is supposed to use four-space indent.