Bug 82238 – Use PassRefPtr in V8DOMWrapper interface to avoid explicit ref() calls

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

Use PassRefPtr in V8DOMWrapper interface to avoid explicit ref() calls

[Adam Klein]

Created attachment 133878 [details]
Patch

[Adam Barth]

Comment on attachment 133878 [details]
Patch

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

In the words of dglazkov, the old version of this code was awesome.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:417
> -    V8DOMWrapper::setJSWrapperForDOMObject(window, v8::Persistent<v8::Object>::New(jsWindow));
> +    V8DOMWrapper::setJSWrapperForDOMObject(PassRefPtr<DOMWindow>(window), v8::Persistent<v8::Object>::New(jsWindow));

I don't think you need to call the constructor explicitly.  The call should be generated implicitly.

> Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:113
>      // Add object to the wrapper map.

Mayvbe remove this useless comment?

> Source/WebCore/bindings/v8/custom/V8XMLHttpRequestConstructor.cpp:68
>      // Add object to the wrapper map.

Ditto.

[Adam Klein]

Comment on attachment 133878 [details]
Patch

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

>> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:417
>> +    V8DOMWrapper::setJSWrapperForDOMObject(PassRefPtr<DOMWindow>(window), v8::Persistent<v8::Object>::New(jsWindow));
> 
> I don't think you need to call the constructor explicitly.  The call should be generated implicitly.

I thought so too, but it won't compile without. Seems like the compiler can't figure out that DOMWindow* is implicitly convertible to PassRefPtr<DOMWindow> when it's doing template specialization.

>> Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:113
>>      // Add object to the wrapper map.
> 
> Mayvbe remove this useless comment?

Removed (and the one above).

>> Source/WebCore/bindings/v8/custom/V8XMLHttpRequestConstructor.cpp:68
>>      // Add object to the wrapper map.
> 
> Ditto.

Removed.

[Adam Klein]

Created attachment 133895 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 133895 [details]
Patch for landing

Clearing flags on attachment: 133895

Committed r112163: <http://trac.webkit.org/changeset/112163>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Klein]

Reopening to attach new patch.

[Adam Klein]

Created attachment 135708 [details]
Patch

[Adam Barth]

> Reopening to attach new patch.

What's changed in the new patch?

[Adam Klein]

(In reply to comment #9)
> > Reopening to attach new patch.
> 
> What's changed in the new patch?

I put this in the ChangeLog:

      Relanding r112163 without modification, as it still seems valid.
      Will watch Chrome Canaries closely for any memory issues.

I tried renaming the method to ensure I'd caught all callers and I had. So I suspect there's nothing wrong with this patch. Note that the OOM failures I've gotten complaints about had both this and the "always use setJSWrapper*" patch (r112318). So my plan is to leave this patch alone on trunk for awhile to shake out any problems.

[Adam Klein]

Created attachment 135728 [details]
Patch

[Adam Barth]

Comment on attachment 135728 [details]
Patch

Ok.  I missed that part of the ChangeLog.

[WebKit Review Bot]

Comment on attachment 135728 [details]
Patch

Clearing flags on attachment: 135728

Committed r113272: <http://trac.webkit.org/changeset/113272>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.