MarkerSubrange.SubdivideGrammarAndSelectionOverlap{Frontmost, FrontmostWithLongestEff...
authordbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Dec 2017 19:59:35 +0000 (19:59 +0000)
committerdbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Dec 2017 19:59:35 +0000 (19:59 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181014

Reviewed by Simon Fraser.

Source/WebCore:

Fixes an issue in the subdivision algorithm where the returned subranges may not be paint order
or reverse paint order when using the default overlap strategy (OverlapStrategy::None) and
either OverlapStrategy::Frontmost or OverlapStrategy::FrontmostWithLongestEffectiveRange, respectively.

Currently we compute the overlapping subranges up to some point p_i on the line by sweeping from the
start of the line through all the unclosed subranges. The unclosed subranges are sorted along the line.
That is, they are not sorted by paint order or reverse paint order. Therefore we must take care to
ensure that we return the computed overlapping subranges with respect to paint order/reverse paint order.

* rendering/MarkerSubrange.cpp:
(WebCore::subdivide):

Tools:

Adds a new test to ensure we compute overlapping subranges in paint order for the default
overlap strategy. Enable tests MarkerSubrange.SubdivideGrammarAndSelectionOverlap{Frontmost, FrontmostWithLongestEffectiveRange}
now that they pass.

* TestWebKitAPI/Tests/WebCore/MarkerSubrange.cpp:
(TestWebKitAPI::TEST):

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

Source/WebCore/ChangeLog
Source/WebCore/rendering/MarkerSubrange.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebCore/MarkerSubrange.cpp

index f16b75c..0dc995b 100644 (file)
@@ -1,3 +1,22 @@
+2017-12-20  Daniel Bates  <dabates@apple.com>
+
+        MarkerSubrange.SubdivideGrammarAndSelectionOverlap{Frontmost, FrontmostWithLongestEffectiveRange} are failing
+        https://bugs.webkit.org/show_bug.cgi?id=181014
+
+        Reviewed by Simon Fraser.
+
+        Fixes an issue in the subdivision algorithm where the returned subranges may not be paint order
+        or reverse paint order when using the default overlap strategy (OverlapStrategy::None) and
+        either OverlapStrategy::Frontmost or OverlapStrategy::FrontmostWithLongestEffectiveRange, respectively.
+
+        Currently we compute the overlapping subranges up to some point p_i on the line by sweeping from the
+        start of the line through all the unclosed subranges. The unclosed subranges are sorted along the line.
+        That is, they are not sorted by paint order or reverse paint order. Therefore we must take care to
+        ensure that we return the computed overlapping subranges with respect to paint order/reverse paint order.
+
+        * rendering/MarkerSubrange.cpp:
+        (WebCore::subdivide):
+
 2017-12-20  Youenn Fablet  <youenn@apple.com>
 
         LayoutTest imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-match.https.html is a flaky failure
index ccc632d..d5781eb 100644 (file)
@@ -70,7 +70,7 @@ Vector<MarkerSubrange> subdivide(const Vector<MarkerSubrange>& subranges, Overla
             if (overlapStrategy == OverlapStrategy::Frontmost || overlapStrategy == OverlapStrategy::FrontmostWithLongestEffectiveRange) {
                 std::optional<unsigned> frontmost;
                 for (unsigned j = 0; j < i; ++j) {
-                    if (!processedSubranges.contains(offsets[j].subrange))
+                    if (!processedSubranges.contains(offsets[j].subrange) && (!frontmost || offsets[j].subrange->type > offsets[*frontmost].subrange->type))
                         frontmost = j;
                 }
                 if (frontmost) {
@@ -84,6 +84,7 @@ Vector<MarkerSubrange> subdivide(const Vector<MarkerSubrange>& subranges, Overla
                         result.append({ offsetSoFar, offsets[i].value, offsets[frontmost.value()].subrange->type, offsets[frontmost.value()].subrange->marker });
                 }
             } else {
+                // The appended subranges may not be in paint order. We will fix this up at the end of this function.
                 for (unsigned j = 0; j < i; ++j) {
                     if (!processedSubranges.contains(offsets[j].subrange))
                         result.append({ offsetSoFar, offsets[i].value, offsets[j].subrange->type, offsets[j].subrange->marker });
@@ -94,6 +95,9 @@ Vector<MarkerSubrange> subdivide(const Vector<MarkerSubrange>& subranges, Overla
         if (offsets[i].kind == Offset::End)
             processedSubranges.add(offsets[i].subrange);
     }
+    // Fix up; sort the subranges so that they are in paint order.
+    if (overlapStrategy == OverlapStrategy::None)
+        std::sort(result.begin(), result.end(), [] (const MarkerSubrange& a, const MarkerSubrange& b) { return a.startOffset < b.startOffset || (a.startOffset == b.startOffset && a.type < b.type); });
     return result;
 }
 
index cae89be..692f55e 100644 (file)
@@ -1,5 +1,19 @@
 2017-12-20  Daniel Bates  <dabates@apple.com>
 
+        MarkerSubrange.SubdivideGrammarAndSelectionOverlap{Frontmost, FrontmostWithLongestEffectiveRange} are failing
+        https://bugs.webkit.org/show_bug.cgi?id=181014
+
+        Reviewed by Simon Fraser.
+
+        Adds a new test to ensure we compute overlapping subranges in paint order for the default
+        overlap strategy. Enable tests MarkerSubrange.SubdivideGrammarAndSelectionOverlap{Frontmost, FrontmostWithLongestEffectiveRange}
+        now that they pass.
+
+        * TestWebKitAPI/Tests/WebCore/MarkerSubrange.cpp:
+        (TestWebKitAPI::TEST):
+
+2017-12-20  Daniel Bates  <dabates@apple.com>
+
         Remove Alternative Presentation Button
         https://bugs.webkit.org/show_bug.cgi?id=180500
         <rdar://problem/35891047>
index d80e1f5..295f437 100644 (file)
@@ -174,7 +174,28 @@ TEST(MarkerSubrange, SubdivideSpellingAndGrammarComplicatedFrontmost)
         EXPECT_EQ(expectedSubranges[i], results[i]);
 }
 
-TEST(MarkerSubrange, DISABLED_SubdivideGrammarAndSelectionOverlapFrontmost)
+TEST(MarkerSubrange, SubdivideGrammarAndSelectionOverlap)
+{
+    Vector<MarkerSubrange> subranges {
+        MarkerSubrange { 0, 40, MarkerSubrange::GrammarError },
+        MarkerSubrange { 2, 60, MarkerSubrange::Selection },
+        MarkerSubrange { 50, 60, MarkerSubrange::GrammarError },
+    };
+    Vector<MarkerSubrange> expectedSubranges {
+        MarkerSubrange { 0, 2, MarkerSubrange::GrammarError },
+        MarkerSubrange { 2, 40, MarkerSubrange::GrammarError },
+        MarkerSubrange { 2, 40, MarkerSubrange::Selection },
+        MarkerSubrange { 40, 50, MarkerSubrange::Selection },
+        MarkerSubrange { 50, 60, MarkerSubrange::GrammarError },
+        MarkerSubrange { 50, 60, MarkerSubrange::Selection },
+    };
+    auto results = subdivide(subranges);
+    ASSERT_EQ(expectedSubranges.size(), results.size());
+    for (size_t i = 0; i < expectedSubranges.size(); ++i)
+        EXPECT_EQ(expectedSubranges[i], results[i]);
+}
+
+TEST(MarkerSubrange, SubdivideGrammarAndSelectionOverlapFrontmost)
 {
     Vector<MarkerSubrange> subranges {
         MarkerSubrange { 0, 40, MarkerSubrange::GrammarError },
@@ -193,7 +214,7 @@ TEST(MarkerSubrange, DISABLED_SubdivideGrammarAndSelectionOverlapFrontmost)
         EXPECT_EQ(expectedSubranges[i], results[i]);
 }
 
-TEST(MarkerSubrange, DISABLED_SubdivideGrammarAndSelectionOverlapFrontmostWithLongestEffectiveRange)
+TEST(MarkerSubrange, SubdivideGrammarAndSelectionOverlapFrontmostWithLongestEffectiveRange)
 {
     Vector<MarkerSubrange> subranges {
         MarkerSubrange { 0, 40, MarkerSubrange::GrammarError },