Bug 127586 - Add support for ActionSheets in WK2 for iOS
Summary: Add support for ActionSheets in WK2 for iOS
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-24 14:33 PST by Enrica Casucci
Modified: 2014-02-01 17:57 PST (History)
2 users (show)

See Also:


Attachments
WebCore Patch (13.37 KB, patch)
2014-01-28 19:13 PST, Enrica Casucci
benjamin: review+
Details | Formatted Diff | Diff
WebKit2 patch (56.87 KB, patch)
2014-01-31 14:30 PST, Enrica Casucci
no flags Details | Formatted Diff | Diff
Updated patch (65.16 KB, patch)
2014-01-31 17:47 PST, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch3 (66.10 KB, patch)
2014-01-31 18:57 PST, Enrica Casucci
benjamin: 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-01-24 14:33:02 PST
This bug tracks the work required to support action sheets  in WebKit2 on iOS.

<rdar://problem/15283667>
Comment 1 Enrica Casucci 2014-01-28 19:13:34 PST
Created attachment 222545 [details]
WebCore Patch
Comment 2 Benjamin Poulain 2014-01-28 19:30:58 PST
Comment on attachment 222545 [details]
WebCore Patch

lgtm
Comment 3 Enrica Casucci 2014-01-28 20:06:16 PST
Committed revision 162997.
Comment 4 Enrica Casucci 2014-01-31 14:30:08 PST
Created attachment 222850 [details]
WebKit2 patch
Comment 5 WebKit Commit Bot 2014-01-31 14:31:04 PST
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.
Comment 6 Benjamin Poulain 2014-01-31 14:54:25 PST
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?
Comment 7 Enrica Casucci 2014-01-31 17:47:53 PST
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.
Comment 8 Enrica Casucci 2014-01-31 18:57:48 PST
Created attachment 222869 [details]
Patch3

I forgot to change the action from string to enum.
Comment 9 Benjamin Poulain 2014-01-31 18:58:53 PST
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.
Comment 10 Benjamin Poulain 2014-01-31 19:42:16 PST
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...
Comment 11 Enrica Casucci 2014-02-01 17:57:54 PST
(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.