Bug 223030 – Use RetainPtr<> / OSObjectPtr<> more in WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Use RetainPtr<> / OSObjectPtr<> more in WebKit.

[Chris Dumez]

Created attachment 422843 [details]
Patch

[Chris Dumez]

Created attachment 422844 [details]
Patch

[Chris Dumez]

Created attachment 422845 [details]
Patch

[Chris Dumez]

Created attachment 422846 [details]
Patch

[Chris Dumez]

Created attachment 422849 [details]
Patch

[Chris Dumez]

Created attachment 422851 [details]
Patch

[Chris Dumez]

Created attachment 422858 [details]
Patch

[Chris Dumez]

Created attachment 422863 [details]
Patch

[Darin Adler]

Comment on attachment 422863 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:161
> +    inline dispatch_queue_t dispatchQueue() const { return m_notificationQueue.get(); }

Unneeded use of "inline" here and throughout this class.

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1569
> +    m_avPlayer = playerRef;

Seems a slight shame that we hold one reference in the object and another in a local variable. Could we just use m_avPlayer directly throughout?

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1609
> +    auto itemRef = adoptF(AVCFPlayerItemCreateWithAsset(kCFAllocatorDefault, avAsset(), m_notificationQueue.get()));
> +    m_avPlayerItem = itemRef;

Ditto.

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1759
> +        static const CFStringRef keyNames[] = {
>              AVCFAssetPropertyPlayable
>          };
> -        propertyKeyName = CFArrayCreate(0, (const void**)keyNames, sizeof(keyNames) / sizeof(keyNames[0]), &kCFTypeArrayCallBacks);
> -    }
> +        return adoptCF(CFArrayCreate(0, (const void**)keyNames, sizeof(keyNames) / sizeof(keyNames[0]), &kCFTypeArrayCallBacks));

Since this code is only called once, we can write it more simply and not try to optimize so much and write such strange code. If this was Objective-C I would just write @[ AVCFAssetPropertyPlayable ] with typecasts. Here we could:

1) Type the local variable to match what the function takes, const void* elements instead of CFStringRef elements.
2) Don't use a static because this is one time code so it’s fine to dynamically allocate the array.
3) Because of (1) and (2) don’t need to typecast when calling CFArrayCreate.
4) Use std::size instead of sizeof(x) / sizeof(x[0]).

> Source/WebCore/platform/graphics/cg/PathCG.cpp:232
>      bool ret = CGPathContainsPoint(path.get(), 0, point, rule == WindRule::EvenOdd ? true : false);
>      return ret;

This code is funny. Lets write it more the way we would if it was new today:

    return CGPathContainsPoint(path.get(), nullptr, point, rule == WindRule::EvenOdd);

> Source/WebCore/platform/network/cf/CredentialStorageCFNet.cpp:49
> +    auto protectionSpaceCF = createCF(protectionSpace);
> +    auto credentialCF = copyCredentialFromProtectionSpace(protectionSpaceCF.get());
>      return core(credentialCF.get());

Would work fine in a single line:

    return core(copyCredentialFromProtectionSpace(createCF(protectionSpace).get()).get());

> Source/WebCore/platform/text/mac/TextBoundaries.mm:75
>          const char* temp = currentTextBreakLocaleID();

"temp"

> Source/WebCore/platform/text/mac/TextBoundaries.mm:80
> +    if (!locale.get())

Do we need .get() here?

> Source/WebCore/platform/text/mac/TextBoundaries.mm:86
> +    if (!tokenizer.get())

Do we need .get() here?

[Chris Dumez]

Created attachment 422878 [details]
Patch

[Chris Dumez]

(In reply to Darin Adler from comment #9)
> Comment on attachment 422863 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422863&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:161
> > +    inline dispatch_queue_t dispatchQueue() const { return m_notificationQueue.get(); }
> 
> Unneeded use of "inline" here and throughout this class.
> 
> > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1569
> > +    m_avPlayer = playerRef;
> 
> Seems a slight shame that we hold one reference in the object and another in
> a local variable. Could we just use m_avPlayer directly throughout?
> 
> > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1609
> > +    auto itemRef = adoptF(AVCFPlayerItemCreateWithAsset(kCFAllocatorDefault, avAsset(), m_notificationQueue.get()));
> > +    m_avPlayerItem = itemRef;
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1759
> > +        static const CFStringRef keyNames[] = {
> >              AVCFAssetPropertyPlayable
> >          };
> > -        propertyKeyName = CFArrayCreate(0, (const void**)keyNames, sizeof(keyNames) / sizeof(keyNames[0]), &kCFTypeArrayCallBacks);
> > -    }
> > +        return adoptCF(CFArrayCreate(0, (const void**)keyNames, sizeof(keyNames) / sizeof(keyNames[0]), &kCFTypeArrayCallBacks));
> 
> Since this code is only called once, we can write it more simply and not try
> to optimize so much and write such strange code. If this was Objective-C I
> would just write @[ AVCFAssetPropertyPlayable ] with typecasts. Here we
> could:
> 
> 1) Type the local variable to match what the function takes, const void*
> elements instead of CFStringRef elements.
> 2) Don't use a static because this is one time code so it’s fine to
> dynamically allocate the array.
> 3) Because of (1) and (2) don’t need to typecast when calling CFArrayCreate.
> 4) Use std::size instead of sizeof(x) / sizeof(x[0]).
> 
> > Source/WebCore/platform/graphics/cg/PathCG.cpp:232
> >      bool ret = CGPathContainsPoint(path.get(), 0, point, rule == WindRule::EvenOdd ? true : false);
> >      return ret;
> 
> This code is funny. Lets write it more the way we would if it was new today:
> 
>     return CGPathContainsPoint(path.get(), nullptr, point, rule ==
> WindRule::EvenOdd);
> 
> > Source/WebCore/platform/network/cf/CredentialStorageCFNet.cpp:49
> > +    auto protectionSpaceCF = createCF(protectionSpace);
> > +    auto credentialCF = copyCredentialFromProtectionSpace(protectionSpaceCF.get());
> >      return core(credentialCF.get());
> 
> Would work fine in a single line:
> 
>     return
> core(copyCredentialFromProtectionSpace(createCF(protectionSpace).get()).
> get());
> 
> > Source/WebCore/platform/text/mac/TextBoundaries.mm:75
> >          const char* temp = currentTextBreakLocaleID();
> 
> "temp"
> 
> > Source/WebCore/platform/text/mac/TextBoundaries.mm:80
> > +    if (!locale.get())
> 
> Do we need .get() here?
> 
> > Source/WebCore/platform/text/mac/TextBoundaries.mm:86
> > +    if (!tokenizer.get())
> 
> Do we need .get() here?

Yes, they were needed:
./platform/text/mac/TextBoundaries.mm:80:9: error: invalid argument type 'WTF::NeverDestroyed<WTF::RetainPtr<const __CFLocale *>, WTF::AnyThreadsAccessTraits>' to unary expression
    if (!locale)
        ^~~~~~~
./platform/text/mac/TextBoundaries.mm:86:9: error: invalid argument type 'NeverDestroyed<RetainPtr<CFStringTokenizerRef> >' (aka 'NeverDestroyed<RetainPtr<__CFStringTokenizer *> >') to unary expression
    if (!tokenizer)
        ^~~~~~~~~~

[Chris Dumez]

Created attachment 422881 [details]
Patch

[Chris Dumez]

Comment on attachment 422881 [details]
Patch

Clearing flags on attachment: 422881

Committed r274252 (235156@main): <https://commits.webkit.org/235156@main>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/75291371>