[webkit-reviews] review granted: [Bug 127586] Add support for ActionSheets in WK2 for iOS : [Attachment 222869] Patch3

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 127586: Add support for ActionSheets in WK2 for iOS
https://bugs.webkit.org/show_bug.cgi?id=127586

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=222869&action=review


LGTM.

I am a little concerned the sheet use _positionInformationDidChange to update
is position. I guess since the sheet is modal, no other gesture would cause a
requestPositionInformation()?

> Source/WebKit2/Shared/ios/WKGestureTypes.h:70
> +typedef enum {
> +    WKSheetActionCopy,
> +    WKSheetActionSaveImage
> +} WKSheetActions;

enum class WKSheetAction {
    Copy,
    SaveImage
}

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.h:76
> +    NSString *_title;

RetainPtr<>

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.mm:39
> +#import "config.h"
> +#import "WKActionSheet.h"
> +#import "WKGestureTypes.h"
> +#import "WKInteractionView.h"
> +
> +#import <SafariServices/SSReadingList.h>
> +
> +#import <UIKit/UIActionSheet_Private.h>
> +#import <UIKit/UIView.h>
> +#import <UIKit/UIWindow_Private.h>
> +
> +#import <WebCore/LocalizedStrings.h>
> +#import <WebCore/SoftLinking.h>
> +#import <wtf/text/WTFString.h>

This should be config + WKActionSheet.h, then empty line, the all the other
headers together.

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.mm:50
> +    id <WKActionSheetDelegate> _sheetDelegate;
> +    id <UIActionSheetDelegate> _delegateWhileRotating;

I think the style *may* be id<protocol> but I am not sure.

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.mm:53
> +    WKInteractionView *_view;

I would move this above UIPopoverArrowDirection for packing but that's a
detail.

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.mm:110
> +			  inView:view
> +		       direction:_arrowDirections
> +    allowInteractionWithViews:nil
> +		 backgroundStyle:UIPopoverBackgroundStyleDefault
> +			animated:YES];

Uh, objective-C indentation, why you so cruel :)

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.mm:124
> +#pragma mark rotation handling code

#pragma mark - Rotation handling code

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.mm:179
> +    NSURL *_url;
> +    NSString *_title;

RetainPtr<>

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.mm:188
> +    _url = url;
> +    _interactionLocation = location;
> +    _title = title;

I would take a copy of title. Maybe of URL too, not sure if that one is
mutable.

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.mm:202
> +    _title = [title retain];

Ditto.

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.mm:236
> +    NSString *urlString = [targetURL absoluteString];
> +    if (!title || [title length] == 0)
> +	   title = urlString;

if (![title length])
    title = [targetURL absoluteString];

(so that you don't need to do a URL serialization at all if there is no title.

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.mm:246
> +	   case WKElementActionTypeCopy:

Case Indent.

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.mm:289
> +

Extra blank line.

> Source/WebKit2/UIProcess/API/ios/WKActionSheetAssistant.mm:45
> +#import "config.h"
> +#import "WebPageProxy.h"
> +#import "WKActionSheet.h"
> +#import "WKActionSheetAssistant.h"
> +#import "WKInteractionView.h"
> +
> +#import <TCC/TCC.h>
> +#import <DataDetectorsUI/DDDetectionController.h>
> +#import <SafariServices/SSReadingList.h>
> +
> +#import <UIKit/UIActionSheet_Private.h>
> +#import <UIKit/UIView.h>
> +#import <UIKit/UIViewController_Private.h>
> +#import <UIKit/UIWindow_Private.h>
> +
> +#import <WebCore/LocalizedStrings.h>
> +#import <WebCore/SoftLinking.h>
> +#import <WebCore/WebCoreNSURLExtras.h>
> +#import <wtf/text/WTFString.h>
> +

Header import style.

> Source/WebKit2/UIProcess/API/ios/WKActionSheetAssistant.mm:64
> +    RetainPtr<WKActionSheet> _interactionSheet;
> +    RefPtr<WebKit::WebPageProxy> _page;
> +    RetainPtr<NSArray> _elementActions;
> +    WKInteractionView *_view;

It is easy to spot code you wrote from the RetainPtr instead of manual memory
management :)

> Source/WebKit2/UIProcess/API/ios/WKActionSheetAssistant.mm:128
> +

Extra blank line.

> Source/WebKit2/UIProcess/API/ios/WKActionSheetAssistant.mm:190
> +    BOOL isJavaScriptURL = [[targetURL scheme] length] && [[targetURL
scheme] caseInsensitiveCompare:@"javascript"] == NSOrderedSame;

Maybe store [targetURL scheme] in a temporary variable?

> Source/WebKit2/UIProcess/API/ios/WKActionSheetAssistant.mm:193
> +    // The sheet is released in actionSheet:clickedButtonAtIndex:

You can probably remove this comment.

> Source/WebKit2/UIProcess/API/ios/WKActionSheetAssistant.mm:220
> +    // The element actions are released in actionSheet:clickedButtonAtIndex:


Same, it is low risk anyway with the RetainPtr.

> Source/WebKit2/UIProcess/API/ios/WKActionSheetAssistant.mm:261
> +    NSMutableArray *actions = [NSMutableArray array];

maybe RetainPtr<NSMutableArray> actions = adoptNS([[NSMutableArray alloc]
initWithCapacity:3]);

(I don't like the autorelease pool, too much trouble when debugging :))

> Source/WebKit2/UIProcess/API/ios/WKActionSheetAssistant.mm:298
> +    NSMutableArray *elementActions = [NSMutableArray array];

Ditto if you want. With initialCapacity of [dataDetectorsActions count].

> Source/WebKit2/UIProcess/API/ios/WKActionSheetAssistant.mm:299
> +    for (NSUInteger actionNumber = 0; actionNumber < [dataDetectorsActions
count]; actionNumber++) {

[dataDetectorsActions count] could go in a temporary.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:448
> +    else if (_positionInformation.clickableElementName == "A") {

I wonder why upper case...