Use OptionSet<DragDestinationAction> for mask values. WebCore::DragData::m_dragDestinationAction can't be validated as an individual enum value in IPC::Decoder and IPC::Encoder since it's a bit mask.
<rdar://problem/63423380>
Created attachment 399793 [details] Patch v1
So the mac-debug-wk1 layout tests fail here: <https://ews-build.webkit.org/#/builders/32/builds/10215> Due to this assert in OptionSet constructor: constexpr OptionSet(T t) : m_storage(static_cast<StorageType>(t)) { #ifndef NDEBUG // This assertion will conflict with the constexpr attribute if we enable it on NDEBUG builds. ASSERT_WITH_MESSAGE(!m_storage || hasOneBitSet(m_storage), "Enumerator is not a zero or a positive power of two."); #endif } Because DragData() constructors want to set an default OptionSet<DragApplicationData> value of DragDestinationAction::Any (which has all bits set): WEBCORE_EXPORT DragData(DragDataRef, const IntPoint& clientPosition, const IntPoint& globalPosition, DragOperation, DragApplicationFlags = DragApplicationNone, OptionSet<DragDestinationAction> actionMask = DragDestinationAction::Any); WEBCORE_EXPORT DragData(const String& dragStorageName, const IntPoint& clientPosition, const IntPoint& globalPosition, DragOperation, DragApplicationFlags = DragApplicationNone, OptionSet<DragDestinationAction> actionMask = DragDestinationAction::Any); I see a few ways to solve this: 1. Use OptionSet::fromRaw() to work around the debug assertion. 2. Get rid of DragDestinationAction::Any and write a template function like OptionSet::allBits() that does the equivalent of logical-or-ing all the enum values together to produce an equivalent to DragDestinationAction::Any. This would also likely require the use of EnumTraits to list all the valid values so they could be logical-or-ed together. 3. Change OptionSet() constructors (both for individual enum values and for std::initializer_list) to verify that the value(s) passed in are valid enum value(s), again after defining EnumTraits for the bit masks. Note that #2 and #3 might be a lot more work because most OptionSet() enums don't have EnumTraits defined, but would probably catch more errors in the long term.
(In reply to David Kilzer (:ddkilzer) from comment #3) > I see a few ways to solve this: > > 1. Use OptionSet::fromRaw() to work around the debug assertion. > > 2. Get rid of DragDestinationAction::Any and write a template function like > OptionSet::allBits() that does the equivalent of logical-or-ing all the enum > values together to produce an equivalent to DragDestinationAction::Any. > This would also likely require the use of EnumTraits to list all the valid > values so they could be logical-or-ed together. > > 3. Change OptionSet() constructors (both for individual enum values and for > std::initializer_list) to verify that the value(s) passed in are valid enum > value(s), again after defining EnumTraits for the bit masks. > > Note that #2 and #3 might be a lot more work because most OptionSet() enums > don't have EnumTraits defined, but would probably catch more errors in the > long term. 4. Allow values with "all bits set" as valid for the OptionSet(T) constructor. That seems like the best option in the short term.
Created attachment 399923 [details] Patch v2
Comment on attachment 399923 [details] Patch v2 Ready for review.
Comment on attachment 399923 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=399923&action=review > Source/WTF/wtf/OptionSet.h:84 > + ASSERT_WITH_MESSAGE(!m_storage || hasOneBitSet(m_storage) || m_storage == std::numeric_limits<std::underlying_type_t<T>>::max(), "Enumerator is not a zero, a positive power of two or has all bits set."); I don't really like this. If we did this we should probably have a static_assert that std::underlying_type<T> is unsigned. > Source/WebCore/page/DragActions.h:34 > +enum class DragDestinationAction : uint32_t { This only needs to be 1 byte. Can't we use a uint8_t? > Source/WebCore/page/DragActions.h:35 > + None = 0, Do we still need this? The default constructor of OptionSet should take care of this case. > Source/WebCore/page/DragActions.h:39 > + Any = std::numeric_limits<uint32_t>::max() What if instead of this we made a function that returned an OptionSet<DragDestinationAction> called anyDragDestination()?
Comment on attachment 399923 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=399923&action=review >> Source/WTF/wtf/OptionSet.h:84 >> + ASSERT_WITH_MESSAGE(!m_storage || hasOneBitSet(m_storage) || m_storage == std::numeric_limits<std::underlying_type_t<T>>::max(), "Enumerator is not a zero, a positive power of two or has all bits set."); > > I don't really like this. If we did this we should probably have a static_assert that std::underlying_type<T> is unsigned. I can change the the words "all bits set" to something like "all maskable bits set". This is actually still correct for an enum with a signed underlying type if you were to create an "Every" or "All" or "Max" enum value. Only the sign bit isn't masked (set) in that case. >> Source/WebCore/page/DragActions.h:34 >> +enum class DragDestinationAction : uint32_t { > > This only needs to be 1 byte. Can't we use a uint8_t? Technically, this should be uint64_t (uint32_t on 32-bit) to match NSUInteger used for WKDragDestinationAction in WKDragDestinationAction.h: typedef NS_OPTIONS(NSUInteger, WKDragDestinationAction) { WKDragDestinationActionNone = 0, WKDragDestinationActionDHTML = 1, WKDragDestinationActionEdit = 2, WKDragDestinationActionLoad = 4, WKDragDestinationActionAny = NSUIntegerMax } WK_API_AVAILABLE(macos(10.13), ios(11.0)); I was taking the "kept in sync" comment in a more literal sense, especially since we freely cast from one to the other in the WebKit project. >> Source/WebCore/page/DragActions.h:35 >> + None = 0, > > Do we still need this? The default constructor of OptionSet should take care of this case. We do not strictly need this, but I thought we should keep it in sync with WKDragDestinationAction. How loose can we be with keeping them in sync? >> Source/WebCore/page/DragActions.h:39 >> + Any = std::numeric_limits<uint32_t>::max() > > What if instead of this we made a function that returned an OptionSet<DragDestinationAction> called anyDragDestination()? Again, I thought we should keep it in sync with WKDragDestinationAction, which also has an "Any" value.
Comment on attachment 399923 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=399923&action=review >>> Source/WebCore/page/DragActions.h:35 >>> + None = 0, >> >> Do we still need this? The default constructor of OptionSet should take care of this case. > > We do not strictly need this, but I thought we should keep it in sync with WKDragDestinationAction. > > How loose can we be with keeping them in sync? I don’t think we need to keep them in sync. We need to translate from one to the other, not try to make them the same. >>> Source/WebCore/page/DragActions.h:39 >>> + Any = std::numeric_limits<uint32_t>::max() >> >> What if instead of this we made a function that returned an OptionSet<DragDestinationAction> called anyDragDestination()? > > Again, I thought we should keep it in sync with WKDragDestinationAction, which also has an "Any" value. I agree with Alex, this should be a constexpr function. And it should have the value 0x7, not 0xFFFFFFFF. Something like this: constexpr OptionSet<DragDestinationAction> anyDragDestination { return DHTML | Edit | Load; } Critically, "any" and "none" must not be enumeration values, but should instead be possible OptionSet values.
Comment on attachment 399923 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=399923&action=review >>>> Source/WebCore/page/DragActions.h:35 >>>> + None = 0, >>> >>> Do we still need this? The default constructor of OptionSet should take care of this case. >> >> We do not strictly need this, but I thought we should keep it in sync with WKDragDestinationAction. >> >> How loose can we be with keeping them in sync? > > I don’t think we need to keep them in sync. We need to translate from one to the other, not try to make them the same. Okay. >>>> Source/WebCore/page/DragActions.h:39 >>>> + Any = std::numeric_limits<uint32_t>::max() >>> >>> What if instead of this we made a function that returned an OptionSet<DragDestinationAction> called anyDragDestination()? >> >> Again, I thought we should keep it in sync with WKDragDestinationAction, which also has an "Any" value. > > I agree with Alex, this should be a constexpr function. And it should have the value 0x7, not 0xFFFFFFFF. Something like this: > > constexpr OptionSet<DragDestinationAction> anyDragDestination { return DHTML | Edit | Load; } > > Critically, "any" and "none" must not be enumeration values, but should instead be possible OptionSet values. I made a template that would calculate the "any" value using EnumTraits in Bug 211988 Attachment #399615 [details]. Does that seem like it's worth doing here? <https://bugs.webkit.org/attachment.cgi?id=399615&action=review> (Alex didn't like the way I added the IsEnumBitfield "trait" to distinguish bitfield enums from other enums, though.) template<typename T, typename E> struct EnumBitfieldValueChecker; template<typename T, typename E, E e, E... es> struct EnumBitfieldValueChecker<T, EnumValues<E, e, es...>> { static constexpr T allValidBits() { return static_cast<T>(e) | EnumBitfieldValueChecker<T, EnumValues<E, es...>>::allValidBits(); } }; template<typename T, typename E> struct EnumBitfieldValueChecker<T, EnumValues<E>> { static constexpr T allValidBits() { return 0; } };
Comment on attachment 399923 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=399923&action=review >>>>> Source/WebCore/page/DragActions.h:39 >>>>> + Any = std::numeric_limits<uint32_t>::max() >>>> >>>> What if instead of this we made a function that returned an OptionSet<DragDestinationAction> called anyDragDestination()? >>> >>> Again, I thought we should keep it in sync with WKDragDestinationAction, which also has an "Any" value. >> >> I agree with Alex, this should be a constexpr function. And it should have the value 0x7, not 0xFFFFFFFF. Something like this: >> >> constexpr OptionSet<DragDestinationAction> anyDragDestination { return DHTML | Edit | Load; } >> >> Critically, "any" and "none" must not be enumeration values, but should instead be possible OptionSet values. > > I made a template that would calculate the "any" value using EnumTraits in Bug 211988 Attachment #399615 [details]. Does that seem like it's worth doing here? > > <https://bugs.webkit.org/attachment.cgi?id=399615&action=review> > > (Alex didn't like the way I added the IsEnumBitfield "trait" to distinguish bitfield enums from other enums, though.) > > template<typename T, typename E> struct EnumBitfieldValueChecker; > > template<typename T, typename E, E e, E... es> > struct EnumBitfieldValueChecker<T, EnumValues<E, e, es...>> { > static constexpr T allValidBits() > { > return static_cast<T>(e) | EnumBitfieldValueChecker<T, EnumValues<E, es...>>::allValidBits(); > } > }; > > template<typename T, typename E> > struct EnumBitfieldValueChecker<T, EnumValues<E>> { > static constexpr T allValidBits() > { > return 0; > } > }; Actually, I could make this an OptionSetTrait<> since it's similar but separate from EnumTrait<>.
Comment on attachment 399923 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=399923&action=review >>>>>> Source/WebCore/page/DragActions.h:39 >>>>>> + Any = std::numeric_limits<uint32_t>::max() >>>>> >>>>> What if instead of this we made a function that returned an OptionSet<DragDestinationAction> called anyDragDestination()? >>>> >>>> Again, I thought we should keep it in sync with WKDragDestinationAction, which also has an "Any" value. >>> >>> I agree with Alex, this should be a constexpr function. And it should have the value 0x7, not 0xFFFFFFFF. Something like this: >>> >>> constexpr OptionSet<DragDestinationAction> anyDragDestination { return DHTML | Edit | Load; } >>> >>> Critically, "any" and "none" must not be enumeration values, but should instead be possible OptionSet values. >> >> I made a template that would calculate the "any" value using EnumTraits in Bug 211988 Attachment #399615 [details]. Does that seem like it's worth doing here? >> >> <https://bugs.webkit.org/attachment.cgi?id=399615&action=review> >> >> (Alex didn't like the way I added the IsEnumBitfield "trait" to distinguish bitfield enums from other enums, though.) >> >> template<typename T, typename E> struct EnumBitfieldValueChecker; >> >> template<typename T, typename E, E e, E... es> >> struct EnumBitfieldValueChecker<T, EnumValues<E, e, es...>> { >> static constexpr T allValidBits() >> { >> return static_cast<T>(e) | EnumBitfieldValueChecker<T, EnumValues<E, es...>>::allValidBits(); >> } >> }; >> >> template<typename T, typename E> >> struct EnumBitfieldValueChecker<T, EnumValues<E>> { >> static constexpr T allValidBits() >> { >> return 0; >> } >> }; > > Actually, I could make this an OptionSetTrait<> since it's similar but separate from EnumTrait<>. I suppose it’s elegant to not have to list all the bits all in a function and keep it updated. On the other hand, in my opinion there’s not necessarily a valuable meaning, for every OptionSet, of setting all the bits.
Created attachment 400097 [details] Patch v3
Comment on attachment 400097 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=400097&action=review > Source/WTF/ChangeLog:22 > + fall back to the original validation checks. (The !m_storage > + check in the OptionSet<> constructor was removed to re-enforce > + that ::None enums don't need to be defined when used with > + OptionSet<>.) Sigh. Looks like PaintBehavior::None is causing problems after the !m_storage check was removed. Probably just need to add that back for now.
Comment on attachment 400097 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=400097&action=review >> Source/WTF/ChangeLog:22 >> + OptionSet<>.) > > Sigh. Looks like PaintBehavior::None is causing problems after the !m_storage check was removed. Probably just need to add that back for now. Oops, that should be PaintBehavior::Normal.
Created attachment 400121 [details] Patch v4
(In reply to David Kilzer (:ddkilzer) from comment #16) > Created attachment 400121 [details] > Patch v4 Compared to Patch v3, this adds back the `!m_storage` check to the OptionSet constructor due to use of PaintBehavior::Normal: constexpr OptionSet(E e) : m_storage(static_cast<StorageType>(e)) { - ASSERT(isValidOptionSetEnum(e)); + ASSERT(!m_storage || isValidOptionSetEnum(e)); }
Comment on attachment 399923 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=399923&action=review >>> Source/WTF/wtf/OptionSet.h:84 >>> + ASSERT_WITH_MESSAGE(!m_storage || hasOneBitSet(m_storage) || m_storage == std::numeric_limits<std::underlying_type_t<T>>::max(), "Enumerator is not a zero, a positive power of two or has all bits set."); >> >> I don't really like this. If we did this we should probably have a static_assert that std::underlying_type<T> is unsigned. > > I can change the the words "all bits set" to something like "all maskable bits set". This is actually still correct for an enum with a signed underlying type if you were to create an "Every" or "All" or "Max" enum value. Only the sign bit isn't masked (set) in that case. In Patches v3 & v4, I removed this and instead added support for OptionSetTraits<>, which validates individual enum values using OptionSetTraits<> if it exists for that enum, otherwise it falls back to the assertion above. >>>>> Source/WebCore/page/DragActions.h:35 >>>>> + None = 0, >>>> >>>> Do we still need this? The default constructor of OptionSet should take care of this case. >>> >>> We do not strictly need this, but I thought we should keep it in sync with WKDragDestinationAction. >>> >>> How loose can we be with keeping them in sync? >> >> I don’t think we need to keep them in sync. We need to translate from one to the other, not try to make them the same. > > Okay. I found that translating from WebKitLegacy/WebKit types back to WebCore types meant that more bits were set than necessary since the *Any value in WebKitLegacy/WebKit/AppKit typically use the "set all bits" idiom. To address this, I apply the OptionSet<>::any() bit mask (made possible to compute with OptionSetTraits<>) when constructing an OptionSet<> using the ::fromRaw() method to make sure each OptionSet<> with OptionSetTraits<> only has valid bits set at any time. See WTF::maskRawValue() in Patches v3 & v4. (Initially I tried to validate the WebKitLegacy/WebKit/AppKit values, but this frequently failed due to the "set all bits" idiom for *Any values noted above.) >>>>>>> Source/WebCore/page/DragActions.h:39 >>>>>>> + Any = std::numeric_limits<uint32_t>::max() >>>>>> >>>>>> What if instead of this we made a function that returned an OptionSet<DragDestinationAction> called anyDragDestination()? >>>>> >>>>> Again, I thought we should keep it in sync with WKDragDestinationAction, which also has an "Any" value. >>>> >>>> I agree with Alex, this should be a constexpr function. And it should have the value 0x7, not 0xFFFFFFFF. Something like this: >>>> >>>> constexpr OptionSet<DragDestinationAction> anyDragDestination { return DHTML | Edit | Load; } >>>> >>>> Critically, "any" and "none" must not be enumeration values, but should instead be possible OptionSet values. >>> >>> I made a template that would calculate the "any" value using EnumTraits in Bug 211988 Attachment #399615 [details]. Does that seem like it's worth doing here? >>> >>> <https://bugs.webkit.org/attachment.cgi?id=399615&action=review> >>> >>> (Alex didn't like the way I added the IsEnumBitfield "trait" to distinguish bitfield enums from other enums, though.) >>> >>> template<typename T, typename E> struct EnumBitfieldValueChecker; >>> >>> template<typename T, typename E, E e, E... es> >>> struct EnumBitfieldValueChecker<T, EnumValues<E, e, es...>> { >>> static constexpr T allValidBits() >>> { >>> return static_cast<T>(e) | EnumBitfieldValueChecker<T, EnumValues<E, es...>>::allValidBits(); >>> } >>> }; >>> >>> template<typename T, typename E> >>> struct EnumBitfieldValueChecker<T, EnumValues<E>> { >>> static constexpr T allValidBits() >>> { >>> return 0; >>> } >>> }; >> >> Actually, I could make this an OptionSetTrait<> since it's similar but separate from EnumTrait<>. > > I suppose it’s elegant to not have to list all the bits all in a function and keep it updated. > > On the other hand, in my opinion there’s not necessarily a valuable meaning, for every OptionSet, of setting all the bits. I made it possible to use an optional OptionSetTraits<> for an OptionSet<> in Patches v3 & v4. This makes it possible to generate an OptionSet<>::any() bit mask with all valid bits set, and to validate individual enum values if it exists.
Patch v4 is ready for review.
Comment on attachment 400121 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=400121&action=review I think this adds too much complexity to OptionSet. Let's push this complexity towards DragDestinationAction. I think OptionSetValues should be added in a different patch. Right now OptionSet serialization is basically unchecked and that's not a known issue because if a compromised process sends invalid bits, they won't be used. > Source/WTF/wtf/OptionSet.h:244 > +constexpr OptionSet<E> OptionSet<E>::any() I don't think this is a good name for this. Just because DragDestinationAction had an any doesn't mean other enums call it "any".
Comment on attachment 400121 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=400121&action=review >> Source/WTF/wtf/OptionSet.h:244 >> +constexpr OptionSet<E> OptionSet<E>::any() > > I don't think this is a good name for this. Just because DragDestinationAction had an any doesn't mean other enums call it "any". I don't have a strong feeling about using ::any(). What would you suggest instead? ::all()?
Comment on attachment 400121 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=400121&action=review >>> Source/WTF/wtf/OptionSet.h:244 >>> +constexpr OptionSet<E> OptionSet<E>::any() >> >> I don't think this is a good name for this. Just because DragDestinationAction had an any doesn't mean other enums call it "any". > > I don't have a strong feeling about using ::any(). What would you suggest instead? ::all()? I would suggest making this just a function that returns an OptionSet<DragDestinationAction>, not generalizing it for OptionSet<E>.
(In reply to Alex Christensen from comment #20) > Comment on attachment 400121 [details] > Patch v4 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400121&action=review > > I think this adds too much complexity to OptionSet. Let's push this > complexity towards DragDestinationAction. I guess you mean this? "I would suggest making this just a function that returns an OptionSet<DragDestinationAction>, not generalizing it for OptionSet<E>." (I got so distracted between my last comment and replying to this that you answered in the meantime. :) Sounds good. We can generalize ::all()/::any() if other OptionSet<> values need this later. > I think OptionSetValues should be added in a different patch. Right now > OptionSet serialization is basically unchecked and that's not a known issue > because if a compromised process sends invalid bits, they won't be used. Okay, I will break this up into a separate patch. Thanks!
Created attachment 400294 [details] Patch v5
(In reply to David Kilzer (:ddkilzer) from comment #24) > Created attachment 400294 [details] > Patch v5 - Removed OptionSet.h changes. - Removed OptionSetTraits<WebCore::DragDestinationAction>. - Replaced OptionSetTraits<WebCore::DragDestinationAction>::any() with DragDestinationActionAny() function.
Committed r262190: <https://trac.webkit.org/changeset/262190> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400294 [details].
Comment on attachment 400294 [details] Patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=400294&action=review > Source/WebCore/page/DragActions.h:42 > +inline constexpr OptionSet<DragDestinationAction> DragDestinationActionAny() No need for inline if we also have constexpr, which means inline. Should not name this with a leading capital letter. I suggest the name anyDragDestinationAction(). > Source/WebCore/platform/DragData.h:79 > + WEBCORE_EXPORT DragData(DragDataRef, const IntPoint& clientPosition, const IntPoint& globalPosition, DragOperation, DragApplicationFlags = DragApplicationNone, OptionSet<DragDestinationAction> actionMask = DragDestinationActionAny()); > + WEBCORE_EXPORT DragData(const String& dragStorageName, const IntPoint& clientPosition, const IntPoint& globalPosition, DragOperation, DragApplicationFlags = DragApplicationNone, OptionSet<DragDestinationAction> actionMask = DragDestinationActionAny()); I suggest omitting the argument name "actionMask" here. > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:3948 > + auto dragDestinationActionMask = OptionSet<WebCore::DragDestinationAction>::fromRaw([m_view _web_dragDestinationActionForDraggingInfo:draggingInfo]); This conversion should *not* be done with fromRaw. We should write a function to do the conversion. We don’t want code that depends on WKDragDestinationAction being identical to OptionSet<WebCore::DragDestinationAction>. Instead we want to write a simple translation function. > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:3949 > + WebCore::DragData dragData(draggingInfo, client, global, static_cast<WebCore::DragOperation>(draggingInfo.draggingSourceOperationMask), applicationFlagsForDrag(m_view.getAutoreleased(), draggingInfo), dragDestinationActionMask); Same here with the static_cast. > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:3961 > + auto dragDestinationActionMask = OptionSet<WebCore::DragDestinationAction>::fromRaw([m_view _web_dragDestinationActionForDraggingInfo:draggingInfo]); Ditto. > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:3962 > + WebCore::DragData dragData(draggingInfo, client, global, static_cast<WebCore::DragOperation>(draggingInfo.draggingSourceOperationMask), applicationFlagsForDrag(m_view.getAutoreleased(), draggingInfo), dragDestinationActionMask); Ditto. > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7314 > + auto dragDestinationActionMask = OptionSet<WebCore::DragDestinationAction>::fromRaw(dragDestinationAction); Ditto. > Source/WebKitLegacy/mac/WebCoreSupport/WebDragClient.mm:89 > + [[m_webView _UIDelegateForwarder] webView:m_webView willPerformDragDestinationAction:static_cast<WebDragDestinationAction>(action) forDraggingInfo:dragData.platformData()]; Same thing again. We should use a function to convert, not rely on the internal mask matching the API one. We’d like to be able to add things internally without being in lock step with public API. > Source/WebKitLegacy/mac/WebView/WebView.mm:1915 > + auto dragDestinationMask = OptionSet<WebCore::DragDestinationAction>::fromRaw([self dragDestinationActionMaskForSession:session]); Ditto. > Source/WebKitLegacy/mac/WebView/WebView.mm:6729 > + return OptionSet<WebCore::DragDestinationAction>::fromRaw([[self _UIDelegateForwarder] webView:self dragDestinationActionMaskForDraggingInfo:draggingInfo]); Ditto.
(In reply to Darin Adler from comment #27) > Comment on attachment 400294 [details] > Patch v5 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400294&action=review > > > Source/WebCore/page/DragActions.h:42 > > +inline constexpr OptionSet<DragDestinationAction> DragDestinationActionAny() > > No need for inline if we also have constexpr, which means inline. > > Should not name this with a leading capital letter. I suggest the name > anyDragDestinationAction(). > > > Source/WebCore/platform/DragData.h:79 > > + WEBCORE_EXPORT DragData(DragDataRef, const IntPoint& clientPosition, const IntPoint& globalPosition, DragOperation, DragApplicationFlags = DragApplicationNone, OptionSet<DragDestinationAction> actionMask = DragDestinationActionAny()); > > + WEBCORE_EXPORT DragData(const String& dragStorageName, const IntPoint& clientPosition, const IntPoint& globalPosition, DragOperation, DragApplicationFlags = DragApplicationNone, OptionSet<DragDestinationAction> actionMask = DragDestinationActionAny()); > > I suggest omitting the argument name "actionMask" here. > > > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:3948 > > + auto dragDestinationActionMask = OptionSet<WebCore::DragDestinationAction>::fromRaw([m_view _web_dragDestinationActionForDraggingInfo:draggingInfo]); > > This conversion should *not* be done with fromRaw. We should write a > function to do the conversion. We don’t want code that depends on > WKDragDestinationAction being identical to > OptionSet<WebCore::DragDestinationAction>. Instead we want to write a simple > translation function. > > > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:3949 > > + WebCore::DragData dragData(draggingInfo, client, global, static_cast<WebCore::DragOperation>(draggingInfo.draggingSourceOperationMask), applicationFlagsForDrag(m_view.getAutoreleased(), draggingInfo), dragDestinationActionMask); > > Same here with the static_cast. > > > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:3961 > > + auto dragDestinationActionMask = OptionSet<WebCore::DragDestinationAction>::fromRaw([m_view _web_dragDestinationActionForDraggingInfo:draggingInfo]); > > Ditto. > > > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:3962 > > + WebCore::DragData dragData(draggingInfo, client, global, static_cast<WebCore::DragOperation>(draggingInfo.draggingSourceOperationMask), applicationFlagsForDrag(m_view.getAutoreleased(), draggingInfo), dragDestinationActionMask); > > Ditto. > > > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7314 > > + auto dragDestinationActionMask = OptionSet<WebCore::DragDestinationAction>::fromRaw(dragDestinationAction); > > Ditto. > > > Source/WebKitLegacy/mac/WebCoreSupport/WebDragClient.mm:89 > > + [[m_webView _UIDelegateForwarder] webView:m_webView willPerformDragDestinationAction:static_cast<WebDragDestinationAction>(action) forDraggingInfo:dragData.platformData()]; > > Same thing again. We should use a function to convert, not rely on the > internal mask matching the API one. We’d like to be able to add things > internally without being in lock step with public API. > > > Source/WebKitLegacy/mac/WebView/WebView.mm:1915 > > + auto dragDestinationMask = OptionSet<WebCore::DragDestinationAction>::fromRaw([self dragDestinationActionMaskForSession:session]); > > Ditto. > > > Source/WebKitLegacy/mac/WebView/WebView.mm:6729 > > + return OptionSet<WebCore::DragDestinationAction>::fromRaw([[self _UIDelegateForwarder] webView:self dragDestinationActionMaskForDraggingInfo:draggingInfo]); > > Ditto. Bug 212507: Don't use casts to convert between WebCore types and {Web,WK}DragDestinationActionMask/{NS,WK}DragOperation