Summary: | Cut down on use of CFGetTypeID, using dynamic_cf_cast instead; related streamlining | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Darin Adler <darin> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, dino, eric.carlson, ews-watchlist, glenn, hi, hta, jer.noble, joepeck, keith_miller, kondapallykalyan, mark.lam, mmaxfield, msaboff, pangle, philipj, saam, sergio, thorton, tommyw, toyoshim, tzagallo, webkit-bug-importer, yutak | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 239591 | ||||||||||||||
Attachments: |
|
Description
Darin Adler
2021-08-23 12:22:47 PDT
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. 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. |