Bug 57088

Created attachment 88559 [details] Patch

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

Created attachment 88562 [details] Patch

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

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.

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

Created attachment 88665 [details] Patch

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.

Created attachment 89049 [details] Updated to ToT

Created attachment 89871 [details] Updated to ToT

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

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

Created attachment 90210 [details] A build fix

Build passed. It's ready to review!

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.

Created attachment 90281 [details] Patch

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.

(In reply to comment #16) > Created an attachment (id=90281) [details] > Patch Thanks for fixing this. This patch looks good to me.

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.

Created attachment 93265 [details] Patch

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 on attachment 93265 [details] Patch Is there any way to do this in more smaller steps?

(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!

Created attachment 93437 [details] Patch

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

Comment on attachment 93437 [details] Patch LGTM. this patch is so much easier to review!

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