[webkit-reviews] review granted: [Bug 188678] Make TextCheckingTypeMask an OptionSet : [Attachment 347318] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 188678: Make TextCheckingTypeMask an OptionSet
https://bugs.webkit.org/show_bug.cgi?id=188678

Attachment 347318: Patch

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




--- Comment #10 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 347318
  --> https://bugs.webkit.org/attachment.cgi?id=347318
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())