[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