<rdar://problem/42446498>
Created attachment 348447 [details] Patch
Comment on attachment 348447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348447&action=review r=me, with comments. > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:373 > + CGRect visibleRect = CGRectNull; See associated next comment. I think this should be view.window.bounds (or equivalent) not CGRectNull. > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:402 > { Probably rename this to something better if we make the associated changes. > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:411 > + visibleRect = view.window.bounds; Could we move this logic to the calling function? I don't like having to do this compare here, if we just set the visible rect to the default before we even call into here, then we can make this cleaner. > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:414 > + remove whitespace > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:549 > + CGRect visibleRect = CGRectNull; Same as above. Should be view.window.bounds
Comment on attachment 348447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348447&action=review Will come back with a v2. I think moving the free function to an instance method will address your concerns. >> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:373 >> + CGRect visibleRect = CGRectNull; > > See associated next comment. > I think this should be view.window.bounds (or equivalent) not CGRectNull. I’m trying to keep the fallback value in one place. If you don’t like using CGRectNull as a sentinel, how about moving presentationStyleForView() to an instance method on WKActionSheetAssistant. That way it could make the delegate call and have the fallback, rather than callers providing it? >> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:414 >> + > > remove whitespace This was already here (the diff got out of order), but I can remove it. >> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:549 >> + CGRect visibleRect = CGRectNull; > > Same as above. Should be view.window.bounds Ditto also :P
Created attachment 348579 [details] Patch
Comment on attachment 348579 [details] Patch I like this better
Comment on attachment 348579 [details] Patch Clearing flags on attachment: 348579 Committed r235870: <https://trac.webkit.org/changeset/235870>
All reviewed patches have been landed. Closing bug.