Cut down on use of CFGetTypeID, using dynamic_cf_cast instead; related streamlining
Created attachment 436230 [details] Patch
Created attachment 436233 [details] Patch
dynamic_cf_cast! Very cool!
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.
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);
(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.
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.
Created attachment 436531 [details] Patch
Created attachment 436549 [details] Patch
Created attachment 436600 [details] Patch
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.
<rdar://problem/82540876>
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`
Committed r281791 (241128@main): <https://commits.webkit.org/241128@main>
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!
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.