[webkit-reviews] review granted: [Bug 133582] WK2 iOS: Adopt the new UIAlertController API to replace UIActionSheet : [Attachment 232626] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 133582: WK2 iOS: Adopt the new UIAlertController API to replace
UIActionSheet
https://bugs.webkit.org/show_bug.cgi?id=133582

Attachment 232626: Patch
https://bugs.webkit.org/attachment.cgi?id=232626&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=232626&action=review


This looks good to me. A lot of small comments. But r=me

> Source/WebKit2/UIProcess/API/Cocoa/_WKElementAction.mm:50
> +    WKElementActionDismissalHandler _dismissalHandler;

I think this needs to be released in -dealloc because this is a (copy)
property.

> Source/WebKit2/UIProcess/ios/WKActionSheet.h:34
> + at interface WKActionSheet : UIAlertController

Nit: We may want to rename this class and file. Fine for now.

> Source/WebKit2/UIProcess/ios/WKActionSheet.mm:44
> +    RetainPtr<id <UIPopoverPresentationControllerDelegate>>
_popoverPresentationControllerDelegateWhileRotating;

Do you really want a RetainPtr<> here? This is a delegate.

Also, it looks like you can remove _delegateWhileRotating, it is no longer
used.

> Source/WebKit2/UIProcess/ios/WKActionSheet.mm:95
> +    UIViewController *presentedViewController =
(_presentedViewControllerWhileRotating.get()) ?
_presentedViewControllerWhileRotating.get() : self;

Style: Unnecessary parenthesis around the ternary condition.

> Source/WebKit2/UIProcess/ios/WKActionSheet.mm:124
> +    // We want to save the view controller that is currently being presented
to re-present it after rotation.

Typo: Extra "it" at the end of the sentence.

> Source/WebKit2/UIProcess/ios/WKActionSheet.mm:143
> +

Style: Extra blank line.

> Source/WebKit2/UIProcess/ios/WKActionSheet.mm:184
> +    UIViewController *presentedViewController =
(_presentedViewControllerWhileRotating.get()) ?
_presentedViewControllerWhileRotating.get() : self;

Style: Extra parenthesis.

> Source/WebKit2/UIProcess/ios/WKActionSheet.mm:196
>      _isRotating = NO;

Should "_isRotating = NO" be in -didRotate? Seems misleading that something
else might change this internal only flag besides rotation.

> Source/WebKit2/UIProcess/ios/WKActionSheetAssistant.mm:200
> +    for (_WKElementAction *action in actions)

Style: Give this for block braces