Replace TextCheckingTypeMask with OptionSet

[Daniel Bates]

Make TextCheckingTypeMask an OptionSet for type safety and code clarity.

[Daniel Bates]

Created attachment 347303 [details]
Patch

[EWS Watchlist]

Comment on attachment 347303 [details]
Patch

Attachment 347303 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8884727

New failing tests:
accessibility/mac/attributed-string-includes-misspelled-with-selection.html
accessibility/mac/attributed-string/attributed-string-does-not-includes-misspelled-for-non-editable.html
editing/spelling/context-menu-suggestions.html
accessibility/mac/attributed-string/attributed-string-for-range.html
accessibility/mac/misspelled-attributed-string.html
accessibility/content-editable-as-textarea.html
accessibility/mac/attributed-string/attributed-string-for-range-with-options.html

[EWS Watchlist]

Created attachment 347312 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Daniel Bates]

Created attachment 347313 [details]
Patch

[EWS Watchlist]

Attachment 347313 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/mac/TextCheckerMac.mm:343:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 347316 [details]
Patch

[Daniel Bates]

Created attachment 347318 [details]
Patch

[EWS Watchlist]

Attachment 347318 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/mac/TextCheckerMac.mm:343:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 347318 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:3723
> -        textCheckingOptions &= TextCheckingTypeReplacement;
> +        textCheckingOptions = TextCheckingType::Replacement;

This looks like a change in behavior.  It could contain other things.

[Antti Koivisto]

Comment on attachment 347318 [details]
Patch

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

r=me with the logic mistake fixed.

> Source/WebCore/SourcesCocoa.txt:530
> +platform/text/mac/TextCheckingMac.mm

Shouldn't this be platform/text/cocoa/TextCheckingCocoa or is this really Mac specific?

>> Source/WebCore/editing/Editor.cpp:3723
>> +        textCheckingOptions = TextCheckingType::Replacement;
> 
> This looks like a change in behavior.  It could contain other things.

This should be 

textCheckingOptions = textCheckingOptions & TextCheckingType::Replacement;

This is so rare that it is probably better to not add OptionSet::operator&= as it would be easy to use by mistake.

> Source/WebCore/editing/SpellChecker.cpp:220
> -        if (requestData.mask() & TextCheckingTypeSpelling)
> +        if (requestData.mask() & TextCheckingType::Spelling)

It would be nice to rename mask() to something more descriptive.

> Source/WebCore/editing/TextCheckingHelper.cpp:586
> -    TextCheckingTypeMask checkingTypes = checkGrammar ? (TextCheckingTypeSpelling | TextCheckingTypeGrammar) : TextCheckingTypeSpelling;
> +    TextCheckingTypeMask checkingTypes = checkGrammar ? TextCheckingTypeMask { TextCheckingType::Spelling, TextCheckingType::Grammar } : TextCheckingTypeMask { TextCheckingType::Spelling };

You could use auto.

Alternatively using if may read better:

TextCheckingTypeMask checkingTypes { TextCheckingType::Spelling };
if (checkGrammar)
   checkingTypes |= { TextCheckingType::Grammar };

> Source/WebCore/platform/text/TextChecking.h:53
> -typedef unsigned TextCheckingTypeMask;
> +using TextCheckingTypeMask = OptionSet<TextCheckingType>;

OptionSet is not really a "mask" so this should at least be renamed. I would prefer just removing the whole type and instead using OptionSet<TextCheckingType> everywhere. It is more readable that way.

> Source/WebCore/platform/text/TextChecking.h:87
> -        , m_mask(TextCheckingTypeNone)
> +        , m_mask(TextCheckingType::None)

This line can be removed, empty set is initialized by default.

Also if TextCheckingTypeMask is renamed, m_mask should be too.

> Source/WebKit/UIProcess/gtk/TextCheckerGtk.cpp:206
> -    ASSERT(request.mask() != TextCheckingTypeNone);
> +    ASSERT(request.mask() != TextCheckingType::None);

This can be just ASSERT(request.mask())

[Antti Koivisto]

Comment on attachment 347318 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:2647
>      TextCheckingTypeMask textCheckingOptions = request.data().mask();

auto

> Source/WebCore/editing/TextCheckingHelper.cpp:354
> -                TextCheckingTypeMask checkingTypes = checkGrammar ? (TextCheckingTypeSpelling | TextCheckingTypeGrammar) : TextCheckingTypeSpelling;
> +                TextCheckingTypeMask checkingTypes = checkGrammar ? TextCheckingTypeMask { TextCheckingType::Spelling, TextCheckingType::Grammar } : TextCheckingTypeMask { TextCheckingType::Spelling };

Same as above here.

[Antti Koivisto]

*** Bug 188510 has been marked as a duplicate of this bug. ***

[Daniel Bates]

(In reply to Antti Koivisto from comment #10)
> > Source/WebCore/SourcesCocoa.txt:530
> > +platform/text/mac/TextCheckingMac.mm
> 
> Shouldn't this be platform/text/cocoa/TextCheckingCocoa or is this really
> Mac specific?
> 

Yes, it is Mac specific. On iOS we use different machinery (UITextChecker) to perform spell checking that does not make use of the flags NSTextCheckingType.

> >> Source/WebCore/editing/Editor.cpp:3723
> >> +        textCheckingOptions = TextCheckingType::Replacement;
> > 
[...]
> 
> This should be 
> 
> textCheckingOptions = textCheckingOptions & TextCheckingType::Replacement;
> 
> This is so rare that it is probably better to not add OptionSet::operator&=
> as it would be easy to use by mistake.

Will fix.

> 
> > Source/WebCore/editing/SpellChecker.cpp:220
> > -        if (requestData.mask() & TextCheckingTypeSpelling)
> > +        if (requestData.mask() & TextCheckingType::Spelling)
> 
> It would be nice to rename mask() to something more descriptive.
> 

Maybe typesToCheck?

> > Source/WebCore/editing/TextCheckingHelper.cpp:586
> > -    TextCheckingTypeMask checkingTypes = checkGrammar ? (TextCheckingTypeSpelling | TextCheckingTypeGrammar) : TextCheckingTypeSpelling;
> > +    TextCheckingTypeMask checkingTypes = checkGrammar ? TextCheckingTypeMask { TextCheckingType::Spelling, TextCheckingType::Grammar } : TextCheckingTypeMask { TextCheckingType::Spelling };
> 
> You could use auto.
> 
> Alternatively using if may read better:
> 
> TextCheckingTypeMask checkingTypes { TextCheckingType::Spelling };
> if (checkGrammar)
>    checkingTypes |= { TextCheckingType::Grammar };
> 

Will change.

> > Source/WebCore/platform/text/TextChecking.h:53
> > -typedef unsigned TextCheckingTypeMask;
> > +using TextCheckingTypeMask = OptionSet<TextCheckingType>;
> 
> OptionSet is not really a "mask" so this should at least be renamed. I would
> prefer just removing the whole type and instead using
> OptionSet<TextCheckingType> everywhere. It is more readable that way.
> 

Will substitute OptionSet<TextCheckingType> for TextCheckingTypeMask and remove the type alias.

> > Source/WebCore/platform/text/TextChecking.h:87
> > -        , m_mask(TextCheckingTypeNone)
> > +        , m_mask(TextCheckingType::None)
> 
> This line can be removed, empty set is initialized by default.
> 

Will remove.

> Also if TextCheckingTypeMask is renamed, m_mask should be too.
> 
> > Source/WebKit/UIProcess/gtk/TextCheckerGtk.cpp:206
> > -    ASSERT(request.mask() != TextCheckingTypeNone);
> > +    ASSERT(request.mask() != TextCheckingType::None);
> 
> This can be just ASSERT(request.mask())

Will change.

[Daniel Bates]

Comment on attachment 347318 [details]
Patch

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

> Source/WebCore/editing/SpellChecker.cpp:222
> -        if (requestData.mask() & TextCheckingTypeSpelling)
> +        if (requestData.mask() & TextCheckingType::Spelling)
>              markerTypes |= DocumentMarker::Spelling;
> -        if (requestData.mask() & TextCheckingTypeGrammar)
> +        if (requestData.mask() & TextCheckingType::Grammar)

Will change this code to read:

if (requestData.sequence() == sequence) {
    OptionSet<DocumentMarker::MarkerType> markerTypes;
    if (requestData.mask().contains(TextCheckingType::Spelling))
        markerTypes |= DocumentMarker::Spelling;
    if (requestData.mask().contains(TextCheckingType::Grammar))
        markerTypes |= DocumentMarker::Grammar;
    if (!markerTypes.isEmpty())
        m_frame.document()->markers().removeMarkers(&m_processingRequest->checkingRange(), markerTypes);
}

[Daniel Bates]

(In reply to Daniel Bates from comment #13)
> > > Source/WebCore/editing/SpellChecker.cpp:220
> > > -        if (requestData.mask() & TextCheckingTypeSpelling)
> > > +        if (requestData.mask() & TextCheckingType::Spelling)
> > 
> > It would be nice to rename mask() to something more descriptive.
> > 
> 
> Maybe typesToCheck?
> 

Or checkingTypes?

[Daniel Bates]

(In reply to Daniel Bates from comment #15)
> (In reply to Daniel Bates from comment #13)
> > > > Source/WebCore/editing/SpellChecker.cpp:220
> > > > -        if (requestData.mask() & TextCheckingTypeSpelling)
> > > > +        if (requestData.mask() & TextCheckingType::Spelling)
> > > 
> > > It would be nice to rename mask() to something more descriptive.
> > > 
> > 
> > Maybe typesToCheck?
> > 
> 
> Or checkingTypes?

I chose to rename TextCheckingRequestData::mask() to TextCheckingRequestData::checkingTypes().

[Daniel Bates]

Created attachment 347365 [details]
For landing

[EWS Watchlist]

Attachment 347365 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/mac/TextCheckerMac.mm:343:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

For some reason the Windows and WinCairo bots are not happy with the patch (attachment #347318 [details]) and have similar error messages:

[[
 UnifiedSource232.cpp
C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/OptionSet.h(84): error C2131: expression did not evaluate to a constant (compiling source file C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource223.cpp) [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
  C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/OptionSet.h(84): note: failure was caused by a read of a variable outside its lifetime (compiling source file C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource223.cpp)
  C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/OptionSet.h(84): note: see usage of '<begin>$L0' (compiling source file C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource223.cpp)
]]
<https://webkit-queues.webkit.org/results/8890767>

Anybody have any insight/suggestions?

[Ross Kirsling]

(In reply to Daniel Bates from comment #19)
> For some reason the Windows and WinCairo bots are not happy with the patch
> (attachment #347318 [details]) and have similar error messages:
> 
> [[
>  UnifiedSource232.cpp
> C:
> \cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHea
> ders\wtf/OptionSet.h(84): error C2131: expression did not evaluate to a
> constant (compiling source file
> C:
> \cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\WebCore\unifi
> ed-sources\UnifiedSource223.cpp)
> [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.
> vcxproj]
>  
> C:
> \cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHea
> ders\wtf/OptionSet.h(84): note: failure was caused by a read of a variable
> outside its lifetime (compiling source file
> C:
> \cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\WebCore\unifi
> ed-sources\UnifiedSource223.cpp)
>  
> C:
> \cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHea
> ders\wtf/OptionSet.h(84): note: see usage of '<begin>$L0' (compiling source
> file
> C:
> \cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\WebCore\unifi
> ed-sources\UnifiedSource223.cpp)
> ]]
> <https://webkit-queues.webkit.org/results/8890767>
> 
> Anybody have any insight/suggestions?

I think we had a related issue here:
https://bugs.webkit.org/show_bug.cgi?id=188501

I'll see what I can find.

[Don Olmstead]

(In reply to Ross Kirsling from comment #20)
> (In reply to Daniel Bates from comment #19)
> > For some reason the Windows and WinCairo bots are not happy with the patch
> > (attachment #347318 [details]) and have similar error messages:
> > 
> > [[
> >  UnifiedSource232.cpp
> > C:
> > \cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHea
> > ders\wtf/OptionSet.h(84): error C2131: expression did not evaluate to a
> > constant (compiling source file
> > C:
> > \cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\WebCore\unifi
> > ed-sources\UnifiedSource223.cpp)
> > [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.
> > vcxproj]
> >  
> > C:
> > \cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHea
> > ders\wtf/OptionSet.h(84): note: failure was caused by a read of a variable
> > outside its lifetime (compiling source file
> > C:
> > \cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\WebCore\unifi
> > ed-sources\UnifiedSource223.cpp)
> >  
> > C:
> > \cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHea
> > ders\wtf/OptionSet.h(84): note: see usage of '<begin>$L0' (compiling source
> > file
> > C:
> > \cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\WebCore\unifi
> > ed-sources\UnifiedSource223.cpp)
> > ]]
> > <https://webkit-queues.webkit.org/results/8890767>
> > 
> > Anybody have any insight/suggestions?
> 
> I think we had a related issue here:
> https://bugs.webkit.org/show_bug.cgi?id=188501
> 
> I'll see what I can find.

I think the issue is around usages of { } which end up in the std::initializer_list constructor. MSVC is not handling those properly and doesn't think they're constexpr.

[Don Olmstead]

Comment on attachment 347365 [details]
For landing

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

Overall patch looks great. VC++ definitely has a bug there.

> Source/WebCore/editing/Editor.cpp:2840
> +        OptionSet<TextCheckingType> textCheckingOptions { TextCheckingType::Spelling };

Can you try using () instead of {} for these cases? I think VC++ should compile it out then.

> Source/WebCore/editing/TextCheckingHelper.cpp:354
> +                OptionSet<TextCheckingType> checkingTypes { TextCheckingType::Spelling };

Same

> Source/WebCore/editing/TextCheckingHelper.cpp:588
> +    OptionSet<TextCheckingType> checkingTypes { TextCheckingType::Spelling };

Same

[Ross Kirsling]

(In reply to Don Olmstead from comment #22)
> Comment on attachment 347365 [details]
> For landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347365&action=review
> 
> Overall patch looks great. VC++ definitely has a bug there.
> 
> > Source/WebCore/editing/Editor.cpp:2840
> > +        OptionSet<TextCheckingType> textCheckingOptions { TextCheckingType::Spelling };
> 
> Can you try using () instead of {} for these cases? I think VC++ should
> compile it out then.
> 
> > Source/WebCore/editing/TextCheckingHelper.cpp:354
> > +                OptionSet<TextCheckingType> checkingTypes { TextCheckingType::Spelling };
> 
> Same
> 
> > Source/WebCore/editing/TextCheckingHelper.cpp:588
> > +    OptionSet<TextCheckingType> checkingTypes { TextCheckingType::Spelling };
> 
> Same

Even though there is something weird with initializer_list and constexpr in MSVC, it seems that changing these lines won't help us at all. :(
I should have a(n initial) working suggestion shortly.

[Ross Kirsling]

Comment on attachment 347365 [details]
For landing

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

> Source/WebCore/editing/Editor.cpp:2653
> +    const bool shouldPerformReplacement = textCheckingOptions.containsAny({ TextCheckingType::Quote, TextCheckingType::Dash, TextCheckingType::Replacement });

This is evidently the singular failing line.

If you change the lvalue to an rvalue, compilation succeeds:
> const bool shouldPerformReplacement = request.data().checkingTypes().containsAny({ TextCheckingType::Quote, TextCheckingType::Dash, TextCheckingType::Replacement });

FWIW, either of the following unergonomic options also technically work:

>  const bool shouldPerformReplacement = textCheckingOptions.containsAny(OptionSet<TextCheckingType>(TextCheckingType::Quote) | OptionSet<TextCheckingType>(TextCheckingType::Dash) | OptionSet<TextCheckingType>(TextCheckingType::Replacement));

> const bool shouldPerformReplacement = textCheckingOptions.contains(TextCheckingType::Quote) || textCheckingOptions.contains(TextCheckingType::Dash) || textCheckingOptions.contains(TextCheckingType::Replacement);

...but simply making the coercion from initializer_list to OptionSet<TextCheckingType> explicit does not help.

This really ought to be addressed in OptionSet and not at the containsAny callsite, but I'm still not 100% clear on the proper fix.

[Daniel Bates]

Created attachment 347562 [details]
For EWS: Ross Kirsling's first suggested fix

Change:

const bool shouldPerformReplacement = textCheckingOptions.containsAny({ TextCheckingType::Quote, TextCheckingType::Dash, TextCheckingType::Replacement });

to read:

const bool shouldPerformReplacement = request.data().checkingTypes().containsAny({ TextCheckingType::Quote, TextCheckingType::Dash, TextCheckingType::Replacement });

[Daniel Bates]

Created attachment 347563 [details]
For EWS: Ross Kirsling's second suggested fix

Change:

const bool shouldPerformReplacement = textCheckingOptions.containsAny({ TextCheckingType::Quote, TextCheckingType::Dash, TextCheckingType::Replacement });

to read:

const bool shouldPerformReplacement = textCheckingOptions.contains(TextCheckingType::Quote) || textCheckingOptions.contains(TextCheckingType::Dash) || textCheckingOptions.contains(TextCheckingType::Replacement);

[Daniel Bates]

Created attachment 347565 [details]
For EWS: Make shouldPerformReplacement non-const

Change:

const bool shouldPerformReplacement = textCheckingOptions.containsAny({ TextCheckingType::Quote, TextCheckingType::Dash, TextCheckingType::Replacement });

to read:

bool shouldPerformReplacement = textCheckingOptions.containsAny({ TextCheckingType::Quote, TextCheckingType::Dash, TextCheckingType::Replacement });

[Daniel Bates]

Created attachment 347566 [details]
For EWS: Explicitly pass constexpr to OptionSet::containsAny()

Change:

const bool shouldPerformReplacement = textCheckingOptions.containsAny({ TextCheckingType::Quote, TextCheckingType::Dash, TextCheckingType::Replacement });

to read:

constexpr OptionSet<TextCheckingType> replacementCriterion { TextCheckingType::Quote, TextCheckingType::Dash, TextCheckingType::Replacement };
const bool shouldPerformReplacement = textCheckingOptions.containsAny(replacementCriterion);

[EWS Watchlist]

Attachment 347562 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/mac/TextCheckerMac.mm:343:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Attachment 347563 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/mac/TextCheckerMac.mm:343:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Attachment 347565 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/mac/TextCheckerMac.mm:343:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Attachment 347566 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/mac/TextCheckerMac.mm:343:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ross Kirsling]

(In reply to Daniel Bates from comment #27)
> Created attachment 347565 [details]
> For EWS: Make shouldPerformReplacement non-const
> 
> Change:
> 
> const bool shouldPerformReplacement = textCheckingOptions.containsAny({
> TextCheckingType::Quote, TextCheckingType::Dash,
> TextCheckingType::Replacement });
> 
> to read:
> 
> bool shouldPerformReplacement = textCheckingOptions.containsAny({
> TextCheckingType::Quote, TextCheckingType::Dash,
> TextCheckingType::Replacement });

Somehow this one only got sent to a subset of the EWS bots.
It does appear to work locally though (to my surprise)!

[Fujii Hironori]

Created attachment 347603 [details]
Patch

Another workaround.

* const bool shouldPerformReplacement → bool shouldPerformReplacement

[Fujii Hironori]

(In reply to Fujii Hironori from comment #34)
> Another workaround.
> 
> * const bool shouldPerformReplacement → bool shouldPerformReplacement

Oops. Exactly same workaround with comment 27.

[Fujii Hironori]

I created a reproducer and reportd.

https://godbolt.org/z/onbBA9
https://gist.github.com/fujii/96ab8fef265870f36ad05c8faf7c3031
https://developercommunity.visualstudio.com/content/problem/316713/msvc-cant-compile-webkits-optionsetcontainsany.html

[Daniel Bates]

(In reply to Ross Kirsling from comment #33)
> (In reply to Daniel Bates from comment #27)
> > Created attachment 347565 [details]
> > For EWS: Make shouldPerformReplacement non-const
> > 
> > Change:
> > 
> > const bool shouldPerformReplacement = textCheckingOptions.containsAny({
> > TextCheckingType::Quote, TextCheckingType::Dash,
> > TextCheckingType::Replacement });
> > 
> > to read:
> > 
> > bool shouldPerformReplacement = textCheckingOptions.containsAny({
> > TextCheckingType::Quote, TextCheckingType::Dash,
> > TextCheckingType::Replacement });
> 
> Somehow this one only got sent to a subset of the EWS bots.
> It does appear to work locally though (to my surprise)!

Yay! Thank you for confirming. I am going to go with this approach.

[Daniel Bates]

(In reply to Fujii Hironori from comment #36)
> I created a reproducer and reportd.
> 
> https://godbolt.org/z/onbBA9
> https://gist.github.com/fujii/96ab8fef265870f36ad05c8faf7c3031
> https://developercommunity.visualstudio.com/content/problem/316713/msvc-cant-
> compile-webkits-optionsetcontainsany.html

Thank you Fujii for creating the reduction and reporting it. Maybe someone should tweet the reduction to the Twitter thread: <https://twitter.com/UlziiLuvsanbat/status/1030589141250007040>.

[EWS Watchlist]

Comment on attachment 347563 [details]
For EWS: Ross Kirsling's second suggested fix

Attachment 347563 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8928919

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html

[EWS Watchlist]

Created attachment 347633 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Daniel Bates]

Committed r235120: <https://trac.webkit.org/changeset/235120>

[Radar WebKit Bug Importer]

<rdar://problem/43565123>