Meaning of OptionSet::contains is unclear when used with OptionSet argument
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 13 Aug 2018 19:16:10 +0000 (19:16 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 13 Aug 2018 19:16:10 +0000 (19:16 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188501

Reviewed by Anders Carlsson.

Source/WebCore:

* dom/DocumentMarkerController.cpp:
(WebCore::DocumentMarkerController::possiblyHasMarkers):
* dom/DocumentMarkerController.h:
(WebCore::DocumentMarkerController::hasMarkers const):
* platform/FileSystem.h:
(WebCore::FileSystem::openAndLockFile):
* rendering/RenderElement.cpp:
(WebCore::RenderElement::selectionColor const):
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::paintForegroundForFragments):

Source/WTF:

The existing behavior is "contains any" but it is not very clear from the name.

* wtf/OptionSet.h:
(WTF::OptionSet::contains const):

This is now for testing a single option only.

(WTF::OptionSet::containsAny const):
(WTF::OptionSet::containsAll const):

Add separate functions for OptionSet argument.

Tools:

* TestWebKitAPI/Tests/WTF/OptionSet.cpp:
(TestWebKitAPI::TEST):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@234811 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/OptionSet.h
Source/WebCore/ChangeLog
Source/WebCore/dom/DocumentMarkerController.cpp
Source/WebCore/dom/DocumentMarkerController.h
Source/WebCore/rendering/RenderElement.cpp
Source/WebCore/rendering/RenderLayer.cpp
Source/WebKit/WebProcess/Plugins/PluginView.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WTF/OptionSet.cpp

index e255901..8098151 100644 (file)
@@ -1,3 +1,22 @@
+2018-08-13  Antti Koivisto  <antti@apple.com>
+
+        Meaning of OptionSet::contains is unclear when used with OptionSet argument
+        https://bugs.webkit.org/show_bug.cgi?id=188501
+
+        Reviewed by Anders Carlsson.
+
+        The existing behavior is "contains any" but it is not very clear from the name.
+
+        * wtf/OptionSet.h:
+        (WTF::OptionSet::contains const):
+
+        This is now for testing a single option only.
+
+        (WTF::OptionSet::containsAny const):
+        (WTF::OptionSet::containsAll const):
+
+        Add separate functions for OptionSet argument.
+
 2018-08-13  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r234747.
index 3f725c1..4042a49 100644 (file)
@@ -96,9 +96,19 @@ public:
 
     constexpr explicit operator bool() { return !isEmpty(); }
 
-    constexpr bool contains(OptionSet optionSet) const
+    constexpr bool contains(T option) const
     {
-        return m_storage & optionSet.m_storage;
+        return containsAny({ option });
+    }
+
+    constexpr bool containsAny(OptionSet optionSet) const
+    {
+        return !!(*this & optionSet);
+    }
+
+    constexpr bool containsAll(OptionSet optionSet) const
+    {
+        return (*this & optionSet) == optionSet;
     }
 
     constexpr friend bool operator==(OptionSet lhs, OptionSet rhs)
index d26a478..7cd7853 100644 (file)
@@ -1,3 +1,21 @@
+2018-08-13  Antti Koivisto  <antti@apple.com>
+
+        Meaning of OptionSet::contains is unclear when used with OptionSet argument
+        https://bugs.webkit.org/show_bug.cgi?id=188501
+
+        Reviewed by Anders Carlsson.
+
+        * dom/DocumentMarkerController.cpp:
+        (WebCore::DocumentMarkerController::possiblyHasMarkers):
+        * dom/DocumentMarkerController.h:
+        (WebCore::DocumentMarkerController::hasMarkers const):
+        * platform/FileSystem.h:
+        (WebCore::FileSystem::openAndLockFile):
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::selectionColor const):
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::paintForegroundForFragments):
+
 2018-08-13  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r234747.
index a02ccbc..0d05b37 100644 (file)
@@ -44,7 +44,7 @@ namespace WebCore {
 
 inline bool DocumentMarkerController::possiblyHasMarkers(OptionSet<DocumentMarker::MarkerType> types)
 {
-    return m_possiblyExistingMarkerTypes.contains(types);
+    return m_possiblyExistingMarkerTypes.containsAny(types);
 }
 
 DocumentMarkerController::DocumentMarkerController(Document& document)
index 9007a18..88b9fce 100644 (file)
@@ -63,7 +63,7 @@ public:
     void copyMarkers(Node* srcNode, unsigned startOffset, int length, Node* dstNode, int delta);
     bool hasMarkers() const
     {
-        ASSERT(m_markers.isEmpty() == !m_possiblyExistingMarkerTypes.contains(DocumentMarker::allMarkers()));
+        ASSERT(m_markers.isEmpty() == !m_possiblyExistingMarkerTypes.containsAny(DocumentMarker::allMarkers()));
         return !m_markers.isEmpty();
     }
     bool hasMarkers(Range&, OptionSet<DocumentMarker::MarkerType> = DocumentMarker::allMarkers());
index d14a3c0..c692b7c 100644 (file)
@@ -1361,7 +1361,7 @@ Color RenderElement::selectionColor(CSSPropertyID colorProperty) const
     // If the element is unselectable, or we are only painting the selection,
     // don't override the foreground color with the selection foreground color.
     if (style().userSelect() == UserSelect::None
-        || (view().frameView().paintBehavior().contains(OptionSet<PaintBehavior>(PaintBehavior::SelectionOnly) | PaintBehavior::SelectionAndBackgroundsOnly)))
+        || (view().frameView().paintBehavior().containsAny({ PaintBehavior::SelectionOnly, PaintBehavior::SelectionAndBackgroundsOnly })))
         return Color();
 
     if (std::unique_ptr<RenderStyle> pseudoStyle = selectionPseudoStyle()) {
index 811bea9..322028d 100644 (file)
@@ -4788,7 +4788,7 @@ void RenderLayer::paintForegroundForFragments(const LayerFragments& layerFragmen
     
     // We have to loop through every fragment multiple times, since we have to repaint in each specific phase in order for
     // interleaving of the fragments to work properly.
-    bool selectionOnly = localPaintingInfo.paintBehavior.contains(OptionSet<PaintBehavior>(PaintBehavior::SelectionAndBackgroundsOnly) | PaintBehavior::SelectionOnly);
+    bool selectionOnly = localPaintingInfo.paintBehavior.containsAny({ PaintBehavior::SelectionAndBackgroundsOnly, PaintBehavior::SelectionOnly });
     paintForegroundForFragmentsWithPhase(selectionOnly ? PaintPhase::Selection : PaintPhase::ChildBlockBackgrounds, layerFragments,
         context, localPaintingInfo, localPaintBehavior, subtreePaintRootForRenderer);
     
index c7fddcc..c368ffe 100644 (file)
@@ -1847,7 +1847,7 @@ bool PluginView::shouldCreateTransientPaintingSnapshot() const
         return false;
 
     if (FrameView* frameView = frame()->view()) {
-        if (frameView->paintBehavior().contains(OptionSet<PaintBehavior>(PaintBehavior::SelectionOnly) | PaintBehavior::SelectionAndBackgroundsOnly | PaintBehavior::ForceBlackText)) {
+        if (frameView->paintBehavior().containsAny({ PaintBehavior::SelectionOnly, PaintBehavior::SelectionAndBackgroundsOnly, PaintBehavior::ForceBlackText})) {
             // This paint behavior is used when drawing the find indicator and there's no need to
             // snapshot plug-ins, because they can never be painted as part of the find indicator.
             return false;
index a346f7e..1c92216 100644 (file)
@@ -1,3 +1,13 @@
+2018-08-13  Antti Koivisto  <antti@apple.com>
+
+        Meaning of OptionSet::contains is unclear when used with OptionSet argument
+        https://bugs.webkit.org/show_bug.cgi?id=188501
+
+        Reviewed by Anders Carlsson.
+
+        * TestWebKitAPI/Tests/WTF/OptionSet.cpp:
+        (TestWebKitAPI::TEST):
+
 2018-08-13  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r234747.
index 13e03de..cf42fdd 100644 (file)
@@ -379,4 +379,32 @@ TEST(WTF_OptionSet, OperatorAnd)
     }
 }
 
+TEST(WTF_OptionSet, ContainsAny)
+{
+    OptionSet<ExampleFlags> set { ExampleFlags::A, ExampleFlags::B };
+
+    EXPECT_TRUE(set.containsAny({ ExampleFlags::A }));
+    EXPECT_TRUE(set.containsAny({ ExampleFlags::B }));
+    EXPECT_FALSE(set.containsAny({ ExampleFlags::C }));
+    EXPECT_FALSE(set.containsAny({ ExampleFlags::C, ExampleFlags::D }));
+    EXPECT_TRUE(set.containsAny({ ExampleFlags::A, ExampleFlags::B }));
+    EXPECT_TRUE(set.containsAny({ ExampleFlags::B, ExampleFlags::C }));
+    EXPECT_TRUE(set.containsAny({ ExampleFlags::A, ExampleFlags::C }));
+    EXPECT_TRUE(set.containsAny({ ExampleFlags::A, ExampleFlags::B, ExampleFlags::C }));
+}
+
+TEST(WTF_OptionSet, ContainsAll)
+{
+    OptionSet<ExampleFlags> set { ExampleFlags::A, ExampleFlags::B };
+
+    EXPECT_TRUE(set.containsAll({ ExampleFlags::A }));
+    EXPECT_TRUE(set.containsAll({ ExampleFlags::B }));
+    EXPECT_FALSE(set.containsAll({ ExampleFlags::C }));
+    EXPECT_FALSE(set.containsAll({ ExampleFlags::C, ExampleFlags::D }));
+    EXPECT_TRUE(set.containsAll({ ExampleFlags::A, ExampleFlags::B }));
+    EXPECT_FALSE(set.containsAll({ ExampleFlags::B, ExampleFlags::C }));
+    EXPECT_FALSE(set.containsAll({ ExampleFlags::A, ExampleFlags::C }));
+    EXPECT_FALSE(set.containsAll({ ExampleFlags::A, ExampleFlags::B, ExampleFlags::C }));
+}
+
 } // namespace TestWebKitAPI