Bug 212885 - [IPC] Adopt enum class for DragSourceAction
Summary: [IPC] Adopt enum class for DragSourceAction
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks: 213086 211988
  Show dependency treegraph
 
Reported: 2020-06-07 13:02 PDT by David Kilzer (:ddkilzer)
Modified: 2020-06-11 13:38 PDT (History)
10 users (show)

See Also:


Attachments
Patch v1 (for EWS) (61.49 KB, patch)
2020-06-07 13:03 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (EWS) (61.48 KB, patch)
2020-06-07 13:08 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v3 (EWS) (62.58 KB, patch)
2020-06-07 16:41 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v4 (37.83 KB, patch)
2020-06-08 13:13 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v5 (73.64 KB, patch)
2020-06-08 14:26 PDT, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff
Patch v5 with Optional<DragSourceAction> in DragState (72.24 KB, patch)
2020-06-10 18:04 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v5 with Optional<DragSourceAction> in DragState v2 (72.30 KB, patch)
2020-06-10 18:24 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2020-06-07 13:02:26 PDT
Adopt enum class for DragSourceAction.

Much like Bug 212605, this uses both Optional<DragSourceAction> and OptionSet<DragSourceAction> based on how the code uses the enum values.
Comment 1 Radar WebKit Bug Importer 2020-06-07 13:02:46 PDT
<rdar://problem/64094134>
Comment 2 David Kilzer (:ddkilzer) 2020-06-07 13:03:31 PDT
Created attachment 401303 [details]
Patch v1 (for EWS)
Comment 3 David Kilzer (:ddkilzer) 2020-06-07 13:08:02 PDT
Created attachment 401304 [details]
Patch v2 (EWS)

Fix check-webkit-style issues.
Comment 4 David Kilzer (:ddkilzer) 2020-06-07 16:41:30 PDT
Created attachment 401306 [details]
Patch v3 (EWS)

Try to fix Windows builds.
Comment 5 David Kilzer (:ddkilzer) 2020-06-08 13:13:52 PDT
Created attachment 401363 [details]
Patch v4
Comment 6 David Kilzer (:ddkilzer) 2020-06-08 14:26:43 PDT
Created attachment 401372 [details]
Patch v5
Comment 7 David Kilzer (:ddkilzer) 2020-06-08 14:29:05 PDT
Comment on attachment 401372 [details]
Patch v5

LOL...couldn't figure out why the patch doubled in size from v3 to v4, but that was due to the ChangeLogs.
Comment 8 Darin Adler 2020-06-08 14:41:49 PDT
Comment on attachment 401372 [details]
Patch v5

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

> Source/WebCore/page/DragState.h:38
> +    OptionSet<DragSourceAction> type;

Reading the code it seems like this is a single value, not a set. Can we try making this Optional instead of OptionSet?
Comment 9 David Kilzer (:ddkilzer) 2020-06-08 16:16:46 PDT
Comment on attachment 401372 [details]
Patch v5

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

>> Source/WebCore/page/DragState.h:38
>> +    OptionSet<DragSourceAction> type;
> 
> Reading the code it seems like this is a single value, not a set. Can we try making this Optional instead of OptionSet?

I don't know how to convert code in EventHandler::handleDrag() if I change DragState.type to Optional<DragSourceAction>.

> Source/WebCore/page/EventHandler.cpp:3824
>      // For drags starting in the selection, the user must wait between the mousedown and mousedrag,
>      // or else we bail on the dragging stuff and allow selection to occur
> -    if (m_mouseDownMayStartDrag && m_dragMayStartSelectionInstead && (dragState().type & DragSourceActionSelection) && event.event().timestamp() - m_mouseDownTimestamp < TextDragDelay) {
> +    if (m_mouseDownMayStartDrag && m_dragMayStartSelectionInstead && dragState().type.contains(DragSourceAction::Selection) && event.event().timestamp() - m_mouseDownTimestamp < TextDragDelay) {
>          ASSERT(event.event().type() == PlatformEvent::MouseMoved);
> -        if ((dragState().type & DragSourceActionImage)) {
> +        if (dragState().type.contains(DragSourceAction::Image)) {
>              // ... unless the mouse is over an image, then we start dragging just the image
> -            dragState().type = DragSourceActionImage;
> -        } else if (!(dragState().type & (DragSourceActionDHTML | DragSourceActionLink))) {
> +            dragState().type = DragSourceAction::Image;
> +        } else if (!dragState().type.containsAny({ DragSourceAction::DHTML, DragSourceAction::Link })) {
>              // ... but only bail if we're not over an unselectable element.

I don't know how to convert this code if DragState.type is not OptionSet<DragSourceAction>.  It assumes multiple values are set in DragState.type.
Comment 10 Darin Adler 2020-06-08 16:29:46 PDT
Comment on attachment 401372 [details]
Patch v5

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

>> Source/WebCore/page/EventHandler.cpp:3824
>>              // ... but only bail if we're not over an unselectable element.
> 
> I don't know how to convert this code if DragState.type is not OptionSet<DragSourceAction>.  It assumes multiple values are set in DragState.type.

No, while the code was written in a way that would work if both were set, what it actually does is that it checks if it is *either* of two values. The type is never *set* to more than one.
Comment 11 David Kilzer (:ddkilzer) 2020-06-08 16:54:09 PDT
Comment on attachment 401372 [details]
Patch v5

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

>>> Source/WebCore/page/EventHandler.cpp:3824
>>> -    if (m_mouseDownMayStartDrag && m_dragMayStartSelectionInstead && (dragState().type & DragSourceActionSelection) && event.event().timestamp() - m_mouseDownTimestamp < TextDragDelay) {
>>> +    if (m_mouseDownMayStartDrag && m_dragMayStartSelectionInstead && dragState().type.contains(DragSourceAction::Selection) && event.event().timestamp() - m_mouseDownTimestamp < TextDragDelay) {
>>>          ASSERT(event.event().type() == PlatformEvent::MouseMoved);
>>> -        if ((dragState().type & DragSourceActionImage)) {
>>> +        if (dragState().type.contains(DragSourceAction::Image)) {
>>>              // ... unless the mouse is over an image, then we start dragging just the image
>>> -            dragState().type = DragSourceActionImage;
>>> -        } else if (!(dragState().type & (DragSourceActionDHTML | DragSourceActionLink))) {
>>> +            dragState().type = DragSourceAction::Image;
>>> +        } else if (!dragState().type.containsAny({ DragSourceAction::DHTML, DragSourceAction::Link })) {
>>>              // ... but only bail if we're not over an unselectable element.
>> 
>> I don't know how to convert this code if DragState.type is not OptionSet<DragSourceAction>.  It assumes multiple values are set in DragState.type.
> 
> No, while the code was written in a way that would work if both were set, what it actually does is that it checks if it is *either* of two values. The type is never *set* to more than one.

Well, I wasn't referring just to this line:

        } else if (!dragState().type.containsAny({ DragSourceAction::DHTML, DragSourceAction::Link })) {

I was referring to the outer `if` block:

    if ([...] && dragState().type.contains(DragSourceAction::Selection) && [...]) {
        [.A.]
        if (dragState().type.contains(DragSourceAction::Image)) {
            [.B.]
        } else if (!dragState().type.containsAny({ DragSourceAction::DHTML, DragSourceAction::Link })) {
            [.C.]
        } else {
            [.D.]
        }
    }

Since one one value of DragState::type can be set, I can just delete most of this code like this?

    if ([...] && dragState().type.contains(DragSourceAction::Selection) && [...]) {
        [.A.]
        [.D.]
    }

Is that what you're talking about?

I originally hesitated in doing this because this is a refactoring patch, and (to me) it feels wrong to simply remove code like this when refactoring, but I can do it if that's what you want me to try.
Comment 12 Darin Adler 2020-06-08 16:58:16 PDT
Comment on attachment 401372 [details]
Patch v5

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

>>>> Source/WebCore/page/EventHandler.cpp:3824
>>>>              // ... but only bail if we're not over an unselectable element.
>>> 
>>> I don't know how to convert this code if DragState.type is not OptionSet<DragSourceAction>.  It assumes multiple values are set in DragState.type.
>> 
>> No, while the code was written in a way that would work if both were set, what it actually does is that it checks if it is *either* of two values. The type is never *set* to more than one.
> 
> Well, I wasn't referring just to this line:
> 
>         } else if (!dragState().type.containsAny({ DragSourceAction::DHTML, DragSourceAction::Link })) {
> 
> I was referring to the outer `if` block:
> 
>     if ([...] && dragState().type.contains(DragSourceAction::Selection) && [...]) {
>         [.A.]
>         if (dragState().type.contains(DragSourceAction::Image)) {
>             [.B.]
>         } else if (!dragState().type.containsAny({ DragSourceAction::DHTML, DragSourceAction::Link })) {
>             [.C.]
>         } else {
>             [.D.]
>         }
>     }
> 
> Since one one value of DragState::type can be set, I can just delete most of this code like this?
> 
>     if ([...] && dragState().type.contains(DragSourceAction::Selection) && [...]) {
>         [.A.]
>         [.D.]
>     }
> 
> Is that what you're talking about?
> 
> I originally hesitated in doing this because this is a refactoring patch, and (to me) it feels wrong to simply remove code like this when refactoring, but I can do it if that's what you want me to try.

Refactoring this to be explicitly "OptionSet" when before it was not explicit is making the confusion worse, so I think we need to straighten this out first.

I guess there is some code can set the type to more than one thing. But this seems disorganized and messy. I think it’s not really a general set. It’s just one type plus possibly "selection". Seems really messy. I suppose you aren’t making it worse, but all this "hasExactlyOneBitSet" stuff is just what happens when we make the data structure too confusing. Clearly the "selection" is an independent flag, and need not be included in an option set.

Anyway, OK if you don’t want to clear this up, but I think it’s messy to use a "set" for this.
Comment 13 David Kilzer (:ddkilzer) 2020-06-10 18:04:33 PDT
Created attachment 401611 [details]
Patch v5 with Optional<DragSourceAction> in DragState

Posting for review/EWS.
Comment 14 David Kilzer (:ddkilzer) 2020-06-10 18:20:46 PDT
Comment on attachment 401611 [details]
Patch v5 with Optional<DragSourceAction> in DragState

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

Experiment to change DragState::type (now ::action) from OptionSet<> to Optional<>.

I didn't update ChangeLog; just testing code changes.

Note that we can't completely remove DragSourceAction::Selection since it's used elsewhere.

Does this look better, Darin?  Any concerns about the changes I pointed out?

> Source/WTF/wtf/OptionSet.h:216
> +    constexpr bool hasExactlyOneBitSet() const
> +    {
> +        return m_storage && !(m_storage & (m_storage - 1));
> +    }
> +
> +    constexpr Optional<E> toSingleValue() const
> +    {
> +        return hasExactlyOneBitSet() ? Optional<E>(static_cast<E>(m_storage)) : WTF::nullopt;
> +    }

These can be removed now since they're not used, too.

> Source/WebCore/page/DragState.h:39
> +    bool includesSelectionAction { false };
> +    Optional<DragSourceAction> action;

Added .includesSelectionAction to hold DragSourceAction::Selection state.
Renamed .type to .action after changing from OptionSet<> to Optional<>.

> Source/WebCore/page/EventHandler.cpp:3650
> +    ASSERT(dragState().includesSelectionAction ^ !!dragState().action);

Based on how this method worked before, only one of dragState().includesSelectionAction and dragState().action should be set.

> Source/WebCore/page/EventHandler.cpp:3658
> -    switch (dragState().type) {
> -    case DragSourceActionSelection:
> +    if (dragState().includesSelectionAction)
>          threshold = TextDragHysteresis;
> -        break;
> -    case DragSourceActionImage:
> +    else if (dragState().action) {
> +        switch (*dragState().action) {
> +        case DragSourceAction::Selection:
> +            ASSERT_NOT_REACHED();
> +            break;

This got kind of weird since we still need case DragSourceAction::Selection in the switch statement (so we're checking every value), but it's now an ASSERT_NOT_REACHED().

> Source/WebCore/page/EventHandler.cpp:3779
> +    OptionSet<DragSourceAction> actionMask = dragState().action;
> +    if (dragState().includesSelectionAction)
> +        actionMask.add(DragSourceAction::Selection);
>      if (auto* page = m_frame.page())
> -        page->dragController().prepareForDragStart(m_frame, dragState().type, *dragState().source, dataTransfer, m_mouseDownContentsPosition);
> +        page->dragController().prepareForDragStart(m_frame, actionMask, *dragState().source, dataTransfer, m_mouseDownContentsPosition);

Had to manually add back DragSourceAction::Selection here using a local variable.

> Source/WebCore/page/EventHandler.cpp:3849
> -    if (!ExactlyOneBitSet(dragState().type)) {
> -        ASSERT(dragState().type & DragSourceActionSelection);
> -        ASSERT(ExactlyOneBitSet(static_cast<DragSourceAction>(dragState().type & ~DragSourceActionSelection)));
> -
> -        dragState().type = DragSourceActionSelection;
> +    if (dragState().action) {
> +        ASSERT(dragState().includesSelectionAction);
> +        dragState().action = WTF::nullopt;

This changed because DragState::action can't have more than one value, so if it's set, we need to clear it.

Should probably also do this here:

        dragState().includesSelectionAction = true;
Comment 15 David Kilzer (:ddkilzer) 2020-06-10 18:24:17 PDT
Created attachment 401613 [details]
Patch v5 with Optional<DragSourceAction> in DragState v2

Added "dragState().includesSelectionAction = true;;" to this:

    if (dragState().action) {
        ASSERT(dragState().includesSelectionAction);
        dragState().action = WTF::nullopt;
        dragState().includesSelectionAction = true;
    }
Comment 16 David Kilzer (:ddkilzer) 2020-06-11 10:57:22 PDT
Comment on attachment 401372 [details]
Patch v5

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

>>>>> Source/WebCore/page/EventHandler.cpp:3824
>>>>>              // ... but only bail if we're not over an unselectable element.
>>>> 
>>>> I don't know how to convert this code if DragState.type is not OptionSet<DragSourceAction>.  It assumes multiple values are set in DragState.type.
>>> 
>>> No, while the code was written in a way that would work if both were set, what it actually does is that it checks if it is *either* of two values. The type is never *set* to more than one.
>> 
>> Well, I wasn't referring just to this line:
>> 
>>         } else if (!dragState().type.containsAny({ DragSourceAction::DHTML, DragSourceAction::Link })) {
>> 
>> I was referring to the outer `if` block:
>> 
>>     if ([...] && dragState().type.contains(DragSourceAction::Selection) && [...]) {
>>         [.A.]
>>         if (dragState().type.contains(DragSourceAction::Image)) {
>>             [.B.]
>>         } else if (!dragState().type.containsAny({ DragSourceAction::DHTML, DragSourceAction::Link })) {
>>             [.C.]
>>         } else {
>>             [.D.]
>>         }
>>     }
>> 
>> Since one one value of DragState::type can be set, I can just delete most of this code like this?
>> 
>>     if ([...] && dragState().type.contains(DragSourceAction::Selection) && [...]) {
>>         [.A.]
>>         [.D.]
>>     }
>> 
>> Is that what you're talking about?
>> 
>> I originally hesitated in doing this because this is a refactoring patch, and (to me) it feels wrong to simply remove code like this when refactoring, but I can do it if that's what you want me to try.
> 
> Refactoring this to be explicitly "OptionSet" when before it was not explicit is making the confusion worse, so I think we need to straighten this out first.
> 
> I guess there is some code can set the type to more than one thing. But this seems disorganized and messy. I think it’s not really a general set. It’s just one type plus possibly "selection". Seems really messy. I suppose you aren’t making it worse, but all this "hasExactlyOneBitSet" stuff is just what happens when we make the data structure too confusing. Clearly the "selection" is an independent flag, and need not be included in an option set.
> 
> Anyway, OK if you don’t want to clear this up, but I think it’s messy to use a "set" for this.

Current plan:  I'm going to land "Patch v5" as-is, and file a follow-up bug to track your concern.  Will also add a FIXME to DragState::type referencing the bug.
Comment 17 David Kilzer (:ddkilzer) 2020-06-11 11:55:54 PDT
Committed r262913: <https://trac.webkit.org/changeset/262913>