Bug 212507 – Don't use casts to convert between WebCore::DragDestinationAction and {Web,WK}DragDestinationAction types

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

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>

[David Kilzer (:ddkilzer)]

Created attachment 400540 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Created attachment 400541 [details]
Patch v2

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #2)
> Created attachment 400541 [details]
> Patch v2

Fix bug title.

[David Kilzer (:ddkilzer)]

Created attachment 400542 [details]
Patch v3

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

Comment on attachment 400542 [details]
Patch v3

Sigh.  Need to test iOS build.

[David Kilzer (:ddkilzer)]

Created attachment 400544 [details]
Patch v4

[David Kilzer (:ddkilzer)]

(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.

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

Created attachment 400628 [details]
Patch v5

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 400631 [details]
Patch v6

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

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);

[David Kilzer (:ddkilzer)]

Created attachment 400642 [details]
Patch v7

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

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.

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 400647 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

Created attachment 400680 [details]
Patch v9

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

Created attachment 400748 [details]
Patch for landing v2

[David Kilzer (:ddkilzer)]

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+.

[EWS]

Committed r262395: <https://trac.webkit.org/changeset/262395>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400748 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63845799>