Bug 222009 - Use smart pointer for WebView ownership in DumpRenderTree
Summary: Use smart pointer for WebView ownership in DumpRenderTree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 222060 222068
  Show dependency treegraph
 
Reported: 2021-02-16 16:33 PST by Chris Dumez
Modified: 2021-02-17 14:09 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.24 KB, patch)
2021-02-16 16:35 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-02-16 16:33:33 PST
Use smart pointer for WebView ownership in DumpRenderTree.
Comment 1 Chris Dumez 2021-02-16 16:35:20 PST
Created attachment 420559 [details]
Patch
Comment 2 Geoffrey Garen 2021-02-17 10:33:29 PST
Comment on attachment 420559 [details]
Patch

r=me
Comment 3 Alex Christensen 2021-02-17 12:34:39 PST
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?
Comment 4 Chris Dumez 2021-02-17 12:35:46 PST
(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).
Comment 5 Darin Adler 2021-02-17 12:59:50 PST
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.)
Comment 6 Chris Dumez 2021-02-17 13:17:58 PST
Comment on attachment 420559 [details]
Patch

Clearing flags on attachment: 420559

Committed r273022 (234223@main): <https://commits.webkit.org/234223@main>
Comment 7 Chris Dumez 2021-02-17 13:18:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2021-02-17 13:18:14 PST
<rdar://problem/74447198>
Comment 9 Chris Dumez 2021-02-17 13:26:21 PST
(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<>.
Comment 10 Chris Dumez 2021-02-17 13:53:06 PST
(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.