Bug 133582 - WK2 iOS: Adopt the new UIAlertController API to replace UIActionSheet
Summary: WK2 iOS: Adopt the new UIAlertController API to replace UIActionSheet
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-06-06 12:16 PDT by Enrica Casucci
Modified: 2014-06-06 15:07 PDT (History)
2 users (show)

See Also:


Attachments
Patch (22.16 KB, patch)
2014-06-06 12:20 PDT, Enrica Casucci
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2014-06-06 12:16:12 PDT
UIActionSheet is deprecated in iOS8.

<rdar://problem/16845223>
Comment 1 Enrica Casucci 2014-06-06 12:20:26 PDT
Created attachment 232626 [details]
Patch
Comment 2 WebKit Commit Bot 2014-06-06 12:21:45 PDT
Attachment 232626 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKActionSheetAssistant.mm:200:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKActionSheetAssistant.mm:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKActionSheetAssistant.mm:210:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKActionSheetAssistant.mm:305:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKActionSheetAssistant.mm:307:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Enrica Casucci 2014-06-06 12:23:55 PDT
Wrong radar. This is the correct one. I fixed it in the ChangeLog <rdar://problem/16845223>
Comment 4 Joseph Pecoraro 2014-06-06 12:59:27 PDT
Comment on attachment 232626 [details]
Patch

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
> +@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
Comment 5 Enrica Casucci 2014-06-06 14:21:21 PDT
Comment on attachment 232626 [details]
Patch

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

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

You are absolutely right. My bad.

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

The code that does the similar thing for WK1 calls retain when assigning _popoverPresentationControllerDelegateWhileRotating, that is why I opted for RetainPtr.
Comment 6 Enrica Casucci 2014-06-06 15:07:09 PDT
Committed revision 169664.