Bug 169168 – [WK2] Add a UI delegate SPI hook to enable or disable navigation on drop

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

[Wenson Hsieh]

Add a UI delegate SPI hook to enable or disable navigation on drop

[Wenson Hsieh]

<rdar://problem/30688374>

[Wenson Hsieh]

Created attachment 303393 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/DragData.cpp:42:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/DragData.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/DragDataMac.mm:96:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/DragDataMac.mm:109:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 4 in 8 files


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

[Wenson Hsieh]

Created attachment 303517 [details]
Fix Mac build

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/DragData.cpp:42:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/DragData.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/DragDataMac.mm:93:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/DragDataMac.mm:109:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 4 in 8 files


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

Comment on attachment 303517 [details]
Fix Mac build

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

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:68
> +- (BOOL)_webViewEnableNavigationOnDrop:(WKWebView *)webView WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

I think it’s more common to use the word “should” with a verb phrase rather than “enable” with a noun phrase in this sort of delegate method.

[Wenson Hsieh]

(In reply to comment #6)
> Comment on attachment 303517 [details]
> Fix Mac build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303517&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:68
> > +- (BOOL)_webViewEnableNavigationOnDrop:(WKWebView *)webView WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> I think it’s more common to use the word “should” with a verb phrase rather
> than “enable” with a noun phrase in this sort of delegate method.

Sounds good -- what about _webViewShouldEnableNavigationOnDrop? I considered making it _webViewShouldNavigateOnDrop just now, but I don't think that conveys drop behavior as faithfully as _webViewShouldEnableNavigationOnDrop, since returning YES does not guarantee that dropping a link will trigger navigation in the case that the page handles the drop.

(In reply to comment #7)
> (In reply to comment #6)
> > Comment on attachment 303517 [details]
> > Fix Mac build
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=303517&action=review
> > 
> > > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:68
> > > +- (BOOL)_webViewEnableNavigationOnDrop:(WKWebView *)webView WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> > 
> > I think it’s more common to use the word “should” with a verb phrase rather
> > than “enable” with a noun phrase in this sort of delegate method.
> 
> Sounds good -- what about _webViewShouldEnableNavigationOnDrop? I considered
> making it _webViewShouldNavigateOnDrop just now, but I don't think that
> conveys drop behavior as faithfully as _webViewShouldEnableNavigationOnDrop,
> since returning YES does not guarantee that dropping a link will trigger
> navigation in the case that the page handles the drop.

I understand now that returning YES doesn’t mean navigation will be attempted (can we avoid asking in case it won’t?) but still I feel like “enable” is the wrong word to use here. Perhaps “allow” works better? But maybe it’s just me. Let’s see what other folks think.

[Wenson Hsieh]

Created attachment 303665 [details]
Plumb drag destination actions through DragData

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/Shared/API/Cocoa/_WKDragDestinationAction.h:28:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/DragData.cpp:42:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/DragData.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/DragDataMac.mm:93:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/DragDataMac.mm:109:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 5 in 25 files


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

Comment on attachment 303665 [details]
Plumb drag destination actions through DragData

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

> Source/WebKit2/ChangeLog:17
> +        * Shared/API/Cocoa/_WKDragDestinationAction.h:

The leading underscore is inconsistent with how we normally name headers, which follows the name of the principal declaration in the header.

> Source/WebKit2/Shared/API/Cocoa/_WKDragDestinationAction.h:33
> +    WKDragDestinationActionAny     = UINT_MAX

Is UINT_MAX the right value to use for something that’s typed NSUInteger.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:69
> +- (WKDragDestinationAction)_webView:(WKWebView *)webView dragDestinationActionMaskForDraggingInfo:(void*)draggingInfo WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

Can the argument type be more specific than void* here?

[Wenson Hsieh]

Comment on attachment 303665 [details]
Plumb drag destination actions through DragData

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

>> Source/WebKit2/ChangeLog:17
>> +        * Shared/API/Cocoa/_WKDragDestinationAction.h:
> 
> The leading underscore is inconsistent with how we normally name headers, which follows the name of the principal declaration in the header.

Got it -- will rename to WKDragDestinationAction.h

>> Source/WebKit2/Shared/API/Cocoa/_WKDragDestinationAction.h:33
>> +    WKDragDestinationActionAny     = UINT_MAX
> 
> Is UINT_MAX the right value to use for something that’s typed NSUInteger.

Good catch. NSUIntegerMax seems more appropriate here.

>> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:69
>> +- (WKDragDestinationAction)_webView:(WKWebView *)webView dragDestinationActionMaskForDraggingInfo:(void*)draggingInfo WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Can the argument type be more specific than void* here?

id would certainly be better.

[Wenson Hsieh]

Created attachment 303679 [details]
Address feedback + fix builds

[WebKit Commit Bot]

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


ERROR: Source/WebKit/mac/DefaultDelegates/WebDefaultUIDelegate.mm:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKit/mac/DefaultDelegates/WebDefaultUIDelegate.mm:71:  The parameter name "]" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/Shared/API/Cocoa/WKDragDestinationAction.h:28:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 29 files


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

[Wenson Hsieh]

Created attachment 303694 [details]
Additional build fixes

[WebKit Commit Bot]

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


ERROR: Source/WebKit/mac/DefaultDelegates/WebDefaultUIDelegate.mm:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKit/mac/DefaultDelegates/WebDefaultUIDelegate.mm:71:  The parameter name "]" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/Shared/API/Cocoa/WKDragDestinationAction.h:32:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 29 files


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

[Wenson Hsieh]

Created attachment 303698 [details]
Fix the windows build

[WebKit Commit Bot]

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


ERROR: Source/WebKit/mac/DefaultDelegates/WebDefaultUIDelegate.mm:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKit/mac/DefaultDelegates/WebDefaultUIDelegate.mm:71:  The parameter name "]" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/Shared/API/Cocoa/WKDragDestinationAction.h:32:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 30 files


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

[Wenson Hsieh]

Created attachment 303702 [details]
Fix 32-bit Mac build

[WebKit Commit Bot]

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


ERROR: Source/WebKit/mac/DefaultDelegates/WebDefaultUIDelegate.mm:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKit/mac/DefaultDelegates/WebDefaultUIDelegate.mm:71:  The parameter name "]" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/Shared/API/Cocoa/WKDragDestinationAction.h:32:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 30 files


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

[Anders Carlsson]

Comment on attachment 303702 [details]
Fix 32-bit Mac build

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

> Source/WebCore/page/DragClient.h:49
> -    virtual DragDestinationAction actionMaskForDrag(const DragData&) = 0;
> +    virtual DragDestinationAction actionMaskForDrag(DragDataRef platformData) = 0;

It'd be even better if we could get rid of this altogether now that it's not called by WebCore anymore.

[Wenson Hsieh]

Comment on attachment 303702 [details]
Fix 32-bit Mac build

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

>> Source/WebCore/page/DragClient.h:49
>> +    virtual DragDestinationAction actionMaskForDrag(DragDataRef platformData) = 0;
> 
> It'd be even better if we could get rid of this altogether now that it's not called by WebCore anymore.

Good idea! I'll remove this. We can access the UI delegate forwarder directly in WebView instead.

[Wenson Hsieh]

Created attachment 303940 [details]
Remove WebDragClient::actionMaskForDrag

[WebKit Commit Bot]

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


ERROR: Source/WebKit/mac/DefaultDelegates/WebDefaultUIDelegate.mm:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKit/mac/DefaultDelegates/WebDefaultUIDelegate.mm:71:  The parameter name "]" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/Shared/API/Cocoa/WKDragDestinationAction.h:32:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 23 files


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

[Wenson Hsieh]

Landed in <https://trac.webkit.org/changeset/213662>

[Wenson Hsieh]

^ Please disregard the above.

[Wenson Hsieh]

Committed <https://trac.webkit.org/changeset/214403/webkit>.