Bug 169168

Summary: [WK2] Add a UI delegate SPI hook to enable or disable navigation on drop
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebKit2Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cdumez, commit-queue, dbates, japhet, mitz, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 170346    
Bug Blocks: 170163    
Attachments:
Description Flags
Patch
none
Fix Mac build
none
Plumb drag destination actions through DragData
none
Address feedback + fix builds
none
Additional build fixes
none
Fix the windows build
none
Fix 32-bit Mac build
none
Remove WebDragClient::actionMaskForDrag thorton: review+

Description Wenson Hsieh 2017-03-04 06:45:27 PST
Add a UI delegate SPI hook to enable or disable navigation on drop
Comment 1 Wenson Hsieh 2017-03-04 06:46:19 PST
<rdar://problem/30688374>
Comment 2 Wenson Hsieh 2017-03-04 07:09:07 PST
Created attachment 303393 [details]
Patch
Comment 3 WebKit Commit Bot 2017-03-04 07:11:05 PST
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.
Comment 4 Wenson Hsieh 2017-03-06 08:57:44 PST
Created attachment 303517 [details]
Fix Mac build
Comment 5 WebKit Commit Bot 2017-03-06 09:00:31 PST
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 6 mitz 2017-03-06 09:06:05 PST
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.
Comment 7 Wenson Hsieh 2017-03-06 09:11:12 PST
(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.
Comment 8 mitz 2017-03-06 09:16:20 PST
(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.
Comment 9 Wenson Hsieh 2017-03-07 09:37:30 PST
Created attachment 303665 [details]
Plumb drag destination actions through DragData
Comment 10 WebKit Commit Bot 2017-03-07 09:39:20 PST
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 11 mitz 2017-03-07 09:47:54 PST
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?
Comment 12 Wenson Hsieh 2017-03-07 10:06:13 PST
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.
Comment 13 Wenson Hsieh 2017-03-07 10:59:27 PST
Created attachment 303679 [details]
Address feedback + fix builds
Comment 14 WebKit Commit Bot 2017-03-07 11:00:49 PST
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.
Comment 15 Wenson Hsieh 2017-03-07 11:52:33 PST
Created attachment 303694 [details]
Additional build fixes
Comment 16 WebKit Commit Bot 2017-03-07 11:54:56 PST
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.
Comment 17 Wenson Hsieh 2017-03-07 12:24:16 PST
Created attachment 303698 [details]
Fix the windows build
Comment 18 WebKit Commit Bot 2017-03-07 12:26:18 PST
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.
Comment 19 Wenson Hsieh 2017-03-07 12:42:27 PST
Created attachment 303702 [details]
Fix 32-bit Mac build
Comment 20 WebKit Commit Bot 2017-03-07 12:44:18 PST
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.
Comment 21 Anders Carlsson 2017-03-08 14:13:56 PST
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.
Comment 22 Wenson Hsieh 2017-03-09 08:32:59 PST
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.
Comment 23 Wenson Hsieh 2017-03-09 11:32:32 PST
Created attachment 303940 [details]
Remove WebDragClient::actionMaskForDrag
Comment 24 WebKit Commit Bot 2017-03-09 11:34:48 PST
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.
Comment 25 Wenson Hsieh 2017-03-09 13:51:59 PST
Landed in <https://trac.webkit.org/changeset/213662>
Comment 26 Wenson Hsieh 2017-03-09 13:52:33 PST
^ Please disregard the above.
Comment 27 Wenson Hsieh 2017-03-29 08:45:33 PDT
Committed <https://trac.webkit.org/changeset/214403/webkit>.