Bug 56085 - [Refactoring] SpellCheckingResult should be replaced with TextCheckingResult
: [Refactoring] SpellCheckingResult should be replaced with TextCheckingResult
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 56368 56369 56811 57088 57089
  Show dependency treegraph
 
Reported: 2011-03-10 02:57 PST by
Modified: 2011-04-05 10:23 PST (History)


Attachments
Patch (65.84 KB, patch)
2011-03-11 02:55 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (67.44 KB, patch)
2011-03-13 21:31 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (68.65 KB, patch)
2011-03-13 22:11 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Yet another attempt to build fix (68.65 KB, patch)
2011-03-13 23:52 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
fix v3 (68.67 KB, patch)
2011-03-14 02:29 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
fix v4 (70.46 KB, patch)
2011-03-14 03:03 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (46.55 KB, patch)
2011-03-15 02:55 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (46.62 KB, patch)
2011-03-15 20:25 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Updated to ToT (46.50 KB, patch)
2011-03-21 22:34 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (46.99 KB, patch)
2011-03-25 00:47 PST, Hajime Morrita
rniwa: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-03-10 02:57:31 PST
These two classes are almost identical so we should kill younger one.
------- Comment #1 From 2011-03-11 02:55:13 PST -------
Created an attachment (id=85454) [details]
Patch
------- Comment #2 From 2011-03-11 02:56:30 PST -------
Ojan, Ryosuke, could you take a look?
My last change was just wrong and I'd like to fix it...
------- Comment #3 From 2011-03-11 03:04:21 PST -------
Attachment 85454 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8135245
------- Comment #4 From 2011-03-11 03:04:54 PST -------
Attachment 85454 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8149237
------- Comment #5 From 2011-03-11 03:05:28 PST -------
Attachment 85454 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8141234
------- Comment #6 From 2011-03-11 03:20:43 PST -------
Attachment 85454 [details] did not build on win:
Build output: http://queues.webkit.org/results/8132210
------- Comment #7 From 2011-03-13 21:31:56 PST -------
Created an attachment (id=85643) [details]
Patch
------- Comment #8 From 2011-03-13 21:45:35 PST -------
Attachment 85643 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8179112
------- Comment #9 From 2011-03-13 21:51:29 PST -------
Attachment 85643 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8175153
------- Comment #10 From 2011-03-13 21:57:15 PST -------
Attachment 85643 [details] did not build on win:
Build output: http://queues.webkit.org/results/8181076
------- Comment #11 From 2011-03-13 22:11:38 PST -------
Created an attachment (id=85645) [details]
Patch
------- Comment #12 From 2011-03-13 22:36:43 PST -------
Attachment 85645 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8181083
------- Comment #13 From 2011-03-13 23:52:01 PST -------
Created an attachment (id=85649) [details]
Yet another attempt to build fix
------- Comment #14 From 2011-03-14 00:12:44 PST -------
Attachment 85649 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8177170
------- Comment #15 From 2011-03-14 01:42:00 PST -------
Attachment 85649 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8177195
------- Comment #16 From 2011-03-14 02:29:06 PST -------
Created an attachment (id=85659) [details]
fix v3
------- Comment #17 From 2011-03-14 02:50:59 PST -------
Attachment 85659 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8174241
------- Comment #18 From 2011-03-14 03:03:58 PST -------
Created an attachment (id=85662) [details]
fix v4
------- Comment #19 From 2011-03-14 12:18:17 PST -------
(From update of attachment 85662 [details])
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.
------- Comment #20 From 2011-03-15 02:55:17 PST -------
Created an attachment (id=85791) [details]
Patch
------- Comment #21 From 2011-03-15 03:00:55 PST -------
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().
------- Comment #22 From 2011-03-15 11:13:16 PST -------
(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?

> 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?
------- Comment #23 From 2011-03-15 20:25:58 PST -------
Created an attachment (id=85901) [details]
Patch
------- Comment #24 From 2011-03-15 20:32:10 PST -------
Hi Ryosuke, thank you for review again!

(In reply to comment #22)
> (From update of attachment 85791 [details] [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.
------- Comment #25 From 2011-03-21 22:34:56 PST -------
Created an attachment (id=86422) [details]
Updated to ToT
------- Comment #26 From 2011-03-23 01:20:07 PST -------
(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?

> 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.
------- Comment #27 From 2011-03-25 00:47:55 PST -------
Created an attachment (id=86906) [details]
Patch
------- Comment #28 From 2011-03-25 00:51:39 PST -------
Hi Ryosuke, thank you for taking a look!
I updated the patch.

(In reply to comment #26)
> (From update of attachment 86422 [details] [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.)
------- Comment #29 From 2011-04-03 00:20:05 PST -------
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.
------- Comment #30 From 2011-04-04 11:42:05 PST -------
(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.
------- Comment #31 From 2011-04-04 12:01:42 PST -------
(From update of attachment 86906 [details])
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.
------- Comment #32 From 2011-04-04 16:24:01 PST -------
Ryosuke, Jia, thank you for helping! 
I'll land this after addressing the Ryosuke's point.
------- Comment #33 From 2011-04-05 10:23:46 PST -------
Committed r82952: <http://trac.webkit.org/changeset/82952>