Don't use casts to convert between WebCore types and {Web,WK}DragDestinationActionMask/{NS,WK}DragOperation. Requested follow-up fixes by Darin Adler from Bug 212115 Comment #27: <https://bugs.webkit.org/show_bug.cgi?id=212115#c27>
Created attachment 400540 [details] Patch v1
Created attachment 400541 [details] Patch v2
(In reply to David Kilzer (:ddkilzer) from comment #2) > Created attachment 400541 [details] > Patch v2 Fix bug title.
Created attachment 400542 [details] Patch v3
(In reply to David Kilzer (:ddkilzer) from comment #4) > Created attachment 400542 [details] > Patch v3 Fix a typo and sort new file in Xcode project.
Comment on attachment 400542 [details] Patch v3 Sigh. Need to test iOS build.
Created attachment 400544 [details] Patch v4
(In reply to David Kilzer (:ddkilzer) from comment #7) > Created attachment 400544 [details] > Patch v4 - Try to fix the iOS builds. (Taking too long on my old MBP to build, so uploading with fix for first known issue.) - I'm pretty sure I broke some layout tests on macOS, so not marking r? until I fix that.
Comment on attachment 400544 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=400544&action=review > Source/WebCore/page/DragActions.h:35 > -// WebCore::DragDestinationAction must be kept in sync with WebDragDestinationAction and WKDragDestinationAction. > +// Must be kept in sync with WebDragDestinationAction and WKDragDestinationAction. You should just remove the comment. Because we are using conversion functions this does *not* need to be kept in sync. > Source/WebCore/page/DragActions.h:63 > +// Must be kept in sync with NSDragOperation. Is this true? If so, why? (Eventually we need to change this to an enum class and OptionSet.) Can we remove DragOperationEvery? > Source/WebKitLegacy/mac/WebCoreSupport/WebDragClient.mm:81 > + return WebDragDestinationActionNone; I think this should be ASSERT_NOT_REACHED(). We would never really want to return WebDragDestinationActionNone. > Source/WebKitLegacy/win/WebCoreSupport/WebDragClient.cpp:73 > + return WebDragDestinationActionNone; Ditto.
Created attachment 400628 [details] Patch v5
(In reply to David Kilzer (:ddkilzer) from comment #10) > Created attachment 400628 [details] > Patch v5 Just changes Patch v4 to use USE(APPKIT) instead of PLATFORM(APPKIT) due to think-o late last night.
Comment on attachment 400544 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=400544&action=review >> Source/WebCore/page/DragActions.h:35 >> +// Must be kept in sync with WebDragDestinationAction and WKDragDestinationAction. > > You should just remove the comment. Because we are using conversion functions this does *not* need to be kept in sync. I will change to "See WebDragDestinationAction and WKDragDestinationAction." since that seems useful. >> Source/WebCore/page/DragActions.h:63 >> +// Must be kept in sync with NSDragOperation. > > Is this true? If so, why? > > (Eventually we need to change this to an enum class and OptionSet.) > > Can we remove DragOperationEvery? ] Is this true? If so, why? I will change this to "See NSDragOperation." Was trying to keep the comments similar to DragDestinationAction above. ] (Eventually we need to change this to an enum class and OptionSet.) ] ] Can we remove DragOperationEvery? I was working on a patch to update DragOperation, but I wanted to address your comments before moving forward, even though you requested changes to DragDestinationAction and DragOperation changes in the same comment. These changes to use OptionSet<> for these Drag enums is blocking Bug 211988, which is what I'm desperately trying to work my way back to without getting side-tracked any further. >> Source/WebKitLegacy/mac/WebCoreSupport/WebDragClient.mm:81 >> + return WebDragDestinationActionNone; > > I think this should be ASSERT_NOT_REACHED(). We would never really want to return WebDragDestinationActionNone. I will add ASSERT_NOT_REACHED(), but we need to keep the final `return WebDragDestinationActionNone` statement to avoid undefined behavior since this method has a return value. I could also use RELEASE_ ASSERT_NOT_REACHED() and remove the final `return WebDragDestinationActionNone` statement if that's preferred.
Created attachment 400631 [details] Patch v6
(In reply to David Kilzer (:ddkilzer) from comment #13) > Created attachment 400631 [details] > Patch v6 - Address Darin's first round of feedback. - Try to fix iOS build failure for reals this time. Remaining: - Fix macOS test failures.
Comment on attachment 400544 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=400544&action=review >>> Source/WebCore/page/DragActions.h:63 >>> +// Must be kept in sync with NSDragOperation. >> >> Is this true? If so, why? >> >> (Eventually we need to change this to an enum class and OptionSet.) >> >> Can we remove DragOperationEvery? > > ] Is this true? If so, why? > I will change this to "See NSDragOperation." Was trying to keep the comments similar to DragDestinationAction above. > > ] (Eventually we need to change this to an enum class and OptionSet.) > ] > ] Can we remove DragOperationEvery? > > I was working on a patch to update DragOperation, but I wanted to address your comments before moving forward, even though you requested changes to DragDestinationAction and DragOperation changes in the same comment. > > These changes to use OptionSet<> for these Drag enums is blocking Bug 211988, which is what I'm desperately trying to work my way back to without getting side-tracked any further. ] ] Is this true? If so, why? ] ] I will change this to "See NSDragOperation." Was trying to keep the comments similar to DragDestinationAction above. Oh my. In WebKitLegacy, we have one method for iOS that uses uint64_t instead of NSDragOperation because that enum isn't defined on that platform in WebView.mm: - (WebCore::DragData)dragDataForSession:(id <UIDropSession>)session client:(CGPoint)clientPosition global:(CGPoint)globalPosition operation:(uint64_t)operation And all the methods that call it: - (uint64_t)_enteredDataInteraction:(id <UIDropSession>)session client:(CGPoint)clientPosition global:(CGPoint)globalPosition operation:(uint64_t)operation - (uint64_t)_updatedDataInteraction:(id <UIDropSession>)session client:(CGPoint)clientPosition global:(CGPoint)globalPosition operation:(uint64_t)operation - (void)_exitedDataInteraction:(id <UIDropSession>)session client:(CGPoint)clientPosition global:(CGPoint)globalPosition operation:(uint64_t)operation - (void)_performDataInteraction:(id <UIDropSession>)session client:(CGPoint)clientPosition global:(CGPoint)globalPosition operation:(uint64_t)operation - (BOOL)_tryToPerformDataInteraction:(id <UIDropSession>)session client:(CGPoint)clientPosition global:(CGPoint)globalPosition operation:(uint64_t)operation This is the perfect little mess that this static_cast<> was obscuring in the -dragDataForSession:client:global:operation: method: auto dragOperationMask = static_cast<WebCore::DragOperation>(operation);
Created attachment 400642 [details] Patch v7
(In reply to David Kilzer (:ddkilzer) from comment #16) > Created attachment 400642 [details] > Patch v7 Tests were failing because I was using '&' instead of '|' operator to add values to a mask. Patch is ready for review.
Comment on attachment 400642 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=400642&action=review > Source/WebKitLegacy/mac/WebView/WebView.mm:601 > +WebCore::DragOperation coreDragOperationMask(CocoaDragOperation operation) > +{ > +#if !USE(APPKIT) > +#define NSDragOperationCopy 1 > +#define NSDragOperationLink 2 > +#define NSDragOperationGeneric 4 > +#define NSDragOperationPrivate 8 > +#define NSDragOperationMove 16 > +#define NSDragOperationDelete 32 > +#endif I'm open to better ways to do this (as well as CocoaDragOperation). I could use WebCore::DragOperation values instead of hard-coding integers, but then we have to keep WebCore::DragOperation in sync with NSDragOperation for reals.
Comment on attachment 400642 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=400642&action=review > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7323 > + WebCore::DragOperation dragOperationMask = coreDragOperationMask(session.allowsMoveOperation ? WebCore::DragOperationEvery : (WebCore::DragOperationEvery & ~WebCore::DragOperationMove)); WebCore::DragOperationEvery and WebCore::DragOperationMove are already a WebCore::DragOperation. We should not call coreDragOperationMask. This only works because the values are the same, but this is the whole point of the patch really. > Source/WebKitLegacy/mac/WebView/WebView.mm:601 > +#if !USE(APPKIT) > +#define NSDragOperationCopy 1 > +#define NSDragOperationLink 2 > +#define NSDragOperationGeneric 4 > +#define NSDragOperationPrivate 8 > +#define NSDragOperationMove 16 > +#define NSDragOperationDelete 32 > +#endif I suggest constants instead of macros. Like: constexpr uint64_t NSDragOperationCopy = 1; Then you don’t have to do all the #undef. Further, might also be good if this could go into an SPI header instead of being here, but maybe not a rush to do that right now.
Comment on attachment 400642 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=400642&action=review >>> Source/WebKitLegacy/mac/WebView/WebView.mm:601 >>> +#endif >> >> I'm open to better ways to do this (as well as CocoaDragOperation). >> >> I could use WebCore::DragOperation values instead of hard-coding integers, but then we have to keep WebCore::DragOperation in sync with NSDragOperation for reals. > > I suggest constants instead of macros. Like: > > constexpr uint64_t NSDragOperationCopy = 1; > > Then you don’t have to do all the #undef. > > Further, might also be good if this could go into an SPI header instead of being here, but maybe not a rush to do that right now. ] Further, might also be good if this could go into an SPI header instead of being here, but maybe not a rush to do that right now. The reason that this is `uint64_t` on iOS (and in the Objective-C methods) is that there's an enum defined in UIKIt that matches a subset of the NSDragOperation values, but we can't include the UIKit header in WebKitLegacy due to circular dependency issues. A better approach here might be to mirror the UIKit typedef in an SPI header, then use that instead. I'll add a FIXME here and explore that in the next patch for DragOperation.
Created attachment 400647 [details] Patch for landing
Comment on attachment 400642 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=400642&action=review >> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7323 >> + WebCore::DragOperation dragOperationMask = coreDragOperationMask(session.allowsMoveOperation ? WebCore::DragOperationEvery : (WebCore::DragOperationEvery & ~WebCore::DragOperationMove)); > > WebCore::DragOperationEvery and WebCore::DragOperationMove are already a WebCore::DragOperation. We should not call coreDragOperationMask. This only works because the values are the same, but this is the whole point of the patch really. Fixed. >>>> Source/WebKitLegacy/mac/WebView/WebView.mm:601 >>>> +#endif >>> >>> I'm open to better ways to do this (as well as CocoaDragOperation). >>> >>> I could use WebCore::DragOperation values instead of hard-coding integers, but then we have to keep WebCore::DragOperation in sync with NSDragOperation for reals. >> >> I suggest constants instead of macros. Like: >> >> constexpr uint64_t NSDragOperationCopy = 1; >> >> Then you don’t have to do all the #undef. >> >> Further, might also be good if this could go into an SPI header instead of being here, but maybe not a rush to do that right now. > > ] Further, might also be good if this could go into an SPI header instead of being here, but maybe not a rush to do that right now. > > The reason that this is `uint64_t` on iOS (and in the Objective-C methods) is that there's an enum defined in UIKIt that matches a subset of the NSDragOperation values, but we can't include the UIKit header in WebKitLegacy due to circular dependency issues. A better approach here might be to mirror the UIKit typedef in an SPI header, then use that instead. > > I'll add a FIXME here and explore that in the next patch for DragOperation. Better yet, I defined the two _UIDragOperation constants and changed the #If/#endif into #if/#else/#endif.
(In reply to David Kilzer (:ddkilzer) from comment #21) > Created attachment 400647 [details] > Patch for landing Need to investigate fast/events/drag-and-drop.html failures before landing.
Created attachment 400680 [details] Patch v9
(In reply to Darin Adler from comment #9) > Comment on attachment 400544 [details] > Patch v4 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400544&action=review > > Can we remove DragOperationEvery? Well that question was prescient. The fast/events/drag-and-drop.html failure on Mac WK1 bots was due to the new conversion functions returning the minimum bits set instead of DragOperationEvery. I guess I'll remove DragOperationEvery in this patch as well.
(In reply to David Kilzer (:ddkilzer) from comment #25) > (In reply to Darin Adler from comment #9) > > Comment on attachment 400544 [details] > > Patch v4 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=400544&action=review > > > > Can we remove DragOperationEvery? > > Well that question was prescient. The fast/events/drag-and-drop.html > failure on Mac WK1 bots was due to the new conversion functions returning > the minimum bits set instead of DragOperationEvery. > > I guess I'll remove DragOperationEvery in this patch as well. Going to split out the DragOperation changes into a separate patch (the basis for using OptionSet<DragOperation>) since this patch is getting too big for a small follow-up.
Created attachment 400748 [details] Patch for landing v2
Comment on attachment 400748 [details] Patch for landing v2 I didn't change the Source/WebKitLegacy/win code since Patch v9, and WPE build is broken due to TLS certificate issues, so marking cq+.
Committed r262395: <https://trac.webkit.org/changeset/262395> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400748 [details].
<rdar://problem/63845799>