Bug 229414 – Cut down on use of CFGetTypeID, using dynamic_cf_cast instead; related streamlining

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Darin Adler]

Cut down on use of CFGetTypeID, using dynamic_cf_cast instead; related streamlining

[Darin Adler]

Created attachment 436230 [details]
Patch

[Darin Adler]

Created attachment 436233 [details]
Patch

[Myles C. Maxfield]

dynamic_cf_cast! Very cool!

[Joseph Pecoraro]

Comment on attachment 436233 [details]
Patch

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

> Source/WebCore/platform/ios/UserAgentIOS.mm:-85
> -        String appNameSuffix = applicationName.isEmpty() ? "" : makeString(" ", applicationName);

Is there a performance benefit to removing these intermediate variables? These `appNameSuffix` variables seemed to make the code more readable, but that may just be personal preference.

[Darin Adler]

Comment on attachment 436233 [details]
Patch

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

>> Source/WebCore/platform/ios/UserAgentIOS.mm:-85
>> -        String appNameSuffix = applicationName.isEmpty() ? "" : makeString(" ", applicationName);
> 
> Is there a performance benefit to removing these intermediate variables? These `appNameSuffix` variables seemed to make the code more readable, but that may just be personal preference.

Yes, there is a performance benefit.

But I can do a different version where we still have local variables, just different ones, and still keep the performance benefit. For example, you might like this better:

    auto separator = applicationName.isEmpty() ? "" : " ";
    return makeString("Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/605.1.15 (KHTML, like Gecko)", separator, applicationName);

[Joseph Pecoraro]

(In reply to Darin Adler from comment #5)
> Comment on attachment 436233 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436233&action=review
> 
> >> Source/WebCore/platform/ios/UserAgentIOS.mm:-85
> >> -        String appNameSuffix = applicationName.isEmpty() ? "" : makeString(" ", applicationName);
> > 
> > Is there a performance benefit to removing these intermediate variables? These `appNameSuffix` variables seemed to make the code more readable, but that may just be personal preference.
> 
> Yes, there is a performance benefit.
> 
> But I can do a different version where we still have local variables, just
> different ones, and still keep the performance benefit. For example, you
> might like this better:
> 
>     auto separator = applicationName.isEmpty() ? "" : " ";
>     return makeString("Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6)
> AppleWebKit/605.1.15 (KHTML, like Gecko)", separator, applicationName);

Yeah that reads better to me then the inlined ternary, just having the name "separator" or "suffix" is helpful to understand the intent.

[Darin Adler]

Comment on attachment 436233 [details]
Patch

Looks like this broke iOS, should be easy to fix. But also, not compiling on Windows, trivial to fix. So not ready to review yet.

[Darin Adler]

Created attachment 436531 [details]
Patch

[Darin Adler]

Created attachment 436549 [details]
Patch

[Darin Adler]

Created attachment 436600 [details]
Patch

[Darin Adler]

Looks like it is passing all EWS tests now, so ready for a review.

I improved the coding style at Joe’s request to use a local variable.

[Radar WebKit Bug Importer]

<rdar://problem/82540876>

[Tim Horton]

Comment on attachment 436600 [details]
Patch

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

> Source/WebKitLegacy/cf/ChangeLog:9
> +        (loadSetting): Rename from populateSetting. Use dyanmic_cf_cast, kCFBooleanTrue, and

`dyanmic`

[Darin Adler]

Committed r281791 (241128@main): <https://commits.webkit.org/241128@main>

[Darin Adler]

Comment on attachment 436600 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        (WebCore::makeSimpleColorFromARGBCFArray): Use dyanamic_cf_cast.

Fixed this one.

>> Source/WebKitLegacy/cf/ChangeLog:9
>> +        (loadSetting): Rename from populateSetting. Use dyanmic_cf_cast, kCFBooleanTrue, and
> 
> `dyanmic`

Didn’t fix this one, because I thought your comment was about the other one!

[Eric Carlson]

Comment on attachment 436600 [details]
Patch

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

> Source/WTF/wtf/cf/TypeCastsCF.h:2
> + * Copyright (C) 2014-2021 Apple Inc. All rights reserved.

Did you mean to update to only 2021?

> Source/WebCore/platform/network/mac/WebCoreURLResponse.h:2
> + * Copyright (C) 2008-2021 Apple Inc. All rights reserved.

Ditto

> Source/WebCore/platform/network/mac/WebCoreURLResponse.mm:2
> + * Copyright (C) 2008-2021 Apple Inc. All rights reserved.

Ditto

> Source/WebKitLegacy/cf/WebCoreSupport/WebInspectorClientCF.cpp:2
> + * Copyright (C) 2008-2021 Apple Inc. All Rights Reserved.

Ditto.