Bug 212885 – [IPC] Adopt enum class for DragSourceAction

• Component: DOM
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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.

[Radar WebKit Bug Importer]

<rdar://problem/64094134>

[David Kilzer (:ddkilzer)]

Created attachment 401303 [details]
Patch v1 (for EWS)

[David Kilzer (:ddkilzer)]

Created attachment 401304 [details]
Patch v2 (EWS)

Fix check-webkit-style issues.

[David Kilzer (:ddkilzer)]

Created attachment 401306 [details]
Patch v3 (EWS)

Try to fix Windows builds.

[David Kilzer (:ddkilzer)]

Created attachment 401363 [details]
Patch v4

[David Kilzer (:ddkilzer)]

Created attachment 401372 [details]
Patch v5

[David Kilzer (:ddkilzer)]

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.

[Darin Adler]

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?

[David Kilzer (:ddkilzer)]

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.

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

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.

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

Created attachment 401611 [details]
Patch v5 with Optional<DragSourceAction> in DragState

Posting for review/EWS.

[David Kilzer (:ddkilzer)]

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;

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Committed r262913: <https://trac.webkit.org/changeset/262913>