Bug 56085 – [Refactoring] SpellCheckingResult should be replaced with TextCheckingResult

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Hajime Morrita]

These two classes are almost identical so we should kill younger one.

[Hajime Morrita]

Created attachment 85454 [details]
Patch

[Hajime Morrita]

Ojan, Ryosuke, could you take a look?
My last change was just wrong and I'd like to fix it...

[Collabora GTK+ EWS bot]

Attachment 85454 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8135245

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[Build Bot]

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

[Hajime Morrita]

Created attachment 85643 [details]
Patch

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[Build Bot]

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

[Hajime Morrita]

Created attachment 85645 [details]
Patch

[WebKit Review Bot]

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

[Hajime Morrita]

Created attachment 85649 [details]
Yet another attempt to build fix

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Hajime Morrita]

Created attachment 85659 [details]
fix v3

[WebKit Review Bot]

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

[Hajime Morrita]

Created attachment 85662 [details]
fix v4

[Ryosuke Niwa]

Comment on attachment 85662 [details]
fix v4

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

> LayoutTests/editing/spelling/spellcheck-paste-grammar.html:9
> +<p id="description"></p>

We need a description here or otherwise we'll have no idea what this test is testing.

> LayoutTests/editing/spelling/spellcheck-paste-grammar.html:17
> +layoutTestController.setAsynchronousSpellCheckingEnabled(true);
> +layoutTestController.waitUntilDone();

We should wrap this inside an if clause so that we don't get console errors.

> LayoutTests/platform/gtk/Skipped:302
> +

Nit: do we need an extra line here?

> Source/WebCore/ChangeLog:10
> +        This change revealed that TextCheckerClient::requestCheckingOfString() should have
> +        additional request type parameter thus added it.

You should elaborate more on why behavior change was needed and what kind behavior change we observe after this patch.

> Source/WebCore/editing/Editor.cpp:3632
> +    bool shouldMarkSpelling = textCheckingOptions & MarkSpelling;
> +    bool shouldMarkGrammar = textCheckingOptions & MarkGrammar;
> +    bool shouldShowCorrectionPanel = textCheckingOptions & ShowCorrectionPanel;

It's not great that there is exact replica of this code in Editor::markAllMisspellingsAndBadGrammarInRanges.

> Source/WebCore/editing/SpellChecker.cpp:106
> +    // Currently we only supports spelling and grammar

Nit: "we only supportS" should be "we only support".

> Source/WebCore/editing/SpellChecker.cpp:169
> +        // Currently we only supports spelling and grammar

Nit: supportS

> Source/WebCore/editing/SpellChecker.cpp:195
> +        if (results[i].type & TextCheckingTypeSpelling) {
> +            if (!markAt(start, results[i].location, results[i].length, DocumentMarker::Spelling, String()))
> +                break;
> +        }
> +
> +        if (results[i].type & TextCheckingTypeGrammar) {
> +            for (size_t j = 0; j < results[i].details.size(); ++j) {
> +                PositionIterator grammarStart = start;
> +                if (!forwardIterator(grammarStart, results[i].details[j].location - startOffset))
> +                    break;
> +                
> +                PositionIterator grammarEnd = grammarStart;
> +                if (!forwardIterator(grammarEnd, results[i].details[j].length))
> +                    break;
> +                if (!markAt(grammarStart, results[i].details[j].location, results[i].details[j].length, DocumentMarker::Grammar, results[i].details[j].userDescription))
> +                    break;
> +            }

Why are we adding this code?  Does this change behavior?  If so, it's not great that code change like this is checked in with a big refactoring.  Ideally, we'll split this into two patches.  r- due to the insufficient description of this change.

> Source/WebCore/platform/text/TextCheckerClient.h:56
> +typedef uint64_t TextCheckingTypeMask;

Why are we typedef-ing uint64_t as TextCheckingTypeMask?  We should just use TextCheckingType instead.  See NodeFlags in Node.h or simply search "1 << 2" for examples of this usage of enum.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:798
> +static void toCoreTextCheckingResults(NSArray *incomingResults, TextCheckingTypeMask checkingTypes, Vector<TextCheckingResult>& results)

Mn... is it really Mac port's practice to have these "toCore" functions?

> Tools/DumpRenderTree/LayoutTestController.cpp:2142
> +        { "markersForSelectionStartAsText", markersForSelectionStartAsTextCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },

I'm not sure if this is a descriptive name.  Also, is there a plan or a possibility of implementing non-text version of this function?  If not, "asText" is redundant given that JavaScript is dynamically typed.

[Hajime Morrita]

Created attachment 85791 [details]
Patch

[Hajime Morrita]

Hi Niwa-san, thank you for your review!
I updated the patch to address the feedback.

At first, I split out the behavioral change to Bug 56368, LayoutTest and
DRT change should also go there.
Other comments are inline:

> 
> > Source/WebCore/editing/Editor.cpp:3632
> > +    bool shouldMarkSpelling = textCheckingOptions & MarkSpelling;
> > +    bool shouldMarkGrammar = textCheckingOptions & MarkGrammar;
> > +    bool shouldShowCorrectionPanel = textCheckingOptions & ShowCorrectionPanel;
> 
> It's not great that there is exact replica of this code in Editor::markAllMisspellingsAndBadGrammarInRanges.
This is a bit tricky. I'd like to handle this on Bug 56369, in which I'm going to introduce
bit-field based struct for representing text checking flags.

> Why are we typedef-ing uint64_t as TextCheckingTypeMask?  We should just use TextCheckingType instead.  See NodeFlags in Node.h or simply search "1 << 2" for examples of this usage of enum.
> 
I'd like to handle this at Bug 56369.
Even until that, I think named typedef is better than opaque int64_t.

> > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:798
> > +static void toCoreTextCheckingResults(NSArray *incomingResults, TextCheckingTypeMask checkingTypes, Vector<TextCheckingResult>& results)
> 
> Mn... is it really Mac port's practice to have these "toCore" functions?
> 
Good point. I renamed it to core().

[Ryosuke Niwa]

Comment on attachment 85791 [details]
Patch

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

Okay, the changes look sane to me.  I have a few questions before r+ing this patch though.

> Source/WebCore/platform/text/TextCheckerClient.h:56
> +typedef uint64_t TextCheckingTypeMask;

Do we really need 64-bit for this?  Can't we just use unsigned integer here?

> Source/WebKit/chromium/src/EditorClientImpl.cpp:874
> -void EditorClientImpl::requestCheckingOfString(SpellChecker* sender, int identifier, const String& text)
> +void EditorClientImpl::requestCheckingOfString(WebCore::SpellChecker* sender, int identifier, WebCore::TextCheckingTypeMask, const WTF::String& text)

Mn... why do we need to use WebCore::SpellChecker instead of SpellChecker now?

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1053
> +    _sender->didCheck(_sequence, core(_results.get(), _types));

Where is this core defined?

[Hajime Morrita]

Created attachment 85901 [details]
Patch

[Hajime Morrita]

Hi Ryosuke, thank you for review again!

(In reply to comment #22)
> (From update of attachment 85791 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85791&action=review
> 
> Okay, the changes look sane to me.  I have a few questions before r+ing this patch though.
> 
> > Source/WebCore/platform/text/TextCheckerClient.h:56
> > +typedef uint64_t TextCheckingTypeMask;
> 
> Do we really need 64-bit for this?  Can't we just use unsigned integer here?
> 
Good point. I just copied from original but now change it to unsigned.

> > Source/WebKit/chromium/src/EditorClientImpl.cpp:874
> > -void EditorClientImpl::requestCheckingOfString(SpellChecker* sender, int identifier, const String& text)
> > +void EditorClientImpl::requestCheckingOfString(WebCore::SpellChecker* sender, int identifier, WebCore::TextCheckingTypeMask, const WTF::String& text)
> 
> Mn... why do we need to use WebCore::SpellChecker instead of SpellChecker now?
> 
Oops. removed.

> > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1053
> > +    _sender->didCheck(_sequence, core(_results.get(), _types));
> 
> Where is this core defined?
It's inside WebEditorClient.mm.

[Hajime Morrita]

Created attachment 86422 [details]
Updated to ToT

[Ryosuke Niwa]

Comment on attachment 86422 [details]
Updated to ToT

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

> Source/WebCore/editing/Editor.h:39
> +#include "TextCheckerClient.h"

Are we including this new header file just because we have TextCheckingTypeMask now?  That doesn't sound like a great trade-off.  Editor.h is included in many headers and we'd like to keep it as light weight as possible.  Can we for example add TextCheckingType.h to avoid this?  Or can we make textCheckingTypeMaskFor a static function that takes Editor or EditorClient?

> Source/WebCore/editing/SpellChecker.cpp:144
> +    switch (type) {
> +    case TextCheckingTypeSpelling:
> +        return DocumentMarker::Spelling;
> +    case TextCheckingTypeGrammar:
> +        return DocumentMarker::Grammar;
> +    default:
> +        ASSERT_NOT_REACHED();
> +        return DocumentMarker::Grammar;
> +    }

I'm not sure if it's worth having a switch statement if the enum only has two values but I guess you have a plan to add more values?

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:-1052
> -    else if (type & NSTextCheckingTypeGrammar)
> -        coreType = DocumentMarker::Grammar;

Are you certain that _results doesn't contain grammarDetails?  Or is it harmless to have grammar details?  New core function adds gramar details to the result.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:-1054
> -        coreType = DocumentMarker::AllMarkers;

In new core function, we don't ever put AllMarkers.  In fact, I think this will cause a behavior change.

[Hajime Morrita]

Created attachment 86906 [details]
Patch

[Hajime Morrita]

Hi Ryosuke, thank you for taking a look!
I updated the patch.

(In reply to comment #26)
> (From update of attachment 86422 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86422&action=review
> 
> > Source/WebCore/editing/Editor.h:39
> > +#include "TextCheckerClient.h"
> 
> Are we including this new header file just because we have TextCheckingTypeMask now?  That doesn't sound like a great trade-off.  Editor.h is included in many headers and we'd like to keep it as light weight as possible.  Can we for example add TextCheckingType.h to avoid this?  Or can we make textCheckingTypeMaskFor a static function that takes Editor or EditorClient?
Moved required definitions to TextChecking.h

> 
> > Source/WebCore/editing/SpellChecker.cpp:144
> > +    switch (type) {
> > +    case TextCheckingTypeSpelling:
> > +        return DocumentMarker::Spelling;
> > +    case TextCheckingTypeGrammar:
> > +        return DocumentMarker::Grammar;
> > +    default:
> > +        ASSERT_NOT_REACHED();
> > +        return DocumentMarker::Grammar;
> > +    }
> 
> I'm not sure if it's worth having a switch statement if the enum only has two values but I guess you have a plan to add more values?
I have no plan at this time. So replaced it with an if statement.


> 
> > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:-1052
> > -    else if (type & NSTextCheckingTypeGrammar)
> > -        coreType = DocumentMarker::Grammar;
> 
> Are you certain that _results doesn't contain grammarDetails?  Or is it harmless to have grammar details?  New core function adds gramar details to the result.
>
We should care about bad grammar. The current code is wrong. I filed Bug 57089 for this.
 
> > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:-1054
> > -        coreType = DocumentMarker::AllMarkers;
> 
> In new core function, we don't ever put AllMarkers.  In fact, I think this will cause a behavior change.
Old code did it just for safe. we can safely ignore it. (And we should never reach that path anyway.)

[Ryosuke Niwa]

Jia & Adele, could you verify that the said code change is correct (i.e. only improves Mac port)? In fact, I'd like to wait to r+this patch until your confirmation.

[Jia Pu]

(In reply to comment #29)
> Jia & Adele, could you verify that the said code change is correct (i.e. only improves Mac port)? In fact, I'd like to wait to r+this patch until your confirmation.

Seems fine to me. I tried it in Safari (Mac). All tests that I maintain passed. Although, the patch need to be updated to resolve a trivial conflict in Editor.cpp caused by changeset 82159.

I'm also working in Editor.cpp for bug 57665. But nothing is difficult to resolve.

[Ryosuke Niwa]

Comment on attachment 86906 [details]
Patch

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

Thank you for the confirmation, Jia.

> Source/WebKit/chromium/src/WebTextCheckingCompletionImpl.cpp:53
> +    switch (error) {
> +    case WebKit::WebTextCheckingResult::ErrorSpelling:
> +        return TextCheckingTypeSpelling;
> +    case WebKit::WebTextCheckingResult::ErrorGrammar:
> +        return TextCheckingTypeGrammar;
> +    default:
> +        ASSERT_NOT_REACHED();
> +        return TextCheckingTypeGrammar;
> +    }

You should also replace this with a if statement.

[Hajime Morrita]

Ryosuke, Jia, thank you for helping! 
I'll land this after addressing the Ryosuke's point.

[Hajime Morrita]

Committed r82952: <http://trac.webkit.org/changeset/82952>