WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
229414
Cut down on use of CFGetTypeID, using dynamic_cf_cast instead; related streamlining
https://bugs.webkit.org/show_bug.cgi?id=229414
Summary
Cut down on use of CFGetTypeID, using dynamic_cf_cast instead; related stream...
Darin Adler
Reported
2021-08-23 12:22:47 PDT
Cut down on use of CFGetTypeID, using dynamic_cf_cast instead; related streamlining
Attachments
Patch
(99.85 KB, patch)
2021-08-23 13:35 PDT
,
Darin Adler
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(100.14 KB, patch)
2021-08-23 14:00 PDT
,
Darin Adler
no flags
Details
Formatted Diff
Diff
Patch
(102.17 KB, patch)
2021-08-26 10:43 PDT
,
Darin Adler
no flags
Details
Formatted Diff
Diff
Patch
(103.30 KB, patch)
2021-08-26 12:35 PDT
,
Darin Adler
no flags
Details
Formatted Diff
Diff
Patch
(103.39 KB, patch)
2021-08-26 18:42 PDT
,
Darin Adler
thorton
: review+
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Darin Adler
Comment 1
2021-08-23 13:35:44 PDT
Created
attachment 436230
[details]
Patch
Darin Adler
Comment 2
2021-08-23 14:00:13 PDT
Created
attachment 436233
[details]
Patch
Myles C. Maxfield
Comment 3
2021-08-23 14:35:29 PDT
dynamic_cf_cast! Very cool!
Joseph Pecoraro
Comment 4
2021-08-23 14:55:06 PDT
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 5
2021-08-23 15:02:20 PDT
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
Comment 6
2021-08-23 15:37:05 PDT
(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 7
2021-08-24 08:59:30 PDT
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
Comment 8
2021-08-26 10:43:36 PDT
Comment hidden (obsolete)
Created
attachment 436531
[details]
Patch
Darin Adler
Comment 9
2021-08-26 12:35:46 PDT
Comment hidden (obsolete)
Created
attachment 436549
[details]
Patch
Darin Adler
Comment 10
2021-08-26 18:42:29 PDT
Created
attachment 436600
[details]
Patch
Darin Adler
Comment 11
2021-08-27 09:38:46 PDT
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
Comment 12
2021-08-30 12:23:23 PDT
<
rdar://problem/82540876
>
Tim Horton
Comment 13
2021-08-30 18:59:01 PDT
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
Comment 14
2021-08-30 20:43:41 PDT
Committed
r281791
(
241128@main
): <
https://commits.webkit.org/241128@main
>
Darin Adler
Comment 15
2021-08-30 20:49:32 PDT
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 16
2022-05-12 11:17:05 PDT
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug