[webkit-reviews] review denied: [Bug 56085] [Refactoring] SpellCheckingResult should be replaced with TextCheckingResult : [Attachment 85662] fix v4

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied MORITA Hajime <morrita at google.com>'s
request for review:
Bug 56085: [Refactoring] SpellCheckingResult should be replaced with
TextCheckingResult
https://bugs.webkit.org/show_bug.cgi?id=56085

Attachment 85662: fix v4
https://bugs.webkit.org/attachment.cgi?id=85662&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.