Bug 82238 - Use PassRefPtr in V8DOMWrapper interface to avoid explicit ref() calls
Summary: Use PassRefPtr in V8DOMWrapper interface to avoid explicit ref() calls
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 Klein
URL:
Keywords:
Depends on: 82914
Blocks:
  Show dependency tree / graph
 
Reported: 2012-03-26 13:26 PDT by Adam Klein
Modified: 2012-04-04 18:38 PDT (History)
5 users (show)

See Also:

Attachments
Patch (14.07 KB, patch)
2012-03-26 13:34 PDT, Adam Klein 		no flags Details | Formatted Diff | Diff
Patch for landing (14.33 KB, patch)
2012-03-26 14:45 PDT, Adam Klein 		no flags Details | Formatted Diff | Diff
Patch (14.55 KB, patch)
2012-04-04 16:16 PDT, Adam Klein 		no flags Details | Formatted Diff | Diff
Patch (14.52 KB, patch)
2012-04-04 17:29 PDT, Adam Klein 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) 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 Klein 2012-03-26 13:26:08 PDT 
Use PassRefPtr in V8DOMWrapper interface to avoid explicit ref() calls
Comment 1 Adam Klein 2012-03-26 13:34:02 PDT 
Created attachment 133878 [details]
Patch
Comment 2 Adam Barth 2012-03-26 14:37:56 PDT 
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.
Comment 3 Adam Klein 2012-03-26 14:45:17 PDT 
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.
Comment 4 Adam Klein 2012-03-26 14:45:40 PDT 
Created attachment 133895 [details]
Patch for landing
Comment 5 WebKit Review Bot 2012-03-26 15:57:40 PDT 
Comment on attachment 133895 [details]
Patch for landing

Clearing flags on attachment: 133895

Committed r112163: <http://trac.webkit.org/changeset/112163>
Comment 6 WebKit Review Bot 2012-03-26 15:57:45 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 7 Adam Klein 2012-04-04 16:16:34 PDT 
Reopening to attach new patch.
Comment 8 Adam Klein 2012-04-04 16:16:36 PDT 
Created attachment 135708 [details]
Patch
Comment 9 Adam Barth 2012-04-04 17:20:45 PDT 
> Reopening to attach new patch.

What's changed in the new patch?
Comment 10 Adam Klein 2012-04-04 17:25:29 PDT 
(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.
Comment 11 Adam Klein 2012-04-04 17:29:46 PDT 
Created attachment 135728 [details]
Patch
Comment 12 Adam Barth 2012-04-04 17:30:26 PDT 
Comment on attachment 135728 [details]
Patch

Ok.  I missed that part of the ChangeLog.
Comment 13 WebKit Review Bot 2012-04-04 18:38:44 PDT 
Comment on attachment 135728 [details]
Patch

Clearing flags on attachment: 135728

Committed r113272: <http://trac.webkit.org/changeset/113272>
Comment 14 WebKit Review Bot 2012-04-04 18:38:49 PDT 
All reviewed patches have been landed.  Closing bug.