Bug 212605 – Use OptionSet<DragOperation> for mask values

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

[David Kilzer (:ddkilzer)]

Use OptionSet<DragOperation> for mask values.

[David Kilzer (:ddkilzer)]

Created attachment 400783 [details]
Patch v1

Let EWS chew on the patch as there will probably be Win/WPE/GTK build failures and maybe some test failures.

[David Kilzer (:ddkilzer)]

Created attachment 400784 [details]
Patch v2

More EWS testing.  Corrected dumb mistakes for GTK and Win/WinCairo.

[Darin Adler]

Comment on attachment 400784 [details]
Patch v2

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

> Source/WebCore/page/DragActions.h:64
>  typedef enum {

Can we make this an enum class?

> Source/WebCore/page/DragActions.h:65
>      DragOperationNone    = 0,

Should remove this.

[David Kilzer (:ddkilzer)]

Created attachment 400833 [details]
Patch v3

Try to fix GTk/Win/WinCairo build failures.

May still need to fix failing layout test for mac-wk1 and mac-debug-wk1.

[Darin Adler]

Comment on attachment 400833 [details]
Patch v3

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

> Source/WebCore/dom/DataTransfer.cpp:603
> +        return { DragOperationNone };

This should just be:

    return { };

We don’t want a set with the value "0", we want a set with no values.

> Source/WebCore/page/DragController.cpp:696
> +static DragOperation defaultOperationForDrag(OptionSet<DragOperation> sourceOperationMask)

This should return Optional<DragOperation>, since it can return "no operation at all".

> Source/WebCore/page/EventHandler.h:166
> +        Optional<OptionSet<DragOperation>> operationMask;

Why does this need to be optional? How is nullopt different from an empty set here?

[David Kilzer (:ddkilzer)]

Comment on attachment 400833 [details]
Patch v3

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

>> Source/WebCore/dom/DataTransfer.cpp:603
>> +        return { DragOperationNone };
> 
> This should just be:
> 
>     return { };
> 
> We don’t want a set with the value "0", we want a set with no values.

I'll change this, but there's no material difference when the OptionSet<DragOperation> is created.

>> Source/WebCore/page/DragController.cpp:696
>> +static DragOperation defaultOperationForDrag(OptionSet<DragOperation> sourceOperationMask)
> 
> This should return Optional<DragOperation>, since it can return "no operation at all".

This seems like more complexity for little value since DragOperationNone is equivalent to an empty Optional<DragOperation>.

Is the goal here to remove all uses of DragOperationNone so it can be removed?  That seems to be the only reason to do this.

>> Source/WebCore/page/EventHandler.h:166
>> +        Optional<OptionSet<DragOperation>> operationMask;
> 
> Why does this need to be optional? How is nullopt different from an empty set here?

I was making a mechanical change here to use OptionSet<DragOperation> in place of just DragOperation>.

I will change it from Optional<OptionSet<DragOperation>> to OptionSet<DragOperation> since nullopt doesn't seem different from the empty set.

[Darin Adler]

Comment on attachment 400833 [details]
Patch v3

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

>>> Source/WebCore/page/DragController.cpp:696
>>> +static DragOperation defaultOperationForDrag(OptionSet<DragOperation> sourceOperationMask)
>> 
>> This should return Optional<DragOperation>, since it can return "no operation at all".
> 
> This seems like more complexity for little value since DragOperationNone is equivalent to an empty Optional<DragOperation>.
> 
> Is the goal here to remove all uses of DragOperationNone so it can be removed?  That seems to be the only reason to do this.

Yes, enumerations used with OptionSet should not have 0 values.

[David Kilzer (:ddkilzer)]

Comment on attachment 400833 [details]
Patch v3

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

>>> Source/WebCore/page/EventHandler.h:166
>>> +        Optional<OptionSet<DragOperation>> operationMask;
>> 
>> Why does this need to be optional? How is nullopt different from an empty set here?
> 
> I was making a mechanical change here to use OptionSet<DragOperation> in place of just DragOperation>.
> 
> I will change it from Optional<OptionSet<DragOperation>> to OptionSet<DragOperation> since nullopt doesn't seem different from the empty set.

Actually, nullopt may be significant.  Need to study the code a bit more.  (I"d prefer not to change the behavior in this patch, though.)

[David Kilzer (:ddkilzer)]

Created attachment 400892 [details]
Patch v4

Patch v4 addresses Darin's comment about defaultOperationForDrag() returning Optional<DragOperation>.  Pulling that thread (and then removing all uses of DragOperationNone basically converted every DragOperation return type into Optional<DragOperation> to replace the ability to use DragOperationNone.

I haven't addressed the issue of Optional<OptionSet<DragOperation>> as the above was more than enough to work through today.

This patch builds with macOS Debug.  I expect build failures for other platforms as I made a lot of changes to get rid of DragOperationNone.  (ChangeLogs are not up-to-date, so please ignore them.)

[David Kilzer (:ddkilzer)]

Comment on attachment 400892 [details]
Patch v4

Need to rebase the patch after Wenson's changes to DragController that he warned me about on Slack.  :)

[David Kilzer (:ddkilzer)]

Created attachment 400897 [details]
Patch v5

Same as Patch v4 but rebased past DragController.{cpp,h} changes in r262469.

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[David Kilzer (:ddkilzer)]

Created attachment 400952 [details]
Patch v6

Try to make GTK and Win/WinCairo ports build.

Still haven't fixed Optional<OptionSet<DragOperation>> yet.  Looking into that while EWS bots chew on this patch.

[David Kilzer (:ddkilzer)]

Created attachment 400957 [details]
Patch v7

Still trying to fix style, GTK and Win/WinCairo build failures.

[David Kilzer (:ddkilzer)]

Created attachment 400969 [details]
Patch v8

Try to fix GTK build.  WinCairo build looks good.

[David Kilzer (:ddkilzer)]

Created attachment 400971 [details]
Patch v9

Hopefully the last build fix for GTK.  WinCairo is building.

[David Kilzer (:ddkilzer)]

Created attachment 400977 [details]
Patch v10

Same as Patch v9, but changed Optional<OptionSet<DragOperation>> to OptionSet<DragOperation> in struct EventHandler::DragTargetResponse.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #17)
> Created attachment 400977 [details]
> Patch v10
> 
> Same as Patch v9, but changed Optional<OptionSet<DragOperation>> to
> OptionSet<DragOperation> in struct EventHandler::DragTargetResponse.

Turns out the Optional<> is significant as two mac-wk1 tests fail when I removed it:

fast/events/drag-and-drop.html
fast/events/prevent-drag-to-navigate.html

So we have to keep Optional<OptionSet<DragOperation>> for now.

Patch v9 seems like a winner (other than fast/css/user-drag-none.html crashing).  Need to figure out what I broke between Patch v7 and Patch v9.  Sigh.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #18)
> Patch v9 seems like a winner (other than fast/css/user-drag-none.html
> crashing).  Need to figure out what I broke between Patch v7 and Patch v9. 
> Sigh.

No changes to Windows code (only GTK code) between Patch v7 and Patch v9.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #19)
> (In reply to David Kilzer (:ddkilzer) from comment #18)
> > Patch v9 seems like a winner (other than fast/css/user-drag-none.html
> > crashing).  Need to figure out what I broke between Patch v7 and Patch v9. 
> > Sigh.
> 
> No changes to Windows code (only GTK code) between Patch v7 and Patch v9.

The crash was in code outside of WebKit, and it didn't reproduce, so I'm sure if this patch caused it.

Run: <https://ews-build.webkit.org/#/builders/10/builds/20183>
First results:  <https://ews-build.webkit.org/results/Windows-EWS/r400971-20183/results.html>

[David Kilzer (:ddkilzer)]

Created attachment 400998 [details]
Patch v11

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #21)
> Created attachment 400998 [details]
> Patch v11

Same as Patch v9 but with updated ChangeLogs.

Ready for review.

[David Kilzer (:ddkilzer)]

Comment on attachment 400998 [details]
Patch v11

Well, except for the typo I made.

[David Kilzer (:ddkilzer)]

Created attachment 400999 [details]
Patch v12

[David Kilzer (:ddkilzer)]

Created attachment 401035 [details]
Patch v13

Identical to Patch v12, but re-posting to avoid a revision with 120+ failing WPT tests.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #25)
> Created attachment 401035 [details]
> Patch v13
> 
> Identical to Patch v12, but re-posting to avoid a revision with 120+ failing
> WPT tests.

This patch is still ready for review.

[Alex Christensen]

Comment on attachment 401035 [details]
Patch v13

Why do we have both EnumTraits and OptionSetTraits for this enum?
Why didn't you make DragOperation an enum class?

[David Kilzer (:ddkilzer)]

(In reply to Alex Christensen from comment #27)
> Comment on attachment 401035 [details]
> Patch v13
> 
> Why do we have both EnumTraits and OptionSetTraits for this enum?

Because CoreIPC transmits both Optional<DragOperation> and OptionSet<DragOperation> types.

> Why didn't you make DragOperation an enum class?

Because this patch was already over 100 KB, and I didn't want to complicate it further.  I was going to fix that in a follow-up patch.  I can make the change here if you'd l;ke.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #28)
> (In reply to Alex Christensen from comment #27)
> > Comment on attachment 401035 [details]
> > Patch v13
> > 
> > Why do we have both EnumTraits and OptionSetTraits for this enum?
> 
> Because CoreIPC transmits both Optional<DragOperation> and
> OptionSet<DragOperation> types.

Trying to anticipate your next question:  "Why are you using both Optional<DragOperation> and OptionSet<DragOperation>?"

The reason I'm using both is that I let the code tell me which one it needed as I did the refactoring.

Optional<DragOperation> means the code wants a single DragOperation value (not multiple values), but it may or may not be set (since we removed DragOperationNone).

OptionSet<DragOperation> means the code wants one or more DragOperation values.

This is my second attempt at this refactoring.  The first attempt used OptionSet<DragOperation> everywhere, and it didn't feel correct since there were places where I wanted to know if a single DragOperation value was being used, so it didn't make sense to pass an OptionSet<DragOperation> into that code.

[David Kilzer (:ddkilzer)]

Still ready for review:
<https://bugs.webkit.org/attachment.cgi?id=401035&action=review>

[Darin Adler]

Comment on attachment 401035 [details]
Patch v13

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

> Source/WebCore/dom/DataTransfer.cpp:626
> +    if ((isGenericMove && operationMask.containsAll({ DragOperationCopy, DragOperationLink })) || operationMask == anyDragOperation())
>          return "all";

Seems like we should refine and write this differently; doesn’t seem like we literally want to be checking "== any" although we do want to preserve behavior. Maybe a containsAll call with the appropriate operations.

> Source/WebCore/page/DragActions.h:64
>  typedef enum {

Even if not converting to enum class, should really change typedef enum { } <name> to just enum <name>.

> Source/WebCore/page/DragController.cpp:701
> +    if (sourceOperationMask == anyDragOperation())
>          return DragOperationCopy;

While this does preserve behavior, this is peculiar. There must be a better way to preserve this behavior that doesn’t involve literally checking against "any". Can refine later, I guess.

> Source/WebCore/page/EventHandler.cpp:2353
> +        case DragOperationCopy:
> +            return "copy"_s;

If the default is "copy", maybe leave this case out.

> Source/WebCore/page/EventHandler.cpp:2385
> +            auto operationFromKeyword = convertDropZoneOperationToDragOperation(keywords[i]);
> +            if (operationFromKeyword) {

Put the declaration inside the if?

> Source/WebCore/page/win/DragControllerWin.cpp:47
> +    // FIXME: to match the macos behaviour we should return WTF::nullopt.

Capitalize "to"? Change "macos" to "macOS"?

> Source/WebCore/platform/gtk/GtkUtilities.cpp:159
> -    // We have no good way to detect DragOperationEvery other than
> +    // We have no good way to detect anyDragOperation() other than
>      // to use it when all applicable flags are on.
>      if (gdkAction & GDK_ACTION_COPY
>          && gdkAction & GDK_ACTION_MOVE
>          && gdkAction & GDK_ACTION_LINK)
> -        return DragOperationEvery;
> +        return anyDragOperation();

Unclear why this code is helpful. We should remove it unless there is some benefit.

> Source/WebCore/platform/gtk/GtkUtilities.cpp:176
> -    if (coreAction == DragOperationNone)
> +    if (coreAction.isEmpty())
>          return static_cast<GdkDragAction>(gdkAction);

This is unneeded. Code below will already do this without adding a special case for empty.

> Source/WebCore/platform/gtk/GtkUtilities.cpp:190
> +    if (coreAction == anyDragOperation() || coreAction.contains(DragOperationCopy))

This is redundant. No need to check against anyDragOperation(). Just checking contains(Copy) will do the same thing.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7312
> +        if (*operation & WebCore::DragOperationMove)
> +            return UIDropOperationMove;
> +        if (*operation & WebCore::DragOperationCopy)
> +            return UIDropOperationCopy;

This should be ==, not &, since it is an operation, not an OptionSet of operations.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7317
> +static OptionSet<WebCore::DragOperation> coreDragOperationForUIDropOperation(UIDropOperation dropOperation)

Why does this return a set? It seems like it’s translating a single operation into a set. Doesn’t make sense to me.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7446
> +    _page->dragEnded(WebCore::roundedIntPoint(client), WebCore::roundedIntPoint(global), currentDragOperation ? *currentDragOperation : OptionSet<WebCore::DragOperation>({ }));

We should teach OptionSet to construct from an Optional, so we don’t need to write this out, since it’s true for any Optional/OptionSet. Maybe as a cleanup pass after this patch, or in the patch would be OK too.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:4236
> +    if (operationMask == WebCore::anyDragOperation())
> +        return NSDragOperationEvery;

I suggest leaving this out. Should be fine to not literally have it be NSDragOperationEvery, but only include the actual operations.

> Source/WebKitLegacy/mac/WebView/WebViewInternal.h:95
> +typedef NSDragOperation CocoaDragOperation;

How about using instead of typedef? This is for C++ only, so we don’t need something C understands.

> Source/WebKitLegacy/mac/WebView/WebViewInternal.h:97
> +typedef uint64_t CocoaDragOperation;

Ditto.

> Source/WebKitLegacy/mac/WebView/WebViewInternal.h:98
> +#endif // USE(APPKIT)

On such a small #if/#endif pair the comment on #endif feels like nonhelpful clutter.

> Source/WebKitLegacy/win/WebCoreSupport/WebDragClient.cpp:174
>              if (effect & DROPEFFECT_COPY)
> -                operation = DragOperationCopy;
> +                operationMask = DragOperationCopy;
>              else if (effect & DROPEFFECT_LINK)
> -                operation = DragOperationLink;
> +                operationMask = DragOperationLink;
>              else if (effect & DROPEFFECT_MOVE)
> -                operation = DragOperationMove;
> +                operationMask = DragOperationMove;

Seems like this should not have else, and this should be adding instead of assigning. But that would be a behavior change. But otherwise this is not really an operation mask, it’s just pretending to be.

> Source/WebKitLegacy/win/WebView.cpp:5845
> +static DWORD dragOperationToDragCursor(Optional<DragOperation> operationMask)

An Optional<DragOperation> is not a "mask"; that would be an OptionSet.

> Source/WebKitLegacy/win/WebView.cpp:5851
> +    if (*operationMask & DragOperationCopy)

This should be ==, not &, since this is a value, not a mask.

[David Kilzer (:ddkilzer)]

Comment on attachment 401035 [details]
Patch v13

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

If I made the suggested change, I did _not_ note it below for brevity.

>> Source/WebCore/dom/DataTransfer.cpp:626
>>          return "all";
> 
> Seems like we should refine and write this differently; doesn’t seem like we literally want to be checking "== any" although we do want to preserve behavior. Maybe a containsAll call with the appropriate operations.

I could change this to operationMask.containsAll(anyDragOperation()).  Is that better?

I don't think it makes sense to list out every operation _again_ since we already do it for WTF::OptionSetTraits<>() and anyDragOperation() in DragActions.h.

I could add a method on OptionSet<DragOperation> that would do this check without having to list out all the operations (assuming an OptionSetTraits<> exists for the enum), but I'm not sure what to call it.  Maybe OptionSet<>::hasAll() or ::hasAllBitsSet()?

>> Source/WebCore/page/DragController.cpp:701
>>          return DragOperationCopy;
> 
> While this does preserve behavior, this is peculiar. There must be a better way to preserve this behavior that doesn’t involve literally checking against "any". Can refine later, I guess.

Using sourceOperationMask.containsAll(anyDragOperation()) as above for consistency.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7317
>> +static OptionSet<WebCore::DragOperation> coreDragOperationForUIDropOperation(UIDropOperation dropOperation)
> 
> Why does this return a set? It seems like it’s translating a single operation into a set. Doesn’t make sense to me.

Will change to return Optional<DragOperation> and use the new OptionSet(Optional) constructor.

[David Kilzer (:ddkilzer)]

Created attachment 401219 [details]
Patch for landing

[Darin Adler]

Comment on attachment 401035 [details]
Patch v13

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

>>> Source/WebCore/dom/DataTransfer.cpp:626
>>>          return "all";
>> 
>> Seems like we should refine and write this differently; doesn’t seem like we literally want to be checking "== any" although we do want to preserve behavior. Maybe a containsAll call with the appropriate operations.
> 
> I could change this to operationMask.containsAll(anyDragOperation()).  Is that better?
> 
> I don't think it makes sense to list out every operation _again_ since we already do it for WTF::OptionSetTraits<>() and anyDragOperation() in DragActions.h.
> 
> I could add a method on OptionSet<DragOperation> that would do this check without having to list out all the operations (assuming an OptionSetTraits<> exists for the enum), but I'm not sure what to call it.  Maybe OptionSet<>::hasAll() or ::hasAllBitsSet()?

I do think that a containsAll that lists all the current values is better. There’s no reason to assume that if we add another in the future that the behavior of this code should implicitly change.

I don’t think we really want to be checking for anyDragOperation here. It’s likely just sloppy programming. Do we really mean all of "copy, link, generic, private, move, and delete"? Seems highly unlikely.

It’s inappropriate for this to include all the bits in the OptionSet. If we added a new bit, it's highly unlikely that it would be meaningful on Windows. For example, seems almost certain that "delete" doesn’t matter one way or the other.

>>> Source/WebCore/page/DragController.cpp:701
>>>          return DragOperationCopy;
>> 
>> While this does preserve behavior, this is peculiar. There must be a better way to preserve this behavior that doesn’t involve literally checking against "any". Can refine later, I guess.
> 
> Using sourceOperationMask.containsAll(anyDragOperation()) as above for consistency.

Same comment as above. I’m almost certain that a mask that includes everything except for "Private", for example, or everything except for "Delete" should not cause this to function return DragOperationMove instead of DragOperationCopy.

It’s fine to preserve behavior, but it’s really nonsense logic.

[Darin Adler]

Comment on attachment 401035 [details]
Patch v13

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

>>>> Source/WebCore/dom/DataTransfer.cpp:626
>>>>          return "all";
>>> 
>>> Seems like we should refine and write this differently; doesn’t seem like we literally want to be checking "== any" although we do want to preserve behavior. Maybe a containsAll call with the appropriate operations.
>> 
>> I could change this to operationMask.containsAll(anyDragOperation()).  Is that better?
>> 
>> I don't think it makes sense to list out every operation _again_ since we already do it for WTF::OptionSetTraits<>() and anyDragOperation() in DragActions.h.
>> 
>> I could add a method on OptionSet<DragOperation> that would do this check without having to list out all the operations (assuming an OptionSetTraits<> exists for the enum), but I'm not sure what to call it.  Maybe OptionSet<>::hasAll() or ::hasAllBitsSet()?
> 
> I do think that a containsAll that lists all the current values is better. There’s no reason to assume that if we add another in the future that the behavior of this code should implicitly change.
> 
> I don’t think we really want to be checking for anyDragOperation here. It’s likely just sloppy programming. Do we really mean all of "copy, link, generic, private, move, and delete"? Seems highly unlikely.
> 
> It’s inappropriate for this to include all the bits in the OptionSet. If we added a new bit, it's highly unlikely that it would be meaningful on Windows. For example, seems almost certain that "delete" doesn’t matter one way or the other.

Basically: Using a form of "any" makes code that is doing what is likely an incorrect operation look logical, but falsely.

[Darin Adler]

Comment on attachment 401035 [details]
Patch v13

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

>>>> Source/WebCore/dom/DataTransfer.cpp:626
>>>>          return "all";
>>> 
>>> Seems like we should refine and write this differently; doesn’t seem like we literally want to be checking "== any" although we do want to preserve behavior. Maybe a containsAll call with the appropriate operations.
>> 
>> I could change this to operationMask.containsAll(anyDragOperation()).  Is that better?
>> 
>> I don't think it makes sense to list out every operation _again_ since we already do it for WTF::OptionSetTraits<>() and anyDragOperation() in DragActions.h.
>> 
>> I could add a method on OptionSet<DragOperation> that would do this check without having to list out all the operations (assuming an OptionSetTraits<> exists for the enum), but I'm not sure what to call it.  Maybe OptionSet<>::hasAll() or ::hasAllBitsSet()?
> 
> I do think that a containsAll that lists all the current values is better. There’s no reason to assume that if we add another in the future that the behavior of this code should implicitly change.
> 
> I don’t think we really want to be checking for anyDragOperation here. It’s likely just sloppy programming. Do we really mean all of "copy, link, generic, private, move, and delete"? Seems highly unlikely.
> 
> It’s inappropriate for this to include all the bits in the OptionSet. If we added a new bit, it's highly unlikely that it would be meaningful on Windows. For example, seems almost certain that "delete" doesn’t matter one way or the other.

Basically: Using a form of "any" makes code that is doing what is likely an incorrect operation look logical, but falsely.

[David Kilzer (:ddkilzer)]

Comment on attachment 401035 [details]
Patch v13

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

>>>>>> Source/WebCore/dom/DataTransfer.cpp:626
>>>>>>          return "all";
>>>>> 
>>>>> Seems like we should refine and write this differently; doesn’t seem like we literally want to be checking "== any" although we do want to preserve behavior. Maybe a containsAll call with the appropriate operations.
>>>> 
>>>> I could change this to operationMask.containsAll(anyDragOperation()).  Is that better?
>>>> 
>>>> I don't think it makes sense to list out every operation _again_ since we already do it for WTF::OptionSetTraits<>() and anyDragOperation() in DragActions.h.
>>>> 
>>>> I could add a method on OptionSet<DragOperation> that would do this check without having to list out all the operations (assuming an OptionSetTraits<> exists for the enum), but I'm not sure what to call it.  Maybe OptionSet<>::hasAll() or ::hasAllBitsSet()?
>>> 
>>> I do think that a containsAll that lists all the current values is better. There’s no reason to assume that if we add another in the future that the behavior of this code should implicitly change.
>>> 
>>> I don’t think we really want to be checking for anyDragOperation here. It’s likely just sloppy programming. Do we really mean all of "copy, link, generic, private, move, and delete"? Seems highly unlikely.
>>> 
>>> It’s inappropriate for this to include all the bits in the OptionSet. If we added a new bit, it's highly unlikely that it would be meaningful on Windows. For example, seems almost certain that "delete" doesn’t matter one way or the other.
>> 
>> Basically: Using a form of "any" makes code that is doing what is likely an incorrect operation look logical, but falsely.
> 
> Basically: Using a form of "any" makes code that is doing what is likely an incorrect operation look logical, but falsely.

Okay, I'll add the individual values in both places.

>>>> Source/WebCore/page/DragController.cpp:701
>>>>          return DragOperationCopy;
>>> 
>>> While this does preserve behavior, this is peculiar. There must be a better way to preserve this behavior that doesn’t involve literally checking against "any". Can refine later, I guess.
>> 
>> Using sourceOperationMask.containsAll(anyDragOperation()) as above for consistency.
> 
> Same comment as above. I’m almost certain that a mask that includes everything except for "Private", for example, or everything except for "Delete" should not cause this to function return DragOperationMove instead of DragOperationCopy.
> 
> It’s fine to preserve behavior, but it’s really nonsense logic.

Ditto.

[David Kilzer (:ddkilzer)]

Created attachment 401222 [details]
Patch for landing v2

[EWS]

Committed r262680: <https://trac.webkit.org/changeset/262680>

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

[Radar WebKit Bug Importer]

<rdar://problem/64069089>

[Radar WebKit Bug Importer]

<rdar://problem/64069091>

[David Kilzer (:ddkilzer)]

(In reply to EWS from comment #39)
> Committed r262680: <https://trac.webkit.org/changeset/262680>
> 
> All reviewed patches have been landed. Closing bug and clearing flags on
> attachment 401222 [details].

Follow-up fix (to address two missed "== anyDragOperation()" uses):
Committed r262681: <https://trac.webkit.org/changeset/262681>