Bug 55571 – [Refactoring] Auto correction panel should be handled by its own class.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Hajime Morrita]

As code inside SUPPORT_AUTOCORRECTION_PANEL grows,
Editor code is hard to edit without breaking the autocorrection-enabled build.
I think it would be great if these code and its ifdef is under separate class namely
AutoCorrectionPanelController or such. And I'm thinking to do that refactoring.

Jia, what do you think about that? Is it OK to do it now?

[Hajime Morrita]

Created attachment 85922 [details]
Patch

[Hajime Morrita]

This can be compiled with SUPPORT_AUTOCORRECTION_PANEL=1. 
(with small modification on WebKit/mac layer for hiding missing API.)

For me, localizing correction panel related code into single class helps simplifying Editor code and 
will make coming work easier.

[Early Warning System Bot]

Attachment 85922 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8181966

[WebKit Review Bot]

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

[Build Bot]

Attachment 85922 [details] did not build on win:
Build output: http://queues.webkit.org/results/8156016

[WebKit Review Bot]

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

[Jia Pu]

(In reply to comment #0)
> As code inside SUPPORT_AUTOCORRECTION_PANEL grows,
> Editor code is hard to edit without breaking the autocorrection-enabled build.
> I think it would be great if these code and its ifdef is under separate class namely
> AutoCorrectionPanelController or such. And I'm thinking to do that refactoring.
> 
> Jia, what do you think about that? Is it OK to do it now?

Morita, I appreciate you doing this refactoring. I will try to have a look at your patch in a day or two. I'd like to apply the patch myself and run a few tests.

[Ryosuke Niwa]

Comment on attachment 85922 [details]
Patch

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

> Source/WebCore/editing/CorrectionPanelController.cpp:81
> +CorrectionPanelController::~CorrectionPanelController()
> +{
> +#if SUPPORT_AUTOCORRECTION_PANEL
> +    dismissCorrectionPanel(ReasonForDismissingCorrectionPanelIgnored);
> +#endif
> +}

It might be better to have EmptyCorrectionPanelController so that we don't have to put if-endif in all of these functions.

> Source/WebCore/editing/Editor.cpp:-2298
> -#if SUPPORT_AUTOCORRECTION_PANEL
>      // If this checking is only for showing correction panel, we shouldn't bother to mark misspellings.
>      if (shouldShowCorrectionPanel)
>          shouldMarkSpelling = false;
> -#endif

Why is it safe to remove this if-endif?

> Source/WebCore/editing/Editor.cpp:-3521
> -#if SUPPORT_AUTOCORRECTION_PANEL
>          // Don't check spelling and grammar if the change of selection is triggered by spelling correction itself.
>          shouldCheckSpellingAndGrammar = !(options & SelectionController::SpellCorrectionTriggered);
> -#endif

Ditto.

[Jia Pu]

Comment on attachment 85922 [details]
Patch

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

> Source/WebCore/editing/CorrectionPanelController.h:72
> +class CorrectionPanelController {

We probably should call it SpellingCorrectionController, since it does more than just managing the panel UI. For instance, it also monitor undone autocorrection, and manager markers accordingly.

> Source/WebCore/editing/CorrectionPanelController.h:85
> +    void unappliedSpellCorrection(const VisibleSelection& selectionOfCorrected, const String& corrected, const String& correction);
> +    void appliedEditing(PassRefPtr<EditCommand>);

Shall we call them respondToUnappliedSpellCorrection() and respondToAppliedEditing()?

> Source/WebCore/editing/Editor.cpp:1061
>  Editor::~Editor()

I suppose we could remove the empty destructor now.

> Source/WebCore/editing/Editor.cpp:2490
> -void Editor::applyCorrectionPanelInfo(const Vector<DocumentMarker::MarkerType>& markerTypesToAdd)
> +void Editor::prepareEditingWord(bool doNotRemoveIfSelectionAtWordBoundary)

Given the new function name, the argument name isn't very clear anymore. The purpose of this function is to remove markers on the word we are about to edit and that we have just edited. The argument indicates whether we want to handle a word if the selection is on word boundary. Maybe we could name it updateMarkersForWordsAffectedByEditing(bool onlyHandleWordsContainingSelection). We could also remove removeSpellAndCorrectionMarkersFromWordsToBeEdited(), since all prepareEditingWord() does is to call removeSpellAndCorrectionMarkersFromWordsToBeEdited().

>> Source/WebCore/editing/Editor.cpp:-3521
>> -#endif
> 
> Ditto.

I'm almost certain we shouldn't remove this #if. It will change the behavior on current Mac OS X.

[Jia Pu]

> 
> >> Source/WebCore/editing/Editor.cpp:-3521
> >> -#endif
> > 
> > Ditto.
> 
> I'm almost certain we shouldn't remove this #if. It will change the behavior on current Mac OS X.

Ah, I take it back. SelectionController::SpellCorrectionTriggered won't get set if SUPPORT_AUTOCORRECTION_PANEL isn't true.

[Hajime Morrita]

Created attachment 86040 [details]
Patch

[Hajime Morrita]

Ryoske, Jia, 
Thank you for quick feedback! 
I updated the patch to address that.

(In reply to comment #8)
> (From update of attachment 85922 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85922&action=review
> 
> > Source/WebCore/editing/CorrectionPanelController.cpp:81
> > +CorrectionPanelController::~CorrectionPanelController()
> > +{
> > +#if SUPPORT_AUTOCORRECTION_PANEL
> > +    dismissCorrectionPanel(ReasonForDismissingCorrectionPanelIgnored);
> > +#endif
> > +}
> 
> It might be better to have EmptyCorrectionPanelController so that we don't have to put if-endif in all of these functions.
Makes sense, On the other hand, introducing virtual controller interface doesn't look usual in WebCore.
So I change it to return NULL if auto-correction is not enabled
(see SpellingCorrectioController::create()). 
But I'm not sure which is the wright way. What do you think?

> > Source/WebCore/editing/Editor.cpp:-2298
> > -#if SUPPORT_AUTOCORRECTION_PANEL
> >      // If this checking is only for showing correction panel, we shouldn't bother to mark misspellings.
> >      if (shouldShowCorrectionPanel)
> >          shouldMarkSpelling = false;
> > -#endif
> 
> Why is it safe to remove this if-endif?
> 
shouldShowCorrectionPanel never goes true unless SUPPORT_AUTOCORRECTION_PANEL.

> > Source/WebCore/editing/Editor.cpp:-3521
> > -#if SUPPORT_AUTOCORRECTION_PANEL
> >          // Don't check spelling and grammar if the change of selection is triggered by spelling correction itself.
> >          shouldCheckSpellingAndGrammar = !(options & SelectionController::SpellCorrectionTriggered);
> > -#endif
> 
> Ditto.
SelectionController::SpellCorrectionTriggered is only passed from SpellingCorrectionControlller.
thus only available when SUPPORT_AUTOCORRECTION_PANEL is given.

----

(In reply to comment #9)
> (From update of attachment 85922 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85922&action=review
> 
> > Source/WebCore/editing/CorrectionPanelController.h:72
> > +class CorrectionPanelController {
> 
> We probably should call it SpellingCorrectionController, since it does more than just managing the panel UI. For instance, it also monitor undone autocorrection, and manager markers accordingly.
> 
It makes sense. I renamed it.

> > Source/WebCore/editing/CorrectionPanelController.h:85
> > +    void unappliedSpellCorrection(const VisibleSelection& selectionOfCorrected, const String& corrected, const String& correction);
> > +    void appliedEditing(PassRefPtr<EditCommand>);
> 
> Shall we call them respondToUnappliedSpellCorrection() and respondToAppliedEditing()?
Sure. Renamed.

> 
> > Source/WebCore/editing/Editor.cpp:1061
> >  Editor::~Editor()
> 
> I suppose we could remove the empty destructor now.
We need this because we moved #include for the controller to Editor.cpp 
to save the compilation time. (The destructor need to be on Editor.cpp in this case.)

> 
> > Source/WebCore/editing/Editor.cpp:2490
> > -void Editor::applyCorrectionPanelInfo(const Vector<DocumentMarker::MarkerType>& markerTypesToAdd)
> > +void Editor::prepareEditingWord(bool doNotRemoveIfSelectionAtWordBoundary)
> 
> Given the new function name, the argument name isn't very clear anymore. The purpose of this function is to remove markers on the word we are about to edit and that we have just edited. The argument indicates whether we want to handle a word if the selection is on word boundary. Maybe we could name it updateMarkersForWordsAffectedByEditing(bool onlyHandleWordsContainingSelection). We could also remove removeSpellAndCorrectionMarkersFromWordsToBeEdited(), since all prepareEditingWord() does is to call removeSpellAndCorrectionMarkersFromWordsToBeEdited().
> 
Renamed and made the two functions into one.

[Early Warning System Bot]

Attachment 86040 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8195052

[WebKit Review Bot]

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

[Ryosuke Niwa]

Comment on attachment 86040 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:1058
> +    , m_spellingCorrector(SpellingCorrectionController::create(frame))

Can we have if-def here instead?  It seems odd that create() returns 0 if correction controller is not supported.

[Ryosuke Niwa]

Comment on attachment 86040 [details]
Patch

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

>> Source/WebCore/editing/Editor.cpp:1058
>> +    , m_spellingCorrector(SpellingCorrectionController::create(frame))
> 
> Can we have if-def here instead?  It seems odd that create() returns 0 if correction controller is not supported.

On second thought, maybe it's better to have all if-defs in one file.  I'd like to hear other contributors' opinions here.

> Source/WebCore/editing/SpellingCorrectionController.cpp:78
> +SpellingCorrectionController* SpellingCorrectionController::create(Frame* frame)
> +{
> +#if SUPPORT_AUTOCORRECTION_PANEL
> +    return new SpellingCorrectionController(frame);
> +#else
> +    UNUSED_PARAM(frame);
> +    return 0;
> +#endif
> +}

This function should return PassOwnPtr and you should be calling adoptPtr(new SpellingCorrectionController(frame)).  I'm still not happy about the fact all these member functions will be linked in non-Lion WebKit even though they're never called in practice because they are referenced in Editor.cpp.  You should really consider adding EmptySpellingCorrectionController or come up with a clever solution to avoid this.

[Build Bot]

Attachment 86040 [details] did not build on win:
Build output: http://queues.webkit.org/results/8191493

[Hajime Morrita]

Created attachment 86044 [details]
Patch

[Hajime Morrita]

This is just for fixing build break ...

[WebKit Review Bot]

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

[Build Bot]

Attachment 86044 [details] did not build on win:
Build output: http://queues.webkit.org/results/8189605

[Hajime Morrita]

Created attachment 86416 [details]
Patch

[Hajime Morrita]

> I'm still not happy about the fact all these member functions will be linked in non-Lion WebKit even though they're never called in practice because they are referenced in Editor.cpp.  You should really consider adding EmptySpellingCorrectionController or come up with a clever solution to avoid this.

Well, I just returned back to the if/endif way. 
Instead of scattering if/endif blocks, This change embrace all codebase in single block 
at SpellingCorrectionController.cpp , and put empty implementations into the else/endif part.
I gave up EmptySpellingCorrectionController because runtime polymorphism doesn't help us 
from build breakages unless the alternative code path is compiled.

[Ryosuke Niwa]

Comment on attachment 86416 [details]
Patch

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

> Source/WebCore/editing/SpellingCorrectionController.cpp:456
> +#else
> +
> +SpellingCorrectionController::SpellingCorrectionController(Frame*) {}
> +SpellingCorrectionController::~SpellingCorrectionController() {}
> +void SpellingCorrectionController::startCorrectionPanelTimer(CorrectionPanelInfo::PanelType) {}
> +void SpellingCorrectionController::stopCorrectionPanelTimer() {}
> +void SpellingCorrectionController::dismiss(ReasonForDismissingCorrectionPanel) {}
> +void SpellingCorrectionController::show(PassRefPtr<Range>, const String&) {}
> +void SpellingCorrectionController::applyCorrectionPanelInfo(const Vector<DocumentMarker::MarkerType>&) {}
> +bool SpellingCorrectionController::applyAutocorrectionBeforeTypingIfAppropriate() { return false; }
> +void SpellingCorrectionController::respondToUnappliedSpellCorrection(const VisibleSelection&, const String&, const String&) {}
> +void SpellingCorrectionController::respondToAppliedEditing(PassRefPtr<EditCommand>) {}
> +void SpellingCorrectionController::respondToChangedSelection(const VisibleSelection&) {}
> +void SpellingCorrectionController::stopPendingCorrection(const VisibleSelection&) {}
> +void SpellingCorrectionController::applyPendingCorrection(const VisibleSelection&) {}
> +void SpellingCorrectionController::handleCorrectionPanelResult(const String&) {}
> +void SpellingCorrectionController::handleCancelOperation() {}
> +void SpellingCorrectionController::markRevsersed(PassRefPtr<Range>) {}
> +void SpellingCorrectionController::recordAutocorrectionResponseReversed(const String&, PassRefPtr<Range>) {}
> +bool SpellingCorrectionController::hasPendingCorrection() const { return false; }
> +bool SpellingCorrectionController::isSpellingMarkerAllowed(PassRefPtr<Range>) const { return true; }
> +bool SpellingCorrectionController::isShowingCorrectionPanel() { return false; }
> +bool SpellingCorrectionController::isAutomaticSpellingCorrectionEnabled() { return false; }
> +
> +#endif

Can we put this in the header line so that compiler can optimize away the function calls?

[Hajime Morrita]

Created attachment 86419 [details]
Updated to ToT

[WebKit Review Bot]

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

[Build Bot]

Attachment 86419 [details] did not build on win:
Build output: http://queues.webkit.org/results/8222095

[Hajime Morrita]

Created attachment 86423 [details]
Fixing the build break

[Ryosuke Niwa]

Comment on attachment 86423 [details]
Fixing the build break

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25352
> +				A7005CCC1330C4BA000CC0BA /* SpellingCorrectionController.cpp in Sources */,

This one is out of order.

> Source/WebCore/editing/Editor.cpp:1056
> +    , m_spellingCorrector(new SpellingCorrectionController(frame))

We should call adoptPtr here.

> Source/WebCore/editing/Editor.cpp:2417
> +    if (!m_spellingCorrector->isAutomaticSpellingCorrectionEnabled())
>          return;

Why are you adding this line to removeSpellAndCorrectionMarkersFromWordsToBeEdited/updateMarkersForWordsAffectedByEditing?

> Source/WebCore/editing/SpellingCorrectionController.cpp:86
> +    if (isAutomaticSpellingCorrectionEnabled()) {

I know you're just copying but I'd prefer an early exist over a nested if.

> Source/WebCore/editing/SpellingCorrectionController.cpp:89
> +        if (type == CorrectionPanelInfo::PanelTypeCorrection)
> +            // If type is PanelTypeReversion, then the new range has been set. So we shouldn't clear it.
> +            m_correctionPanelInfo.rangeToBeReplaced.clear();

Need curly brackets here since comment + statement occupies 2 lines.

> Source/WebCore/editing/SpellingCorrectionController.cpp:105
> +    if (currentSelection != oldSelection) {

Ditto about early exit.

> Source/WebCore/editing/SpellingCorrectionController.cpp:135
> +    return !(m_frame->document()->markers()->hasMarkers(misspellingRange.get(), DocumentMarker::SpellCheckingExemption));

I don't think you need parenthesis before !

> Source/WebCore/editing/SpellingCorrectionController.cpp:186
> +    RefPtr<Range> correctionStartOffsetInParagraphAsRange = Range::create(paragraphRangeContainingCorrection->startContainer(ec)->document(), paragraphRangeContainingCorrection->startPosition(), paragraphRangeContainingCorrection->startPosition());

Wow! this is a really long statement.  Maybe the variable name is too verbose.

> Source/WebCore/editing/SpellingCorrectionController.cpp:215
> +        if (m_correctionPanelInfo.panelType == CorrectionPanelInfo::PanelTypeReversion)
> +            markers->addMarker(replacementRange.get(), markerTypesToAdd[i]);
> +        else
> +            markers->addMarker(replacementRange.get(), markerTypesToAdd[i], m_correctionPanelInfo.replacedString);

I know you're just copying but it seems silly to check this condition in each iteration.  We should just do:

String description;
if (m_correctionPanelInfo.panelType == CorrectionPanelInfo::PanelTypeReversion)
    description = m_correctionPanelInfo.replacedString;
for (size_t i = 0; i < size; ++i)
    markers->addMarker(replacementRange.get(), markerTypesToAdd[i], description);

Also, I'm surprised to learn that addMarker takes String instead of const String&.  It's probably a bug.

> Source/WebCore/editing/SpellingCorrectionController.cpp:313
> +        } else {
> +            if (!m_correctionPanelIsDismissedByEditor)

It seems like this should just be a single "else if".

> Source/WebCore/editing/SpellingCorrectionController.cpp:365
> +        if (((marker.type == DocumentMarker::Replacement && !marker.description.isNull()) || marker.type == DocumentMarker::Spelling) && static_cast<int>(marker.endOffset) == endOffset) {

This condition seems too long to fit in one line. Maybe split it into two lines or extract an inline function?

> Source/WebCore/editing/SpellingCorrectionController.cpp:383
> +void SpellingCorrectionController::respondToAppliedEditing(PassRefPtr<EditCommand> cmd)

Please spell out "command".

> Source/WebCore/editing/SpellingCorrectionController.cpp:454
> +SpellingCorrectionController::SpellingCorrectionController(Frame*) {}
> +SpellingCorrectionController::~SpellingCorrectionController() {}
> +void SpellingCorrectionController::startCorrectionPanelTimer(CorrectionPanelInfo::PanelType) {}
> +void SpellingCorrectionController::stopCorrectionPanelTimer() {}
> +void SpellingCorrectionController::dismiss(ReasonForDismissingCorrectionPanel) {}
> +void SpellingCorrectionController::show(PassRefPtr<Range>, const String&) {}
> +void SpellingCorrectionController::applyCorrectionPanelInfo(const Vector<DocumentMarker::MarkerType>&) {}
> +bool SpellingCorrectionController::applyAutocorrectionBeforeTypingIfAppropriate() { return false; }
> +void SpellingCorrectionController::respondToUnappliedSpellCorrection(const VisibleSelection&, const String&, const String&) {}
> +void SpellingCorrectionController::respondToAppliedEditing(PassRefPtr<EditCommand>) {}
> +void SpellingCorrectionController::respondToChangedSelection(const VisibleSelection&) {}
> +void SpellingCorrectionController::stopPendingCorrection(const VisibleSelection&) {}
> +void SpellingCorrectionController::applyPendingCorrection(const VisibleSelection&) {}
> +void SpellingCorrectionController::handleCorrectionPanelResult(const String&) {}
> +void SpellingCorrectionController::handleCancelOperation() {}
> +void SpellingCorrectionController::markRevsersed(PassRefPtr<Range>) {}
> +void SpellingCorrectionController::recordAutocorrectionResponseReversed(const String&, PassRefPtr<Range>) {}
> +bool SpellingCorrectionController::hasPendingCorrection() const { return false; }
> +bool SpellingCorrectionController::isSpellingMarkerAllowed(PassRefPtr<Range>) const { return true; }
> +bool SpellingCorrectionController::isShowingCorrectionPanel() { return false; }
> +bool SpellingCorrectionController::isAutomaticSpellingCorrectionEnabled() { return false; }

I still think we should put this in the header to take the advantage of optimizing compilers.

[Jia Pu]

Comment on attachment 86423 [details]
Fixing the build break

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

>> Source/WebCore/editing/SpellingCorrectionController.cpp:186
>> +    RefPtr<Range> correctionStartOffsetInParagraphAsRange = Range::create(paragraphRangeContainingCorrection->startContainer(ec)->document(), paragraphRangeContainingCorrection->startPosition(), paragraphRangeContainingCorrection->startPosition());
> 
> Wow! this is a really long statement.  Maybe the variable name is too verbose.

Huh, now looking back at the code myself, it seems we could shed the "ContainingCorrection" part from "paragraphRangeContainingCorrection".

[Jia Pu]

Morita, I have landed a patch for bug 56055, which is likely to cause some conflict with your latest patch. But none of them is difficult to resolve. I have:
1. removed EditorClient::isShowingCorrectionPanel(), Editor::isShowingCorrectionPanel().
2. dropped Editor* argument in EditorClient::showCorrectionPanel().
3. added EditorClient::dismissCorrectionPanelSoon(), Editor::dismissCorrectionPanelSoon().
4. removed Editor::handleCancelOperation().

[Hajime Morrita]

Hi Jia, thank you kindly for letting me know this.
I'll update my local working copy.
It doesn't look hard to do it, as you mentioned. 

(In reply to comment #31)
> Morita, I have landed a patch for bug 56055, which is likely to cause some conflict with your latest patch. But none of them is difficult to resolve. I have:
> 1. removed EditorClient::isShowingCorrectionPanel(), Editor::isShowingCorrectionPanel().
> 2. dropped Editor* argument in EditorClient::showCorrectionPanel().
> 3. added EditorClient::dismissCorrectionPanelSoon(), Editor::dismissCorrectionPanelSoon().
> 4. removed Editor::handleCancelOperation().

[Hajime Morrita]

Created attachment 86887 [details]
Patch

[WebKit Review Bot]

Attachment 86887 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/editing/SpellingCorrectionController.h:74:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hajime Morrita]

Hi Ryosuke, Jia, thank you for taking care of this.
I update the patch for address your feedback.

(The style error is a false positive. it mis-recognized a macro to a function.)

(In reply to comment #29)
> (From update of attachment 86423 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86423&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:25352
> > +				A7005CCC1330C4BA000CC0BA /* SpellingCorrectionController.cpp in Sources */,
> 
> This one is out of order.
Fixed.

> 
> > Source/WebCore/editing/Editor.cpp:1056
> > +    , m_spellingCorrector(new SpellingCorrectionController(frame))
> 
> We should call adoptPtr here.
Fixed.

> 
> > Source/WebCore/editing/Editor.cpp:2417
> > +    if (!m_spellingCorrector->isAutomaticSpellingCorrectionEnabled())
> >          return;
> 
> Why are you adding this line to removeSpellAndCorrectionMarkersFromWordsToBeEdited/updateMarkersForWordsAffectedByEditing?
> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:86
> > +    if (isAutomaticSpellingCorrectionEnabled()) {
> 
> I know you're just copying but I'd prefer an early exist over a nested if.
Changed to early-exist style.

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:89
> > +        if (type == CorrectionPanelInfo::PanelTypeCorrection)
> > +            // If type is PanelTypeReversion, then the new range has been set. So we shouldn't clear it.
> > +            m_correctionPanelInfo.rangeToBeReplaced.clear();
> 
> Need curly brackets here since comment + statement occupies 2 lines.
Moved comment outside the if block

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:105
> > +    if (currentSelection != oldSelection) {
> 
> Ditto about early exit.
Fixed.

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:135
> > +    return !(m_frame->document()->markers()->hasMarkers(misspellingRange.get(), DocumentMarker::SpellCheckingExemption));
> 
> I don't think you need parenthesis before !
Removed the parenthesis.

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:186
> > +    RefPtr<Range> correctionStartOffsetInParagraphAsRange = Range::create(paragraphRangeContainingCorrection->startContainer(ec)->document(), paragraphRangeContainingCorrection->startPosition(), paragraphRangeContainingCorrection->startPosition());
> 
> Wow! this is a really long statement.  Maybe the variable name is too verbose.
Made some variable names shorter, as Jia suggested.

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:215
> > +        if (m_correctionPanelInfo.panelType == CorrectionPanelInfo::PanelTypeReversion)
> > +            markers->addMarker(replacementRange.get(), markerTypesToAdd[i]);
> > +        else
> > +            markers->addMarker(replacementRange.get(), markerTypesToAdd[i], m_correctionPanelInfo.replacedString);
> 
> I know you're just copying but it seems silly to check this condition in each iteration.  We should just do:
> 
> String description;
> if (m_correctionPanelInfo.panelType == CorrectionPanelInfo::PanelTypeReversion)
>     description = m_correctionPanelInfo.replacedString;
> for (size_t i = 0; i < size; ++i)
>     markers->addMarker(replacementRange.get(), markerTypesToAdd[i], description);
> 
Fixed.

> Also, I'm surprised to learn that addMarker takes String instead of const String&.  It's probably a bug.
This fix will be done by my upcoming DocumentMarkerController refactoring.

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:313
> > +        } else {
> > +            if (!m_correctionPanelIsDismissedByEditor)
> 
> It seems like this should just be a single "else if".
Fixed.

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:365
> > +        if (((marker.type == DocumentMarker::Replacement && !marker.description.isNull()) || marker.type == DocumentMarker::Spelling) && static_cast<int>(marker.endOffset) == endOffset) {
> 
> This condition seems too long to fit in one line. Maybe split it into two lines or extract an inline function?
Divided the line.

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:383
> > +void SpellingCorrectionController::respondToAppliedEditing(PassRefPtr<EditCommand> cmd)
> 
> Please spell out "command".
Done.

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:454
> > +SpellingCorrectionController::SpellingCorrectionController(Frame*) {}
> > +SpellingCorrectionController::~SpellingCorrectionController() {}
> > +void SpellingCorrectionController::startCorrectionPanelTimer(CorrectionPanelInfo::PanelType) {}
> > +void SpellingCorrectionController::stopCorrectionPanelTimer() {}
> > +void SpellingCorrectionController::dismiss(ReasonForDismissingCorrectionPanel) {}
> > +void SpellingCorrectionController::show(PassRefPtr<Range>, const String&) {}
> > +void SpellingCorrectionController::applyCorrectionPanelInfo(const Vector<DocumentMarker::MarkerType>&) {}
> > +bool SpellingCorrectionController::applyAutocorrectionBeforeTypingIfAppropriate() { return false; }
> > +void SpellingCorrectionController::respondToUnappliedSpellCorrection(const VisibleSelection&, const String&, const String&) {}
> > +void SpellingCorrectionController::respondToAppliedEditing(PassRefPtr<EditCommand>) {}
> > +void SpellingCorrectionController::respondToChangedSelection(const VisibleSelection&) {}
> > +void SpellingCorrectionController::stopPendingCorrection(const VisibleSelection&) {}
> > +void SpellingCorrectionController::applyPendingCorrection(const VisibleSelection&) {}
> > +void SpellingCorrectionController::handleCorrectionPanelResult(const String&) {}
> > +void SpellingCorrectionController::handleCancelOperation() {}
> > +void SpellingCorrectionController::markRevsersed(PassRefPtr<Range>) {}
> > +void SpellingCorrectionController::recordAutocorrectionResponseReversed(const String&, PassRefPtr<Range>) {}
> > +bool SpellingCorrectionController::hasPendingCorrection() const { return false; }
> > +bool SpellingCorrectionController::isSpellingMarkerAllowed(PassRefPtr<Range>) const { return true; }
> > +bool SpellingCorrectionController::isShowingCorrectionPanel() { return false; }
> > +bool SpellingCorrectionController::isAutomaticSpellingCorrectionEnabled() { return false; }
> 
> I still think we should put this in the header to take the advantage of optimizing compilers.
Tried to put this inline. 
Please see SpellingCorrectionController.h.
Although it might not be a typical webkit way, I think it's worth to do it.

[Build Bot]

Attachment 86887 [details] did not build on win:
Build output: http://queues.webkit.org/results/8233935

[WebKit Review Bot]

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

[Ryosuke Niwa]

Could fix the build?

[Hajime Morrita]

Created attachment 88469 [details]
Patch

[Hajime Morrita]

> Could fix the build?
I'm sorry for my slow response.
I just updated the patch to fix build break and catchup ToT.

[WebKit Review Bot]

Attachment 88469 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebCore/editing/SpellingCorrectionController.h:75:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Attachment 88469 [details] did not build on win:
Build output: http://queues.webkit.org/results/8344336

[Hajime Morrita]

Created attachment 88504 [details]
Patch

[WebKit Review Bot]

Attachment 88504 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebCore/editing/SpellingCorrectionController.h:75:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hajime Morrita]

Created attachment 88648 [details]
Updated to Tot

[WebKit Review Bot]

Attachment 88648 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/editing/SpellingCorrectionController.h:75:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jia Pu]

In both WebKit and WK2, we need to replace <WebCore/CorrectionPanelInfo.h> with <WebCore/SpellingCorrectionController.h>. Also, this patch seems change the behavior of undoing autocorrection on lion. I will take a look.

[Hajime Morrita]

Created attachment 88686 [details]
Patch

[Hajime Morrita]

Hi Jia, thanks much for your corporation!

> In both WebKit and WK2, we need to replace <WebCore/CorrectionPanelInfo.h> with <WebCore/SpellingCorrectionController.h>. Also, this patch seems change the behavior of undoing autocorrection on lion. I will take a look.
I fixed inclusion lines, and some outdated/wrong part of the patch.
Could you try updated one when you have time?

[WebKit Review Bot]

Attachment 88686 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/editing/SpellingCorrectionController.h:75:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jia Pu]

Comment on attachment 88686 [details]
Patch

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

> Source/WebCore/editing/SpellingCorrectionController.cpp:159
> +void SpellingCorrectionController::show(PassRefPtr<Range> rangeToReplace, const String& replacement)
> +{
> +    FloatRect boundingBox = windowRectForRange(rangeToReplace.get());
> +    if (boundingBox.isEmpty())
> +        return;
> +    m_correctionPanelInfo.rangeToBeReplaced = rangeToReplace;

After this assignment, rangeToReplace has a null pointer.

> Source/WebCore/editing/SpellingCorrectionController.cpp:160
> +    m_correctionPanelInfo.replacedString = plainText(rangeToReplace.get());

Now the return value of plainText() will be an empty string.

[Darin Adler]

Comment on attachment 88686 [details]
Patch

Need a new patch with the problem Jia Pu pointed out fixed.

[Hajime Morrita]

Created attachment 88728 [details]
Patch

[Hajime Morrita]

Jia, Darin, thank you for your responsive investigation!
I updated the patch, Could you have  another try ?
Thanks in advance.

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:159
> > +void SpellingCorrectionController::show(PassRefPtr<Range> rangeToReplace, const String& replacement)
> > +{
> > +    FloatRect boundingBox = windowRectForRange(rangeToReplace.get());
> > +    if (boundingBox.isEmpty())
> > +        return;
> > +    m_correctionPanelInfo.rangeToBeReplaced = rangeToReplace;
> 
> After this assignment, rangeToReplace has a null pointer.
I trapped PassRefPtr trap... Fixed.

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:160
> > +    m_correctionPanelInfo.replacedString = plainText(rangeToReplace.get());
> 
> Now the return value of plainText() will be an empty string.
Fixed by above change.

[Jia Pu]

(In reply to comment #54)
> Jia, Darin, thank you for your responsive investigation!
> I updated the patch, Could you have  another try ?

There's one remaining issue related to undoing autocorrection. I'm trying to figure out.

[WebKit Review Bot]

Attachment 88728 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/editing/SpellingCorrectionController.h:75:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jia Pu]

Comment on attachment 88728 [details]
Patch

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

> Source/WebCore/editing/SpellingCorrectionController.cpp:363
> +bool SpellingCorrectionController::shouldRemoveMarkersUponEditing()
> +{
> +    return REMOVE_MARKERS_UPON_EDITING && m_correctionPanelInfo.isActive;

On platforms where REMOVE_MARKERS_UPON_EDITING is true, if an editing action changes a word that bears a spelling, grammar or autocorrection marker, we always want to remove it. It is not related to whether the correction panel is active. So this function should return true as long as REMOVE_MARKERS_UPON_EDITING is true.

[Hajime Morrita]

Created attachment 88745 [details]
Patch

[Hajime Morrita]

> On platforms where REMOVE_MARKERS_UPON_EDITING is true, if an editing action changes a word that bears a spelling, grammar or autocorrection marker, we always want to remove it. It is not related to whether the correction panel is active. So this function should return true as long as REMOVE_MARKERS_UPON_EDITING is true.
Hi Jia, Thanks for your patient debugging!
I just fixed the pointed problem, praying this works...

[WebKit Review Bot]

Attachment 88745 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/editing/SpellingCorrectionController.h:75:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jia Pu]

Comment on attachment 88745 [details]
Patch

This latest patch seems fine on my part. Thanks, Morita.

[Darin Adler]

Comment on attachment 88745 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:-3546
>          bool shouldCheckSpellingAndGrammar = true;
> -#if SUPPORT_AUTOCORRECTION_PANEL
>          // Don't check spelling and grammar if the change of selection is triggered by spelling correction itself.
>          shouldCheckSpellingAndGrammar = !(options & SelectionController::SpellCorrectionTriggered);
> -#endif

Since we are removing this #if, we should combine the definition and initialization into a single line.

> Source/WebCore/editing/SpellingCorrectionController.h:77
> +#define UNLESS_ENABLED(functionBody) functionBody

I think you should just put the braces here in the macro so we can use ( ) instead of ({ }) around all those function bodies.

> Source/WebCore/editing/SpellingCorrectionController.h:80
> +class SpellingCorrectionController {

This class should be marked noncopyable.

> Source/WebCore/editing/SpellingCorrectionController.h:83
> +    SpellingCorrectionController(Frame*) UNLESS_ENABLED({})
> +    ~SpellingCorrectionController() UNLESS_ENABLED({})

This technique with all the macros on each function seems a little ugly to me, but I have no better suggestion.

[Hajime Morrita]

Hi Darin, thank you for reviewing!
I'll land this after addressing you feedback.

> > Source/WebCore/editing/SpellingCorrectionController.h:83
> > +    SpellingCorrectionController(Frame*) UNLESS_ENABLED({})
> > +    ~SpellingCorrectionController() UNLESS_ENABLED({})
> 
> This technique with all the macros on each function seems a little ugly to me, but I have no better suggestion.
I have same impression. One possible idea is to provide empty abstract class.
But it will have some performance penalty and I just gave it up at this time.

[Hajime Morrita]

Committed r83344: <http://trac.webkit.org/changeset/83344>

[Adam Roben (:aroben)]

This has broken Windows and Leopard so far. Not sure why it didn't break Win EWS.

[WebKit Review Bot]

http://trac.webkit.org/changeset/83344 might have broken Windows Release (Build), Windows Debug (Build), and WinCE Release (Build)

[Jessie Berlin]

(In reply to comment #64)
> Committed r83344: <http://trac.webkit.org/changeset/83344>

Why is the UNLESS_ENABLED macro ok? Why should we be compiling this code with empty stubs for features that are not enabled? Why not compile it out instead of potentially adding runtime overhead and introducing subtle bugs because someone calls something that is not enabled and it doesn't fail at compile-time?

[Hajime Morrita]

> Why is the UNLESS_ENABLED macro ok? Why should we be compiling this code with empty stubs for features that are not enabled? Why not compile it out instead of potentially adding runtime overhead and introducing subtle bugs because someone calls something that is not enabled and it doesn't fail at compile-time?
So how about to use caller side macro like RUN_IF_AUTOCORRECTION_ENABLED() ?
That would be:
IF_AUTOCORRECTION_ENABLED(m_spellingCorrector->someMethod());

The point is to minimize the number of ifdefs. 
#if/#endif pairs occupy two additional lines, it's makes reading/changing code really hard.
In editor, there is so meny ifdefs. I don't want any more.

This is important especially for Editor because editor-related ifdefs are large number of small chunks,
that is different from typical flags like SVG, which has large, smal number of blocks.

For more long term goal, I'd like to factor the Editor class into more meaningful group of classes
like spell checking, command execution, text composition, etc.
This is a part of such refactoring.