[webkit-reviews] review granted: [Bug 212605] Use OptionSet<DragOperation> for mask values : [Attachment 401035] Patch v13

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 212605: Use OptionSet<DragOperation> for mask values
https://bugs.webkit.org/show_bug.cgi?id=212605

Attachment 401035: Patch v13

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




--- Comment #31 from Darin Adler <darin at apple.com> ---
Comment on attachment 401035
  --> https://bugs.webkit.org/attachment.cgi?id=401035
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.