Move TextIterator::rangeFromLocationAndLength off of live ranges

• Component: HTML Editing
• Status: RESOLVED FIXED

[Darin Adler]

Move TextIterator::rangeFromLocationAndLength off of live ranges

[Darin Adler]

Created attachment 394236 [details]
Patch for EWS (includes all of the patch from bug 209207 too)

[Darin Adler]

Created attachment 394316 [details]
Patch

[Darin Adler]

Created attachment 394757 [details]
Patch

[Darin Adler]

Created attachment 394767 [details]
Patch

[Darin Adler]

Created attachment 394782 [details]
Patch

[Darin Adler]

Created attachment 394821 [details]
Patch

[Darin Adler]

Created attachment 394846 [details]
Patch

[Darin Adler]

Created attachment 394848 [details]
Patch

[Darin Adler]

Created attachment 394851 [details]
Patch

[Darin Adler]

Created attachment 394853 [details]
Patch

[Darin Adler]

Created attachment 394859 [details]
Patch

[Darin Adler]

Created attachment 394860 [details]
Patch

[Darin Adler]

OK, should be ready for review now.

[Darin Adler]

This patch also resolves bug 205314; should probably mention that in the change log.

[Darin Adler]

Windows bot failure isn’t real. Just conflict with a part of my latch with Dave Kilzer’s identical fix for a compile error on newer macOS that prevented the bot from applying the patch. I could upload a new patch to address it but I don’t think I should.

[Darin Adler]

Created attachment 394869 [details]
Patch

[Antti Koivisto]

Comment on attachment 394869 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394869&action=review

> Source/WebCore/editing/CharacterRange.h:40
> +using CharacterCount = std::size_t;

Why is this alias helpful? Why not just use size_t?

(I know it is not new in this patch)

> Source/WebCore/editing/CharacterRange.h:44
> +    CharacterCount location { 0 };
> +    CharacterCount length { 0 };

It is bit weird that a location type is called "count".

> Source/WebCore/editing/CharacterRange.h:79
> +inline Optional<CharacterRange> makeCharacterRange(CFRange range)
> +{
> +    ASSERT(range.location == kCFNotFound || range.location >= 0);
> +    ASSERT(range.length >= 0);
> +    if (range.location < 0 || range.length < 0)
> +        return WTF::nullopt;
> +    return CharacterRange { static_cast<CharacterCount>(range.location), static_cast<CharacterCount>(range.length) };
> +}

Most clients are just unwrapping the Optional without any checking. It might be better to add a separate function for the few cases where kCFNotFound is actually a valid input (makeCharacterRangeIfValid or something) and make the regular version assert and return a plain CharacterRange.

> Source/WebCore/editing/TextCheckingHelper.cpp:67
> +        badGrammar.range.location = checkLocation + badGrammarLocation;
> +        badGrammar.range.length = badGrammarLength;

badGrammar.range = { checkLocation + badGrammarLocation, badGrammarLength };

?

> Source/WebCore/editing/TextCheckingHelper.cpp:101
> +            misspelling.range.location = wordStart + misspellingLocation;
> +            misspelling.range.length = misspellingLength;

Similarly

> Source/WebCore/editing/TextCheckingHelper.cpp:279
> +            CharacterRange mispellingCharacterRange;
> +            mispellingCharacterRange.location = currentChunkOffset + misspellingLocation;
> +            mispellingCharacterRange.length = misspellingLength;

auto mispellingCharacterRange = CharacterRange { currentChunkOffset + misspellingLocation,  misspellingLength };

?

> Source/WebCore/editing/TextCheckingHelper.cpp:317
> +    outGrammarDetail.range.location = 0;
> +    outGrammarDetail.range.length = 0;

outGrammarDetail.range = { };

> Source/WebCore/editing/TextCheckingHelper.cpp:471
> +    outGrammarDetail.range.location = 0;
> +    outGrammarDetail.range.length = 0;

outGrammarDetail.range = { };

> Source/WebKit/UIProcess/WebGrammarDetail.cpp:48
> +    m_grammarDetail.range.location = location;
> +    m_grammarDetail.range.length = length;

m_grammarDetail.range = { location, length };

> Source/WebKit/UIProcess/gtk/TextCheckerGtk.cpp:273
> +        misspellingResult.range.location = offset + misspellingLocation;
> +        misspellingResult.range.length = misspellingLength;

etc

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5435
> +    CharacterRange characterRange;
> +    characterRange.location = WebCore::characterCount(*surroundingRange) + offset;
> +    characterRange.length = characterCount;

etc

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4081
> +    CharacterRange newSelectionRange;
> +    newSelectionRange.location = newSelectionLocation.unsafeGet();
> +    newSelectionRange.length = newSelectionLength.unsafeGet();

etc

> Source/WebKitLegacy/win/WebCoreSupport/WebEditorClient.cpp:805
> +        detail.range.location = location;
> +        detail.range.length = length;

auto characterRange = CharacterRange { location, length }

> Source/WebKitLegacy/win/WebView.cpp:7771
> +    CharacterRange characterRange;
> +    characterRange.location = location;
> +    characterRange.length = length;

How about just

auto characterRange = CharacterRange { location, length }

[Darin Adler]

I now think this breaks the Mac Catalyst build and I won’t land it before learning how to at least build that configuration locally. Also I think we need this configuration in EWS.

[Darin Adler]

Comment on attachment 394869 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394869&action=review

Thank you so much for the review! I really appreciate it. I could use a little help deciding what to do. A couple of your comments I can definitely do, the others open questions and I would like style advice.

>> Source/WebCore/editing/CharacterRange.h:40
>> +using CharacterCount = std::size_t;
> 
> Why is this alias helpful? Why not just use size_t?
> 
> (I know it is not new in this patch)

I think it’s helpful to make the semantics clearer. It’s like a variable name. Otherwise it just seems random what is int and what is size_t. But not 100% sure. I could definitely retire this type with a global replace.

Separate thought: maybe this should be std::uint64_t instead of std::size_t?

>> Source/WebCore/editing/CharacterRange.h:44
>> +    CharacterCount length { 0 };
> 
> It is bit weird that a location type is called "count".

It’s the count of the number of characters before that location. I am open to other ideas about the names. Sometimes people call this an index or an offset.

>> Source/WebCore/editing/CharacterRange.h:79
>> +}
> 
> Most clients are just unwrapping the Optional without any checking. It might be better to add a separate function for the few cases where kCFNotFound is actually a valid input (makeCharacterRangeIfValid or something) and make the regular version assert and return a plain CharacterRange.

Thanks for the suggestion!

Seems OK, that’s how I started, and I’d be willing to go back to that approach.

If I was willing to just assert I could make this a constructor of CharacterRange instead of a named function. Not sure how you feel about that? Makes the code more elegant, but also elegantly hides the fact that we might need to check for not found.

I don’t really need to provide a helper function; can just check for "not found" in all the places that need it. But I was thinking this Optional might help people remember the check in future code they write?

>> Source/WebCore/editing/TextCheckingHelper.cpp:67
>> +        badGrammar.range.length = badGrammarLength;
> 
> badGrammar.range = { checkLocation + badGrammarLocation, badGrammarLength };
> 
> ?

Can’t do exactly that because checkLocation is unsigned, badGrammarLocation is int, badGrammarLength is int, and ranges want CharacterCount/std::size_t. We could write:

    badGrammar.range = CharacterRange(checkLocation + badGrammarLocation, badGrammarLength);

If you like that better I’d be willing to go with it.

>> Source/WebCore/editing/TextCheckingHelper.cpp:101
>> +            misspelling.range.length = misspellingLength;
> 
> Similarly

Same issue/choice.

>> Source/WebCore/editing/TextCheckingHelper.cpp:279
>> +            mispellingCharacterRange.length = misspellingLength;
> 
> auto mispellingCharacterRange = CharacterRange { currentChunkOffset + misspellingLocation,  misspellingLength };
> 
> ?

Same thing again, the mix of types.

>> Source/WebCore/editing/TextCheckingHelper.cpp:317
>> +    outGrammarDetail.range.length = 0;
> 
> outGrammarDetail.range = { };

Will do. But really, out parameters stink and that’s the real fix. If we created a new GrammarDetail everything would be initialized.

>> Source/WebCore/editing/TextCheckingHelper.cpp:471
>> +    outGrammarDetail.range.length = 0;
> 
> outGrammarDetail.range = { };

OK.

>> Source/WebKit/UIProcess/WebGrammarDetail.cpp:48
>> +    m_grammarDetail.range.length = length;
> 
> m_grammarDetail.range = { location, length };

Same problem with types here. Could use the CharacterRange(location, length) syntax.

>> Source/WebKit/UIProcess/gtk/TextCheckerGtk.cpp:273
>> +        misspellingResult.range.length = misspellingLength;
> 
> etc

Yup, same thing again.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5435
>> +    characterRange.length = characterCount;
> 
> etc

Same problem with types here. std::size_t + std::int64_t might not yield a std::size_t.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4081
>> +    newSelectionRange.length = newSelectionLength.unsafeGet();
> 
> etc

Here it’s int64_t, not std::size_t.

>> Source/WebKitLegacy/win/WebCoreSupport/WebEditorClient.cpp:805
>> +        detail.range.length = length;
> 
> auto characterRange = CharacterRange { location, length }

Here it’s int.

>> Source/WebKitLegacy/win/WebView.cpp:7771
>> +    characterRange.length = length;
> 
> How about just
> 
> auto characterRange = CharacterRange { location, length }

Here it’s unsigned.

[Darin Adler]

Comment on attachment 394869 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394869&action=review

>>> Source/WebCore/editing/TextCheckingHelper.cpp:67
>>> +        badGrammar.range.length = badGrammarLength;
>> 
>> badGrammar.range = { checkLocation + badGrammarLocation, badGrammarLength };
>> 
>> ?
> 
> Can’t do exactly that because checkLocation is unsigned, badGrammarLocation is int, badGrammarLength is int, and ranges want CharacterCount/std::size_t. We could write:
> 
>     badGrammar.range = CharacterRange(checkLocation + badGrammarLocation, badGrammarLength);
> 
> If you like that better I’d be willing to go with it.

Or:

    badGrammar.range = { static_cast<CharacterCount>(checkLocation + badGrammarLocation), static_cast<CharacterCount>(badGrammarLength) };

[Antti Koivisto]

> If I was willing to just assert I could make this a constructor of
> CharacterRange instead of a named function. Not sure how you feel about
> that? Makes the code more elegant, but also elegantly hides the fact that we
> might need to check for not found.

I think for simple structs it is cleaner to use standalone construction functions that complex constructors

[Antti Koivisto]

Comment on attachment 394869 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394869&action=review

> Source/WebCore/editing/Editor.cpp:2776
> +static void correctSpellcheckingPreservingTextCheckingParagraph(TextCheckingParagraph& paragraph, Range& rangeToReplace, const String& replacement, CharacterCount resultLocation, CharacterCount resultLength)

This could take CharacterRange

> Source/WebCore/editing/Editor.cpp:3781
> +        CharacterCount subrangeOffset = scannerPosition + relativeStartPosition;
> +        CharacterCount subrangeLength = relativeEndPosition - relativeStartPosition + 1;

Could be CharacterRange

[Darin Adler]

Created attachment 394878 [details]
Patch

[Darin Adler]

Comment on attachment 394878 [details]
Patch

Already reviewed. Just need EWS to check various platforms before I land.

[Darin Adler]

(In reply to Antti Koivisto from comment #21)
> > If I was willing to just assert I could make this a constructor of
> > CharacterRange instead of a named function. Not sure how you feel about
> > that? Makes the code more elegant, but also elegantly hides the fact that we
> > might need to check for not found.
> 
> I think for simple structs it is cleaner to use standalone construction
> functions that complex constructors

Oh, I didn't see this comment. I ended up making CharacterRange allow construction with CFRange and NSRange. It seems to have turned out well enough that I won’t change it to makeCharacterRange right now. Could come back and do that later if we have regrets.

[Darin Adler]

Comment on attachment 394869 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394869&action=review

>> Source/WebCore/editing/Editor.cpp:3781
>> +        CharacterCount subrangeLength = relativeEndPosition - relativeStartPosition + 1;
> 
> Could be CharacterRange

Did all the other changes in the patch I just posed. Did this change, too, locally and will include it in the patch I land.

[Darin Adler]

Created attachment 394879 [details]
Patch

[Darin Adler]

Committed r259184: <https://trac.webkit.org/changeset/259184>

[Radar WebKit Bug Importer]

<rdar://problem/61036152>

[Alexey Proskuryakov]

Comment on attachment 394879 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394879&action=review

> LayoutTests/platform/mac-wk1/TestExpectations:241
> +# FIXME: Should be fixed in Catalina; need to re-enable.

Asked someone to look into this.

> LayoutTests/platform/mac-wk1/TestExpectations:723
> +fast/images/animated-gif-paint-after-animation.html [ Skip ]
> +fast/images/async-image-background-image-repeated.html [ Skip ]
> +fast/images/async-image-background-image.html [ Skip ]
> +fast/images/async-image-multiple-clients-repaint.html [ Skip ]
> +fast/images/composited-animated-gif-outside-viewport.html [ Skip ]
> +fast/images/decode-render-animated-image.html [ Skip ]
> +fast/images/decode-render-static-image.html [ Skip ]
> +fast/images/decoding-attribute-async-small-image.html [ Skip ]
> +fast/images/decoding-attribute-dynamic-async-small-image.html [ Skip ]
> +fast/images/sprite-sheet-image-draw.html [ Skip ]

We've got a lot of unfortunate test expectations...

> LayoutTests/platform/mac-wk2/TestExpectations:514
>  # RTL Scrollbars are enabled on Sierra WebKit2.

This comment is obsolete.

> LayoutTests/platform/mac-wk2/TestExpectations:544
>  # <rdar://problem/25063128>

This is closed as unreproducible. But still happening on some (internal) bots.

> LayoutTests/platform/mac-wk2/TestExpectations:-585
> -webkit.org/b/162524 [ Sierra Release ] http/tests/cache/disk-cache/disk-cache-redirect.html [ Pass Timeout ]

We need to update the bug.

> LayoutTests/platform/mac-wk2/TestExpectations:-602
> -webkit.org/b/163453 [ Sierra Release ] http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/basic-upgrade-cors.https.html [ Pass Timeout ]

We need to update the bug.

> LayoutTests/platform/mac-wk2/TestExpectations:-614
> -webkit.org/b/167206 [ Sierra Release ] mathml/opentype/large-operators-displaystyle-dynamic.html [ Pass ImageOnlyFailure ]

We need to update the bug.

> LayoutTests/platform/mac-wk2/TestExpectations:-643
> -webkit.org/b/171939 [ Sierra Release ] media/track/track-cue-rendering-on-resize.html [ Pass Timeout ]
> -
> -webkit.org/b/171947 [ Sierra Release ] transitions/extra-transition.html [ Pass Failure ]

We need to update the bugs.

> LayoutTests/platform/mac-wk2/TestExpectations:-649
> -webkit.org/b/172054 [ Sierra Debug ] loader/stateobjects/replacestate-size-iframe.html [ Pass Timeout ]

We need to update the bug.

> LayoutTests/platform/mac-wk2/TestExpectations:642
> +http/tests/storageAccess/request-and-grant-access-then-navigate-cross-site-should-not-have-access-database.html [ Pass Timeout ]
> +http/tests/storageAccess/deny-with-prompt-does-not-preserve-gesture.html [ Skip ]
> +http/tests/storageAccess/deny-with-prompt-does-not-preserve-gesture-database.html [ Skip ]

Something wrong about these, they are skipped everywhere with a confusing comment saying "only tested on Mac".

> LayoutTests/platform/mac-wk2/TestExpectations:-696
> -webkit.org/b/172829 [ Sierra Debug ] loader/stateobjects/replacestate-size.html [ Pass Timeout ]

We need to update the bug.

> LayoutTests/platform/mac-wk2/TestExpectations:700
> +http/tests/ssl/applepay/ApplePayError.html [ Pass ]
> +http/tests/ssl/applepay/ApplePaySessionV3.html [ Pass ]
> +http/tests/ssl/applepay/ApplePaySessionV4.html [ Pass ]
> +http/tests/ssl/applepay/ApplePaySessionV5.html [ Pass ]
> +http/tests/ssl/applepay/ApplePayRequestShippingContactV3.https.html [ Pass ]
> +http/tests/ssl/applepay/ApplePayShippingAddressChangeEventErrorsV3.https.html [ Pass ]
> +accessibility/mac/apple-pay-labels.html [ Pass ]
> +accessibility/mac/apple-pay-session-v4.html [ Pass ]

None of this seems to be necessary, there are no other expectations that would skip these tests.

> LayoutTests/platform/mac-wk2/TestExpectations:712
> +http/tests/resourceLoadStatistics/cookies-with-and-without-user-interaction.html [ Pass ]
> +http/tests/resourceLoadStatistics/cookie-deletion.html [ Pass ]
> +http/tests/resourceLoadStatistics/non-prevalent-resources-can-access-cookies-in-a-third-party-context.html [ Pass ]
> +http/tests/resourceLoadStatistics/add-blocking-to-redirect.html [ Pass ]
> +http/tests/resourceLoadStatistics/do-not-remove-blocking-in-redirect.html [ Pass ]
> +http/tests/resourceLoadStatistics/clear-in-memory-and-persistent-store.html [ Pass ]
> +http/tests/resourceLoadStatistics/clear-in-memory-and-persistent-store-one-hour.html [ Pass ]
> +http/tests/resourceLoadStatistics/grandfathering.html [ Pass ]
> +http/tests/resourceLoadStatistics/strip-referrer-to-origin-for-third-party-redirects.html [ Pass ]
> +http/tests/resourceLoadStatistics/strip-referrer-to-origin-for-third-party-requests.html [ Pass ]
> +http/tests/resourceLoadStatistics/cap-cache-max-age-for-prevalent-resource.html [ Pass ]

Not sure why we are doing this one by one, not enabling the whole http/tests/resourceLoadStatistics directory.

> LayoutTests/platform/mac-wk2/TestExpectations:-759
> -webkit.org/b/178553 [ HighSierra Release ] imported/w3c/web-platform-tests/html/dom/dynamic-markup-insertion/opening-the-input-stream/011.html [ Skip ]
> -
> -webkit.org/b/165620 [ Sierra ] http/tests/cache/disk-cache/disk-cache-request-headers.html [ Pass Failure ]

We need to update the bugs.

> LayoutTests/platform/mac-wk2/TestExpectations:-797
> -webkit.org/b/183860 [ Sierra Release ] http/wpt/service-workers/third-party-registration.html [ Pass Failure ]

We need to update the bug.

> LayoutTests/platform/mac-wk2/TestExpectations:-857
> -webkit.org/b/191658 [ Sierra Release ] fast/layers/no-clipping-overflow-hidden-added-after-transform.html [ Pass ImageOnlyFailure ]

We need to update the bug.

> LayoutTests/platform/mac/TestExpectations:1169
>  # rdar://problem/27475162

We need to update the bug.

> LayoutTests/platform/mac/TestExpectations:1188
> +webkit.org/b/159755 fast/text/emoji-num-glyphs.html [ Pass ]
> +webkit.org/b/159755 fast/text/emoji-gender-2-3.html [ Pass ]
> +webkit.org/b/159755 fast/text/emoji-gender-2-4.html [ Pass ]
> +webkit.org/b/159755 fast/text/emoji-gender-2-5.html [ Pass ]
> +webkit.org/b/159755 fast/text/emoji-gender-2-6.html [ Pass ]
> +webkit.org/b/159755 fast/text/emoji-gender-2-7.html [ Pass ]
> +webkit.org/b/159755 fast/text/emoji-gender-2-8.html [ Pass ]
> +webkit.org/b/159755 fast/text/emoji-gender-2-9.html [ Pass ]
> +
> +webkit.org/b/159755 fast/text/emoji-gender-fe0f-3.html [ Pass ]
> +webkit.org/b/159755 fast/text/emoji-gender-fe0f-4.html [ Pass ]
> +webkit.org/b/159755 fast/text/emoji-gender-fe0f-5.html [ Pass ]
> +webkit.org/b/159755 fast/text/emoji-gender-fe0f-6.html [ Pass ]
> +webkit.org/b/159755 fast/text/emoji-gender-fe0f-7.html [ Pass ]
> +webkit.org/b/159755 fast/text/emoji-gender-fe0f-8.html [ Pass ]
> +webkit.org/b/159755 fast/text/emoji-gender-fe0f-9.html [ Pass ]
> +webkit.org/b/159755 fast/text/emoji-gender.html [ Pass ]

Hmm, why are we not running these on iOS.

> LayoutTests/platform/mac/TestExpectations:-1304
> -webkit.org/b/171272 [ Sierra ] fast/text/kaithi.html [ ImageOnlyFailure ]

We need to update the bug.

> LayoutTests/platform/mac/TestExpectations:-1341
> -webkit.org/b/173328 [ Sierra ] fast/text/system-font-fallback-emoji.html [ Failure ]
> -webkit.org/b/173328 [ Sierra ] fast/text/system-font-fallback.html [ ImageOnlyFailure ]

We need to update the bugs.

> LayoutTests/platform/mac/TestExpectations:1389
>  # <rdar://problem/29031509> REGRESSION? (FontParser-195): svg/W3C-SVG-1.1/fonts-elem-* and svg/W3C-SVG-1.1/text-intro-* tests failing
> -[ HighSierra+ ] svg/W3C-SVG-1.1/fonts-elem-01-t.svg [ Failure ]
> -[ HighSierra+ ] svg/W3C-SVG-1.1/fonts-elem-02-t.svg [ Failure ]
> -[ HighSierra+ ] svg/W3C-SVG-1.1/fonts-elem-03-b.svg [ Failure ]
> -[ HighSierra+ ] svg/W3C-SVG-1.1/fonts-elem-07-b.svg [ Failure ]
> -[ HighSierra+ ] svg/W3C-SVG-1.1/text-intro-01-t.svg [ Failure ]
> -[ HighSierra+ ] svg/W3C-SVG-1.1/text-intro-02-b.svg [ Failure ]
> -[ HighSierra+ ] svg/W3C-SVG-1.1/text-intro-03-b.svg [ Failure ]
> -[ HighSierra+ ] svg/W3C-SVG-1.1/text-intro-04-t.svg [ Failure ]
> -[ HighSierra+ ] svg/batik/text/textEffect3.svg [ Failure ]
> -[ HighSierra+ ] svg/batik/text/textPosition2.svg [ Failure ]
> -[ HighSierra+ ] svg/custom/acid3-test-77.html [ Failure ]
> -[ HighSierra+ ] svg/custom/svg-fonts-fallback.xhtml [ Failure ]
> -[ HighSierra+ ] svg/wicd/test-rightsizing-b.xhtml [ Failure ]
> -[ HighSierra+ ] fast/css-generated-content/initial-letter-first-line-wrapping.html [ ImageOnlyFailure  ]
> +svg/W3C-SVG-1.1/fonts-elem-01-t.svg [ Failure ]
> +svg/W3C-SVG-1.1/fonts-elem-02-t.svg [ Failure ]
> +svg/W3C-SVG-1.1/fonts-elem-03-b.svg [ Failure ]
> +svg/W3C-SVG-1.1/fonts-elem-07-b.svg [ Failure ]
> +svg/W3C-SVG-1.1/text-intro-01-t.svg [ Failure ]
> +svg/W3C-SVG-1.1/text-intro-02-b.svg [ Failure ]
> +svg/W3C-SVG-1.1/text-intro-03-b.svg [ Failure ]
> +svg/W3C-SVG-1.1/text-intro-04-t.svg [ Failure ]
> +svg/batik/text/textEffect3.svg [ Failure ]
> +svg/batik/text/textPosition2.svg [ Failure ]
> +svg/custom/acid3-test-77.html [ Failure ]
> +svg/custom/svg-fonts-fallback.xhtml [ Failure ]
> +svg/wicd/test-rightsizing-b.xhtml [ Failure ]
> +fast/css-generated-content/initial-letter-first-line-wrapping.html [ ImageOnlyFailure  ]

This bug is marked as fixed in High Sierra.

> LayoutTests/platform/mac/TestExpectations:1391
>  # <rdar://problem/30493910> REGRESSION: LayoutTest fast/writing-mode/broken-ideograph-small-caps.html failing

This is marked as no longer happening in High Sierra.

> LayoutTests/platform/mac/TestExpectations:1394
>  # <rdar://problem/30182401> REGRESSION: LayoutTest media/media-source/media-source-abort-resets-parser.html failing

This is marked as a duplicate of a fixed bug.

> LayoutTests/platform/mac/TestExpectations:1397
>  # <rdar://problem/30517283> REGRESSION: LayoutTests security/block-test.html and security/block-test-no-port.html failing

This too. There are probably quite a few more, I stopped checking.

> LayoutTests/platform/mac/TestExpectations:1425
>  # WebCryptoAPI features that enabled only for High Sierra/iOS 11

Surprising that we have to enable these one by one.

> LayoutTests/platform/mac/TestExpectations:1635
> +css-dark-mode/older-systems [ Skip ]

This directory should be deleted instead.

It is surprising that so much unrelated TestExpectations cleanup was done in this patch, without expert review.

> LayoutTests/platform/mac/TestExpectations:-1703
> -webkit.org/b/199275 [ HighSierra ] webgpu [ Skip ]
> -webkit.org/b/199275 [ HighSierra ] inspector/canvas/create-context-webgpu.html [ Skip ]
> -webkit.org/b/199275 [ HighSierra ] inspector/canvas/requestClientNodes-webgpu.html [ Skip ]
> -webkit.org/b/199275 [ HighSierra ] inspector/canvas/requestShaderSource-webgpu.html [ Skip ]
> -webkit.org/b/199275 [ HighSierra ] inspector/canvas/resolveContext-webgpu.html [ Skip ]
> -webkit.org/b/199275 [ HighSierra ] inspector/canvas/shaderProgram-add-remove-webgpu.html [ Skip ]
> -webkit.org/b/199275 [ HighSierra ] inspector/canvas/updateShader-webgpu.html [ Skip ]

I updated this bug.

[Darin Adler]

What I did in the TestExpectations was expand Sierra and HighSierra to be "false", convert Sierra+, HighSierra+, and Mojave+ to be "true".

And make a small number of changes specifically about this patch.

No other cleanup or rationalization. That can be done by someone else in another bug.

[Darin Adler]

(In reply to Darin Adler from comment #31)
> No other cleanup or rationalization. That can be done by someone else in
> another bug.

Sorry, that’s wrong. I did a tiny bit more. Adding Pass to tests that are passing.