[webkit-reviews] review granted: [Bug 212885] [IPC] Adopt enum class for DragSourceAction : [Attachment 401372] Patch v5

[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 212885: [IPC] Adopt enum class for DragSourceAction
https://bugs.webkit.org/show_bug.cgi?id=212885

Attachment 401372: Patch v5

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




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