Bug 94882

Summary:

[V8] V8DOMWindowShell should use ScopedPersistent

Product:

WebKit

Reporter:

Adam Barth <abarth>

Component:

New Bugs

Assignee:

Adam Barth <abarth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

eric, haraken, japhet, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Work in progress	none
Patch	none

Adam Barth

Reported 2012-08-23 17:50:57 PDT

[V8] V8DOMWindowShell should use ScopedPersistent

Attachments
Work in progress (19.95 KB, patch) 2012-08-23 17:51 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch (20.09 KB, patch) 2012-08-23 21:31 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Adam Barth

Comment 1 2012-08-23 17:51:23 PDT

Created attachment 160297 [details] Work in progress

Adam Barth

Comment 2 2012-08-23 21:31:38 PDT

Created attachment 160323 [details] Patch

Kentaro Hara

Comment 3 2012-08-23 21:44:15 PDT

Comment on attachment 160323 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160323&action=review > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:200 > + ASSERT(m_context.get().IsEmpty() || !m_global.get().IsEmpty()); Shall we implement ScopedContext::isEmpty() to avoid m_context.get().IsEmpty() repeatedly? > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:-201 > -#ifndef NDEBUG > - V8GCController::unregisterGlobalHandle(this, m_global); > -#endif Are you intending to remove this? > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:464 > + m_context.get()->Global()->ForceSet(v8::String::New("document"), documentWrapper, static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete)); Shall we implement ScopedPersistent::operator-> so that we can write this like m_context->Global() ?

Adam Barth

Comment 4 2012-08-23 21:51:55 PDT

(In reply to comment #3) > (From update of attachment 160323 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160323&action=review > > > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:200 > > + ASSERT(m_context.get().IsEmpty() || !m_global.get().IsEmpty()); > > Shall we implement ScopedContext::isEmpty() to avoid m_context.get().IsEmpty() repeatedly? Sure. > > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:-201 > > -#ifndef NDEBUG > > - V8GCController::unregisterGlobalHandle(this, m_global); > > -#endif > > Are you intending to remove this? Yes. I've never found the registerGlobalHandle thing useful. We can add it to ScopedPersistent if we want. > > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:464 > > + m_context.get()->Global()->ForceSet(v8::String::New("document"), documentWrapper, static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete)); > > Shall we implement ScopedPersistent::operator-> so that we can write this like m_context->Global() ? Sure. Do you mind if I do those things in a followup patch? I'll deploy them to all the uses of ScopedPersistent.

Kentaro Hara

Comment 5 2012-08-23 21:53:05 PDT

Comment on attachment 160323 [details] Patch > Do you mind if I do those things in a followup patch? Sounds good.

WebKit Review Bot

Comment 6 2012-08-23 22:59:18 PDT

Comment on attachment 160323 [details] Patch Clearing flags on attachment: 160323 Committed r126544: <http://trac.webkit.org/changeset/126544>

WebKit Review Bot

Comment 7 2012-08-23 22:59:22 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.