Bug 222009 – Use smart pointer for WebView ownership in DumpRenderTree

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Dumez]

Use smart pointer for WebView ownership in DumpRenderTree.

[Chris Dumez]

Created attachment 420559 [details]
Patch

[Geoffrey Garen]

Comment on attachment 420559 [details]
Patch

r=me

[Alex Christensen]

Comment on attachment 420559 [details]
Patch

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

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:781
> +    return webView.leakRef();

Why don't we just make createWebViewAndOffscreenWindow return a RetainPtr?

[Chris Dumez]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 420559 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420559&action=review
> 
> > Tools/DumpRenderTree/mac/DumpRenderTree.mm:781
> > +    return webView.leakRef();
> 
> Why don't we just make createWebViewAndOffscreenWindow return a RetainPtr?

I believe I tried that but this function is declared in the header and I was unable to have the function return a RetainPtr<> in the header (adding the include for Retain.h gave a build error).

[Darin Adler]

Comment on attachment 420559 [details]
Patch

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

>>> Tools/DumpRenderTree/mac/DumpRenderTree.mm:781
>>> +    return webView.leakRef();
>> 
>> Why don't we just make createWebViewAndOffscreenWindow return a RetainPtr?
> 
> I believe I tried that but this function is declared in the header and I was unable to have the function return a RetainPtr<> in the header (adding the include for Retain.h gave a build error).

Not sure this is important/urgent, but might be able to do it by putting #ifdef __cplusplus around some of the code in the header. (Also, should be Forward.h in the header.)

[Chris Dumez]

Comment on attachment 420559 [details]
Patch

Clearing flags on attachment: 420559

Committed r273022 (234223@main): <https://commits.webkit.org/234223@main>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/74447198>

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 420559 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420559&action=review
> 
> >>> Tools/DumpRenderTree/mac/DumpRenderTree.mm:781
> >>> +    return webView.leakRef();
> >> 
> >> Why don't we just make createWebViewAndOffscreenWindow return a RetainPtr?
> > 
> > I believe I tried that but this function is declared in the header and I was unable to have the function return a RetainPtr<> in the header (adding the include for Retain.h gave a build error).
> 
> Not sure this is important/urgent, but might be able to do it by putting
> #ifdef __cplusplus around some of the code in the header. (Also, should be
> Forward.h in the header.)

Oh nice. I'll give that a try, Thanks. I would me much nicer if I could return a RetainPtr<>.

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 420559 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420559&action=review
> 
> >>> Tools/DumpRenderTree/mac/DumpRenderTree.mm:781
> >>> +    return webView.leakRef();
> >> 
> >> Why don't we just make createWebViewAndOffscreenWindow return a RetainPtr?
> > 
> > I believe I tried that but this function is declared in the header and I was unable to have the function return a RetainPtr<> in the header (adding the include for Retain.h gave a build error).
> 
> Not sure this is important/urgent, but might be able to do it by putting
> #ifdef __cplusplus around some of the code in the header. (Also, should be
> Forward.h in the header.)

Also, I think this means adding a forward declaration for RetainPtr in Forward.h because there isn't one currently.