Bug 57088 – Refactoring: Editor::TextCheckingOptions should be replaced with TextCheckingType

• Component: HTML Editing
• Status: RESOLVED FIXED

[Hajime Morrita]

These two are almost identical. We should use one of them.
And the enum is actually a bitmask. So TextCheckingType should be a class, which has bitfields for each flag.

Marked blocked by 2 refactoring bugs, which touch the spellchecking area.

[Hajime Morrita]

Created attachment 88559 [details]
Patch

[WebKit Review Bot]

Attachment 88559 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8346447

[Hajime Morrita]

Created attachment 88562 [details]
Patch

[WebKit Review Bot]

Attachment 88562 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8348427

[Ryosuke Niwa]

Comment on attachment 88562 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:2226
> +    bool shouldMarkSpelling = textCheckingTypes.spelling();
> +    bool shouldMarkGrammar = textCheckingTypes.grammar();
> +    bool shouldPerformReplacement = textCheckingTypes.replacement();
> +    bool shouldShowCorrectionPanel = textCheckingTypes.correctionPanel();
> +    bool shouldCheckForCorrection = shouldShowCorrectionPanel || textCheckingTypes.correction();

I think these member functions should be named as isSpelling, isGrammar, etc...

> Source/WebCore/editing/Editor.cpp:3623
> -TextCheckingTypeMask Editor::textCheckingTypeMaskFor(TextCheckingOptions textCheckingOptions)
> +TextCheckingTypes Editor::addTextCheckSettings(const TextCheckingTypes& textCheckingTypes)

I don't think this function name makes sense since you're not modifying the original textCheckingTypes.  Something like resolveReplacedTextCheckingType might work.

[WebKit Review Bot]

Attachment 88562 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8341639

[Hajime Morrita]

Created attachment 88665 [details]
Patch

[Hajime Morrita]

Hi Ryosuke, thank you for taking a look!
I update the patch to your feedback.

(In reply to comment #5)
> (From update of attachment 88562 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88562&action=review
> 
> > Source/WebCore/editing/Editor.cpp:2226
> > +    bool shouldMarkSpelling = textCheckingTypes.spelling();
> > +    bool shouldMarkGrammar = textCheckingTypes.grammar();
> > +    bool shouldPerformReplacement = textCheckingTypes.replacement();
> > +    bool shouldShowCorrectionPanel = textCheckingTypes.correctionPanel();
> > +    bool shouldCheckForCorrection = shouldShowCorrectionPanel || textCheckingTypes.correction();
> 
> I think these member functions should be named as isSpelling, isGrammar, etc...
Fixed.

> 
> > Source/WebCore/editing/Editor.cpp:3623
> > -TextCheckingTypeMask Editor::textCheckingTypeMaskFor(TextCheckingOptions textCheckingOptions)
> > +TextCheckingTypes Editor::addTextCheckSettings(const TextCheckingTypes& textCheckingTypes)
> 
> I don't think this function name makes sense since you're not modifying the original textCheckingTypes.  Something like resolveReplacedTextCheckingType might work.
Fixed.

[Hajime Morrita]

Created attachment 89049 [details]
Updated to ToT

[Hajime Morrita]

Created attachment 89871 [details]
Updated to ToT

[WebKit Review Bot]

Attachment 89871 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8457309

[Daniel Bates]

Attachment 89871 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8436375

[Hajime Morrita]

Created attachment 90210 [details]
A build fix

[Hajime Morrita]

Build passed. It's ready to review!

[Jia Pu]

Comment on attachment 90210 [details]
A build fix

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

> Source/WebCore/editing/Editor.cpp:3249
> -    bool shouldMarkSpelling = textCheckingOptions & MarkSpelling;
> -    bool shouldMarkGrammar = textCheckingOptions & MarkGrammar;
> -    bool shouldShowCorrectionPanel = textCheckingOptions & ShowCorrectionPanel;
> -    bool shouldCheckForCorrection = shouldShowCorrectionPanel || (textCheckingOptions & CheckForCorrection);
> -
> -    TextCheckingTypeMask checkingTypes = 0;
> -    if (shouldMarkSpelling)
> -        checkingTypes |= TextCheckingTypeSpelling;
> -    if (shouldMarkGrammar)
> -        checkingTypes |= TextCheckingTypeGrammar;
> -    if (shouldCheckForCorrection)
> -        checkingTypes |= TextCheckingTypeCorrection;
> -
> +    TextCheckingTypes result = types;

This change is problematic at least on Mac. In original code, TextCheckingTypeCorrection will be set if ShowCorrectionPanel is set in textCheckingOptions. This is not the case in new code. The end result is that, if types.isCorrectionPanel() is true, but types.isCorrection() is false, the spell checker will not check for correction. I suggest to add code here to call setCorrection() if types.isCorrectionPanel() is true.

[Hajime Morrita]

Created attachment 90281 [details]
Patch

[Hajime Morrita]

Hi Jia, thank you for your feedback! 
I updated to patch to address your point.
I confused the relationship between checking and showing the correction.
 

(In reply to comment #15)
> (From update of attachment 90210 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90210&action=review
> 
> > Source/WebCore/editing/Editor.cpp:3249
> > -    bool shouldMarkSpelling = textCheckingOptions & MarkSpelling;
> > -    bool shouldMarkGrammar = textCheckingOptions & MarkGrammar;
> > -    bool shouldShowCorrectionPanel = textCheckingOptions & ShowCorrectionPanel;
> > -    bool shouldCheckForCorrection = shouldShowCorrectionPanel || (textCheckingOptions & CheckForCorrection);
> > -
> > -    TextCheckingTypeMask checkingTypes = 0;
> > -    if (shouldMarkSpelling)
> > -        checkingTypes |= TextCheckingTypeSpelling;
> > -    if (shouldMarkGrammar)
> > -        checkingTypes |= TextCheckingTypeGrammar;
> > -    if (shouldCheckForCorrection)
> > -        checkingTypes |= TextCheckingTypeCorrection;
> > -
> > +    TextCheckingTypes result = types;
> 
> This change is problematic at least on Mac. In original code, TextCheckingTypeCorrection will be set if ShowCorrectionPanel is set in textCheckingOptions. This is not the case in new code. The end result is that, if types.isCorrectionPanel() is true, but types.isCorrection() is false, the spell checker will not check for correction. I suggest to add code here to call setCorrection() if types.isCorrectionPanel() is true.

[Jia Pu]

(In reply to comment #16)
> Created an attachment (id=90281) [details]
> Patch

Thanks for fixing this. This patch looks good to me.

[Ryosuke Niwa]

Comment on attachment 90281 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:1981
> +                                 && (isAutomaticQuoteSubstitutionEnabled()

I don't think we do these deep indentation.  Ditto about other places.

> Source/WebCore/editing/Editor.cpp:3259
> +    if (types.isReplacement()) {
> +        result.setLink(isAutomaticLinkDetectionEnabled());
> +        result.setQuote(isAutomaticQuoteSubstitutionEnabled());
> +        result.setDash(isAutomaticDashSubstitutionEnabled());
> +        result.setReplacement(isAutomaticTextReplacementEnabled());
> +        result.setCorrection(types.isSpelling() && isAutomaticSpellingCorrectionEnabled());
>      }

It seems silly that we have to copy flags manually like this.  Can't we make this function a member of TextCheckingTypes and use bitwise operations?  Also, it's hard to verify that adding Correction to TypeCheckingTypes doesn't regress anything.  Could you explain briefly why it's okay?

> Source/WebCore/platform/text/TextChecking.h:54
> +        Correction  = 1 << 9,

This is a new value.  I can't tell easily from the patch whether this is correct or not.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:860
> +static unsigned toNSTypes(const TextCheckingTypes& types)

Don't they normally name these kinds of functions kit?

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebEditorClientMac.mm:241
> +static unsigned toNSTypes(const TextCheckingTypes& types)

Ditto.

[Hajime Morrita]

Created attachment 93265 [details]
Patch

[Hajime Morrita]

Comment on attachment 90281 [details]
Patch

Hi Ryosuke, 
I'm sorry for my slow response. I'd like to come back this now.

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

>> Source/WebCore/editing/Editor.cpp:1981
>> +                                 && (isAutomaticQuoteSubstitutionEnabled()
> 
> I don't think we do these deep indentation.  Ditto about other places.

Fixed.

>> Source/WebCore/editing/Editor.cpp:3259
>>      }
> 
> It seems silly that we have to copy flags manually like this.  Can't we make this function a member of TextCheckingTypes and use bitwise operations?  Also, it's hard to verify that adding Correction to TypeCheckingTypes doesn't regress anything.  Could you explain briefly why it's okay?

No, we cannot. because flags source is Editor class (or Settings class) and there is no grouping mechanism for these flag on settings side.
Actually this code is what old code was doing.

>> Source/WebCore/platform/text/TextChecking.h:54
>> +        Correction  = 1 << 9,
> 
> This is a new value.  I can't tell easily from the patch whether this is correct or not.

Actually, what new is "TextCheckingTypes::CorrectionPanel" "TextCheckingTypes::Correction" has associated old enum value
TextCheckingTypeCorrection. The value TextCheckingTypes::Correction panel came from Editor:: ShowCorrectionPanel.

The goal of this patch is unifying TextCheckingType, which is for carring result type,
and Editor::TextCheckingOptionFlags, which is for specifying request type, into TextCheckingTypes.
because the results basically contain what the caller requested.

So code paths where originally TextCheckingType don't need to care about CorrectionPanel.
The code where originally used Editor:: ShowCorrectionPanel should replace them with  TextCheckingTypes::CorrectionPanel.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:861
> +{

That's right. Renamed.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebEditorClientMac.mm:242
> +{

Done

[Darin Adler]

Comment on attachment 93265 [details]
Patch

Is there any way to do this in more smaller steps?

[Hajime Morrita]

(In reply to comment #22)
> (From update of attachment 93265 [details])
> Is there any way to do this in more smaller steps?
I see. I'll try to use plain enum instead of a new class,
and concentrate to eliminate Editor::TextCheckingOptions at first.
Thanks for your feedback!

[Hajime Morrita]

Created attachment 93437 [details]
Patch

[Hajime Morrita]

Darin is right.
Now the change becomes small and obvious.

[Ryosuke Niwa]

Comment on attachment 93437 [details]
Patch

LGTM. this patch is so much easier to review!

[Hajime Morrita]

Committed r86520: <http://trac.webkit.org/changeset/86520>