Bug 204320 – Move [UIDevice currentDevice] calls to UI process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

Calling [UIDevice currentDevice] will cause the runningboard daemon to be accessed. Since this service will be removed from the WebContent sandbox, these calls should be moved to the UI process.

For example, the function exernalDeviceDisplayNameForPlayer in MediaPlayerPrivateAVFoundationObjC.mm is calling [[PAL::getUIDeviceClass() currentDevice] localizedModel], which should be moved to the UI process.

[Radar WebKit Bug Importer]

<rdar://problem/57299885>

[Per Arne Vollan]

Created attachment 390700 [details]
Patch

[Per Arne Vollan]

Comment on attachment 390700 [details]
Patch

Needs more work.

[Per Arne Vollan]

Created attachment 390787 [details]
Patch

[Darin Adler]

Comment on attachment 390787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390787&action=review

Looks great, but one significant problem.

> Source/WebCore/platform/ios/LocalizedDeviceModel.mm:47
> +    auto localizedDeviceModel = retainPtr([[PAL::getUIDeviceClass() currentDevice] localizedModel]);

This isn’t setting cachedLocalizedDeviceModel, and it should be setting it. Unless our intent is to never cache it?

There should be no need to explicitly call retainPtr. It’s almost never something you have to call explicitly.

[Per Arne Vollan]

Created attachment 390943 [details]
Patch

[Per Arne Vollan]

(In reply to Darin Adler from comment #5)
> Comment on attachment 390787 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390787&action=review
> 
> Looks great, but one significant problem.
> 
> > Source/WebCore/platform/ios/LocalizedDeviceModel.mm:47
> > +    auto localizedDeviceModel = retainPtr([[PAL::getUIDeviceClass() currentDevice] localizedModel]);
> 
> This isn’t setting cachedLocalizedDeviceModel, and it should be setting it.
> Unless our intent is to never cache it?
> 

Good point!

> There should be no need to explicitly call retainPtr. It’s almost never
> something you have to call explicitly.

Thanks for reviewing! I have updated the patch accordingly.

[Darin Adler]

Comment on attachment 390943 [details]
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/MediaPlayerPrivateAVFoundationObjC.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;

[Darin Adler]

(In reply to Darin Adler from comment #8)
> review- before of the overrelease, otherwise looks great

because of

[Per Arne Vollan]

Created attachment 390964 [details]
Patch

[Per Arne Vollan]

(In reply to Darin Adler from comment #8)
> Comment on attachment 390943 [details]
> 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/MediaPlayerPrivateAVFoundationObjC.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.
> 

Ah, thanks for catching this!

> Could remove that extra space on this line after the "=" since you are
> touching this.
> 

Done.

> > 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.
> 

Fixed.

> > 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;

Yes, that is much better :)

Thanks for reviewing!

[Per Arne Vollan]

Comment on attachment 390964 [details]
Patch

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 390964 [details]
Patch

Clearing flags on attachment: 390964

Committed r256839: <https://trac.webkit.org/changeset/256839>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.