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.
<rdar://problem/64094134>
Created attachment 401303 [details] Patch v1 (for EWS)
Created attachment 401304 [details] Patch v2 (EWS) Fix check-webkit-style issues.
Created attachment 401306 [details] Patch v3 (EWS) Try to fix Windows builds.
Created attachment 401363 [details] Patch v4
Created attachment 401372 [details] Patch v5
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 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 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 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 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 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.
Created attachment 401611 [details] Patch v5 with Optional<DragSourceAction> in DragState Posting for review/EWS.
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;
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 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.
Committed r262913: <https://trac.webkit.org/changeset/262913>