Bug 221914 - Reduce explicit usage of [objC release] in WebKit even more
Summary: Reduce explicit usage of [objC release] in WebKit even more
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 221932
  Show dependency treegraph
 
Reported: 2021-02-15 12:16 PST by Chris Dumez
Modified: 2021-02-16 11:27 PST (History)
14 users (show)

See Also:


Attachments
Patch (117.77 KB, patch)
2021-02-15 14:50 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (124.06 KB, patch)
2021-02-16 10:00 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (124.79 KB, patch)
2021-02-16 10:06 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-15 12:16:13 PST
Reduce explicit usage of [objC release] in WebKit even more.
Comment 1 Chris Dumez 2021-02-15 14:50:10 PST
Created attachment 420379 [details]
Patch
Comment 2 EWS Watchlist 2021-02-15 14:50:58 PST
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)
Comment 3 Alex Christensen 2021-02-16 09:31:40 PST
Comment on attachment 420379 [details]
Patch

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

> Source/WebKitLegacy/mac/History/WebHistory.mm:664
> +    _sharedHistory = retainPtr(history).leakRef();

I think it would be nicer to change the type of _sharedHistory to a function-scoped NeverDestroyed<RetainPtr>

> Source/WebKitLegacy/mac/Plugins/WebPluginDatabase.mm:195
> +    auto oldPaths = adoptNS(additionalWebPlugInPaths);

Ditto.  additionalWebPlugInPaths should be a RetainPtr.
Comment 4 Chris Dumez 2021-02-16 09:34:16 PST
(In reply to Alex Christensen from comment #3)
> Comment on attachment 420379 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420379&action=review
> 
> > Source/WebKitLegacy/mac/History/WebHistory.mm:664
> > +    _sharedHistory = retainPtr(history).leakRef();
> 
> I think it would be nicer to change the type of _sharedHistory to a
> function-scoped NeverDestroyed<RetainPtr>

Same comment as below. _sharedHistory is a global static.

> 
> > Source/WebKitLegacy/mac/Plugins/WebPluginDatabase.mm:195
> > +    auto oldPaths = adoptNS(additionalWebPlugInPaths);
> 
> Ditto.  additionalWebPlugInPaths should be a RetainPtr.

additionalWebPlugInPaths is a global static. If I use a RetainPtr<> then the compiler complains about having an exit-time destructor. We usually use NeverDestroyed<> but it does not seem to fit the bill here since we want to update the pointer and release the previous value.
Comment 5 Darin Adler 2021-02-16 09:36:06 PST
(In reply to Chris Dumez from comment #4)
> additionalWebPlugInPaths is a global static. If I use a RetainPtr<> then the
> compiler complains about having an exit-time destructor. We usually use
> NeverDestroyed<> but it does not seem to fit the bill here since we want to
> update the pointer and release the previous value.

NeverDestroyed<RetainPtr<>> should work fine for that. Despite its name, assigning something to such a variable would update the pointer and release the previous value. That’s *assigning*, not *destroying*.
Comment 6 Chris Dumez 2021-02-16 09:39:24 PST
(In reply to Darin Adler from comment #5)
> (In reply to Chris Dumez from comment #4)
> > additionalWebPlugInPaths is a global static. If I use a RetainPtr<> then the
> > compiler complains about having an exit-time destructor. We usually use
> > NeverDestroyed<> but it does not seem to fit the bill here since we want to
> > update the pointer and release the previous value.
> 
> NeverDestroyed<RetainPtr<>> should work fine for that. Despite its name,
> assigning something to such a variable would update the pointer and release
> the previous value. That’s *assigning*, not *destroying*.

Hmm. I did not see an operator=() in NeverDestroyed<>.
Comment 7 Darin Adler 2021-02-16 09:47:41 PST
You have to write .get() on the left side of the "=" operator. Adding operator= to NeverDestroyed would eliminate the need to write .get().
Comment 8 Chris Dumez 2021-02-16 10:00:34 PST
Created attachment 420484 [details]
Patch
Comment 9 Chris Dumez 2021-02-16 10:06:57 PST
Created attachment 420487 [details]
Patch
Comment 10 EWS 2021-02-16 11:26:55 PST
Committed r272919: <https://commits.webkit.org/r272919>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420487 [details].
Comment 11 Radar WebKit Bug Importer 2021-02-16 11:27:21 PST
<rdar://problem/74399599>