Summary: | [WK2] Add a UI delegate SPI hook to enable or disable navigation on drop | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||||
Component: | WebKit2 | Assignee: | 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
Wenson Hsieh
2017-03-04 06:45:27 PST
Created attachment 303393 [details]
Patch
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.
Created attachment 303517 [details]
Fix Mac build
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. (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. Created attachment 303665 [details]
Plumb drag destination actions through DragData
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? 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. Created attachment 303679 [details]
Address feedback + fix builds
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.
Created attachment 303694 [details]
Additional build fixes
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.
Created attachment 303698 [details]
Fix the windows build
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.
Created attachment 303702 [details]
Fix 32-bit Mac build
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 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 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. Created attachment 303940 [details]
Remove WebDragClient::actionMaskForDrag
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.
Landed in <https://trac.webkit.org/changeset/213662> ^ Please disregard the above. Committed <https://trac.webkit.org/changeset/214403/webkit>. |