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.
<rdar://problem/57299885>
Created attachment 390700 [details] Patch
Comment on attachment 390700 [details] Patch Needs more work.
Created attachment 390787 [details] Patch
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.
Created attachment 390943 [details] Patch
(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.
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;
(In reply to Darin Adler from comment #8) > review- before of the overrelease, otherwise looks great because of
Created attachment 390964 [details] Patch
(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!
Comment on attachment 390964 [details] Patch Thanks for reviewing!
Comment on attachment 390964 [details] Patch Clearing flags on attachment: 390964 Committed r256839: <https://trac.webkit.org/changeset/256839>
All reviewed patches have been landed. Closing bug.