Bug 57088

Summary: Refactoring: Editor::TextCheckingOptions should be replaced with TextCheckingType
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: HTML EditingAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: dbates, dglazkov, jiapu.mail, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 55571, 56085    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Updated to ToT
none
Updated to ToT
none
A build fix
none
Patch
none
Patch
none
Patch rniwa: review+

Description Hajime Morrita 2011-03-25 00:18:29 PDT
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.
Comment 1 Hajime Morrita 2011-04-06 19:49:04 PDT
Created attachment 88559 [details]
Patch
Comment 2 WebKit Review Bot 2011-04-06 20:09:57 PDT
Attachment 88559 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8346447
Comment 3 Hajime Morrita 2011-04-06 20:19:12 PDT
Created attachment 88562 [details]
Patch
Comment 4 WebKit Review Bot 2011-04-06 20:47:04 PDT
Attachment 88562 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8348427
Comment 5 Ryosuke Niwa 2011-04-07 03:51:28 PDT
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.
Comment 6 WebKit Review Bot 2011-04-07 06:38:02 PDT
Attachment 88562 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8341639
Comment 7 Hajime Morrita 2011-04-07 11:22:19 PDT
Created attachment 88665 [details]
Patch
Comment 8 Hajime Morrita 2011-04-07 11:23:05 PDT
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.
Comment 9 Hajime Morrita 2011-04-11 12:15:42 PDT
Created attachment 89049 [details]
Updated to ToT
Comment 10 Hajime Morrita 2011-04-15 16:18:45 PDT
Created attachment 89871 [details]
Updated to ToT
Comment 11 WebKit Review Bot 2011-04-15 20:07:41 PDT
Attachment 89871 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8457309
Comment 12 Daniel Bates 2011-04-15 21:10:16 PDT
Attachment 89871 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8436375
Comment 13 Hajime Morrita 2011-04-19 09:08:17 PDT
Created attachment 90210 [details]
A build fix
Comment 14 Hajime Morrita 2011-04-19 13:01:43 PDT
Build passed. It's ready to review!
Comment 15 Jia Pu 2011-04-19 15:44:11 PDT
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.
Comment 16 Hajime Morrita 2011-04-19 17:48:50 PDT
Created attachment 90281 [details]
Patch
Comment 17 Hajime Morrita 2011-04-19 17:50:52 PDT
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.
Comment 18 Jia Pu 2011-04-20 12:17:07 PDT
(In reply to comment #16)
> Created an attachment (id=90281) [details]
> Patch

Thanks for fixing this. This patch looks good to me.
Comment 19 Ryosuke Niwa 2011-05-03 09:35:59 PDT
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.
Comment 20 Hajime Morrita 2011-05-12 01:21:46 PDT
Created attachment 93265 [details]
Patch
Comment 21 Hajime Morrita 2011-05-12 01:22:38 PDT
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
Comment 22 Darin Adler 2011-05-12 09:56:25 PDT
Comment on attachment 93265 [details]
Patch

Is there any way to do this in more smaller steps?
Comment 23 Hajime Morrita 2011-05-12 19:53:28 PDT
(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!
Comment 24 Hajime Morrita 2011-05-13 05:22:20 PDT
Created attachment 93437 [details]
Patch
Comment 25 Hajime Morrita 2011-05-13 05:23:34 PDT
Darin is right.
Now the change becomes small and obvious.
Comment 26 Ryosuke Niwa 2011-05-13 09:47:54 PDT
Comment on attachment 93437 [details]
Patch

LGTM. this patch is so much easier to review!
Comment 27 Hajime Morrita 2011-05-15 19:46:31 PDT
Committed r86520: <http://trac.webkit.org/changeset/86520>