Make TextCheckingTypeMask an OptionSet for type safety and code clarity.
Created attachment 347303 [details] Patch
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
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
Created attachment 347313 [details] Patch
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.
Created attachment 347316 [details] Patch
Created attachment 347318 [details] Patch
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.
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.
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())
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.
*** Bug 188510 has been marked as a duplicate of this bug. ***
(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.
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); }
(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?
(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().
Created attachment 347365 [details] For landing
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.
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?
(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.
(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.
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
(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.
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.
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 });
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);
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 });
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);
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.
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.
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.
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.
(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)!
Created attachment 347603 [details] Patch Another workaround. * const bool shouldPerformReplacement → bool shouldPerformReplacement
(In reply to Fujii Hironori from comment #34) > Another workaround. > > * const bool shouldPerformReplacement → bool shouldPerformReplacement Oops. Exactly same workaround with comment 27.
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
(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.
(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>.
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
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
Committed r235120: <https://trac.webkit.org/changeset/235120>
<rdar://problem/43565123>