Bug 56085

Created attachment 85454 [details] Patch

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

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

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

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

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

Created attachment 85643 [details] Patch

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

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

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

Created attachment 85645 [details] Patch

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

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

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

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

Created attachment 85659 [details] fix v3

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

Created attachment 85662 [details] fix v4

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.

Created attachment 85791 [details] Patch

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 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?

Created attachment 85901 [details] Patch

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.

Created attachment 86422 [details] Updated to ToT

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.

Created attachment 86906 [details] Patch

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.)

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.

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

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

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