Bug 94882 - [V8] V8DOMWindowShell should use ScopedPersistent
Summary: [V8] V8DOMWindowShell should use ScopedPersistent
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: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-23 17:50 PDT by Adam Barth
Modified: 2012-08-23 22:59 PDT (History)
4 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-08-23 17:50:57 PDT 
[V8] V8DOMWindowShell should use ScopedPersistent
Comment 1 Adam Barth 2012-08-23 17:51:23 PDT 
Created attachment 160297 [details]
Work in progress
Comment 2 Adam Barth 2012-08-23 21:31:38 PDT 
Created attachment 160323 [details]
Patch
Comment 3 Kentaro Hara 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() ?
Comment 4 Adam Barth 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.
Comment 5 Kentaro Hara 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.
Comment 6 WebKit Review Bot 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>
Comment 7 WebKit Review Bot 2012-08-23 22:59:22 PDT 
All reviewed patches have been landed.  Closing bug.