Bug 127586 – Add support for ActionSheets in WK2 for iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Enrica Casucci]

This bug tracks the work required to support action sheets  in WebKit2 on iOS.

<rdar://problem/15283667>

[Enrica Casucci]

Created attachment 222545 [details]
WebCore Patch

[Benjamin Poulain]

Comment on attachment 222545 [details]
WebCore Patch

lgtm

[Enrica Casucci]

Committed revision 162997.

[Enrica Casucci]

Created attachment 222850 [details]
WebKit2 patch

[WebKit Commit Bot]

Attachment 222850 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/ios/WKActionSheet.h:34:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 222850 [details]
WebKit2 patch

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

I am not a fan of having WKInteractionView be the delegate of the action sheet. I am afraid this goes in the general design issue of having a single object responding to everything.
Would it be possible to have a wrapper with nice encapsulation or does it has to know too much about the page?

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:41
> +#import <DataDetectorsUI/DDDetectionController.h>
> +#import <SafariServices/SSReadingList.h>
> +#import <TCC/TCC.h>

Import order.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:58
> +#import <WebCore/WebCoreNSURLExtras.h>
> +#import <WebCore/SoftLinking.h>

Import order.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:190
> +    NSArray* _elementActions;

Style for the *.
This should really be a RetainPtr.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:439
> +// WKActionSheetDelegate

#pragma mark - Implementation of WKActionSheetDelegate

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:441
> +- (UIView *)superviewForSheet

_superviewForSheet

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:450
> +    UIView* currentView = self;

Space star.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:873
> +

Extra blank line.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:879
> +    m_interactionNode = 0;

= nullptr;

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:882
> +void WebPage::performActionOnElement(const String& action)

I would prefer a typed enum for the action.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:887
> +    WebCore::HTMLElement* element = toHTMLElement(m_interactionNode.get());

Do you need the WebCore namespace prefix in Webpage?

[Enrica Casucci]

Created attachment 222867 [details]
Updated patch

I agree with Ben's suggestion. It is not a good idea to add a lot of code to WKInteractionView to handle the action sheet specific tasks.
The new patch has a controller class WKActionSheetAssistant that encapsulates lifetime management and delegates.

[Enrica Casucci]

Created attachment 222869 [details]
Patch3

I forgot to change the action from string to enum.

[Benjamin Poulain]

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

> Source/WebKit2/UIProcess/WebPageProxy.h:454
> +    void performActionOnElement(const String&);

Shouldn't the actions be typed enum.

> Source/WebKit2/UIProcess/WebPageProxy.h:455
> +    void saveImageToLibrary(const SharedMemory::Handle& imageHandle, uint64_t imageSize);

Shouldn't this one be private since it is an incoming message?

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.h:56
> +- (WKElementActionInfo *) initWithInfo:(NSURL *)url location:(CGPoint)location title:(NSString *)title rect:(CGRect)rect;

Shouldn't this be initWithURL:location: etc?
Extra space ater the first closing parenthesis.

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.h:61
> +@property (nonatomic, readonly) CGImageRef snapshot;

Let's remove snapshot for now and add it back when we have the feature.

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.h:67
> +typedef enum {

You could use typed enum here since it stays in WebKit2.

> Source/WebKit2/UIProcess/API/ios/WKActionSheet.h:78
> +    NSString *_title;
> +    WKElementActionHandlerInternal _actionHandler;
> +    WKElementActionType _type;

Those could be moved to the class's implementation to make them private.

[Benjamin Poulain]

Comment on attachment 222869 [details]
Patch3

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

[Enrica Casucci]

(In reply to comment #10)
> (From update of attachment 222869 [details])
> 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()?
That is correct.
Thanks for the review! I'll address your comments before landing.

[Ahmad Saleem]

This change landed in:

https://github.com/WebKit/WebKit/commit/9ab981b5dbf91cf0ed6a992800f21c73bb0f1d68

https://github.com/WebKit/WebKit/commit/b239158e18d0cd8dcda05c1830caab041653443a

and it seems didn't backed out.

Marking this as "RESOLVED FIXED".