Bug 100897 – [V8] Unify the V8GCController visitors

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

[V8] Unify the V8GCController visitors

[Adam Barth]

Created attachment 171730 [details]
Patch

[Adam Barth]

Created attachment 171732 [details]
patch (rebased onto bug 100707)

[Eric Seidel (no email)]

Comment on attachment 171732 [details]
patch (rebased onto bug 100707)

LGTM.  Presumably you'll want to wait for haraken too.

[Adam Barth]

@haraken: Thoughts?

[Kentaro Hara]

Comment on attachment 171732 [details]
patch (rebased onto bug 100707)

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

LGTM with some nits.

> Source/WebCore/bindings/v8/V8GCController.cpp:176
> +class GCHandleVisitor : public v8::PersistentHandleVisitor {

Nit: I would prefer DOMWrapperVisitor than GCHandleVisitor.

> Source/WebCore/bindings/v8/V8GCController.cpp:178
> +    virtual void VisitPersistentHandle(v8::Persistent<v8::Value> value, uint16_t classId) OVERRIDE

VisitDOMWrapper() or VisitDOMWrapperHandle() ?

> Source/WebCore/bindings/v8/V8GCController.cpp:220
> +        } else {
> +            ASSERT(classId == v8DOMObjectClassId);
> +            m_grouper.addToGroup(type->opaqueRootForGC(object, wrapper), wrapper);
> +        }

} else if (classId == v8DOMObjectClassId) {
    m_grouper.addToGroup(type->opaqueRootForGC(object, wrapper), wrapper);
  } else {
    ASSERT_NOT_REACHED();
  }

might be better, to avoid executing m_grouper.addToGroup() for unexpected objects in a release build.

[Adam Barth]

(In reply to comment #5)
> (From update of attachment 171732 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171732&action=review
> 
> > Source/WebCore/bindings/v8/V8GCController.cpp:178
> > +    virtual void VisitPersistentHandle(v8::Persistent<v8::Value> value, uint16_t classId) OVERRIDE
> 
> VisitDOMWrapper() or VisitDOMWrapperHandle() ?

This name comes from the V8 API.  I'll make your other suggested changes, however.  ;)

[Kentaro Hara]

(In reply to comment #6)
> (In reply to comment #5)
> > > +    virtual void VisitPersistentHandle(v8::Persistent<v8::Value> value, uint16_t classId) OVERRIDE
> > 
> > VisitDOMWrapper() or VisitDOMWrapperHandle() ?
> 
> This name comes from the V8 API.  I'll make your other suggested changes, however.  ;)

Makes sense.

[Adam Barth]

Created attachment 171828 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 171828 [details]
Patch for landing

Rejecting attachment 171828 [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:
ler.cpp:252: error: 'visitor' was not declared in this scope
cc1plus: warnings being treated as errors
Source/WebCore/bindings/v8/V8GCController.cpp: At global scope:
Source/WebCore/bindings/v8/V8GCController.cpp:136: error: 'void WebCore::addImplicitReferencesForNodeWithEventListeners(WebCore::Node*, v8::Persistent<v8::Object>)' defined but not used
make: *** [out/Release/obj.target/webcore_remaining/Source/WebCore/bindings/v8/V8GCController.o] Error 1
make: *** Waiting for unfinished jobs....

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

[Adam Barth]

Created attachment 171843 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 171843 [details]
Patch for landing

Clearing flags on attachment: 171843

Committed r133169: <http://trac.webkit.org/changeset/133169>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

BTW, classID should be stored not in global handles but in WrapperTypeInfo, shouldn't it? Then we can remove the code to set/get class IDs. If that's the case, I'll upload a patch. (Hopefully we want to eliminate a class_id_ field from global handles of V8, but it would be difficult for compatibility reasons.)

[Adam Barth]

> BTW, classID should be stored not in global handles but in WrapperTypeInfo, shouldn't it? Then we can remove the code to set/get class IDs. If that's the case, I'll upload a patch. (Hopefully we want to eliminate a class_id_ field from global handles of V8, but it would be difficult for compatibility reasons.)

We need to store the class ID in the handle, not in the WrapperTypeInfo.  We use them to know that a handle has a WrapperTypeInfo field in the first place.  There's lots of code that might open persistent V8 handles.  When we enumerate the handles, we need to know whether the current handle is a DOM wrapper or some other kind of handle.