[webkit-reviews] review denied: [Bug 204320] Move [UIDevice currentDevice] calls to UI process : [Attachment 390943] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Feb 17 12:36:20 PST 2020
Darin Adler <darin at apple.com> has denied Per Arne Vollan <pvollan at apple.com>'s
request for review:
Bug 204320: Move [UIDevice currentDevice] calls to UI process
https://bugs.webkit.org/show_bug.cgi?id=204320
Attachment 390943: Patch
https://bugs.webkit.org/attachment.cgi?id=390943&action=review
--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 390943
--> https://bugs.webkit.org/attachment.cgi?id=390943
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=390943&action=review
review- before of the overrelease, otherwise looks great
>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:2737
> + displayName = [[NSString stringWithFormat:@"%@ %@",
localizedDeviceModelName, displayName] autorelease];
The stringWithFormat function already returns something autoreleased. Calling
autorelease on it a second time is going to overrelease it! Please do not do
this.
Could remove that extra space on this line after the "=" since you are touching
this.
> Source/WebCore/platform/ios/LocalizedDeviceModel.h:30
> +#import <wtf/text/WTFString.h>
Only need to include Forward.h here. No need to include WTFString.h.
> Source/WebCore/platform/ios/LocalizedDeviceModel.mm:50
> + if (cachedLocalizedDeviceModel().hasValue())
> + return *cachedLocalizedDeviceModel();
> +
> + auto localizedModel = String([[PAL::getUIDeviceClass() currentDevice]
localizedModel]);
> + cachedLocalizedDeviceModel() = localizedModel;
> +
> + return localizedModel;
Here’s how I think we should write this:
auto& deviceModel = cachedLocalizedDeviceModel();
if (!deviceModel)
deviceModel = String([[PAL::getUIDeviceClass() currentDevice]
localizedModel]);
return *deviceModel;
More information about the webkit-reviews
mailing list