Created attachment 136833 [details] A layout test Greetings, Unfortunately, my r113405 <http://trac.webkit.org/changeset/113405> unveiled another side-effect that SpellChecker::didCheck() does not delete existing markers when adding markers. (The attached layout test is a reduced test case.) Even though this issue happens only on Chrome Canary, I would like to add an argument to this function so Chrome can remove existing markers. (I will upload my change with my next comment.) Regards, Hironori Bono
Created attachment 136835 [details] A patch v0
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 136835 [details] A patch v0 How are other ports dealing with this problem? Or is this Chromium specific feature?
Greetings Niwa-san, Thanks for your quick response. Even though this issue technically happens on all platforms that uses TextCheckingClient::requestCheckingOfString(), it practically happens only on Chromium because this function is used only by Chromium now. (Even though Morita-san once implemented this function for Safari WebKit, Safari now uses WebKit2 and we cannot reproduce this problem on Safari now.) Regards, Hironori Bono (In reply to comment #3) > (From update of attachment 136835 [details]) > How are other ports dealing with this problem? Or is this Chromium specific feature?
(In reply to comment #4) > Thanks for your quick response. Even though this issue technically happens on all platforms that uses TextCheckingClient::requestCheckingOfString(), it practically happens only on Chromium because this function is used only by Chromium now. (Even though Morita-san once implemented this function for Safari WebKit, Safari now uses WebKit2 and we cannot reproduce this problem on Safari now.) Can we tie this fix to the usage of TextCheckingClient::requestCheckingOfString instead of adding new flag? It's non-obvious that we should be calling this function with a flag when some other port start using this API.
Greetings Niwa-san, Thanks for your response. Even though this is just a random thought, one option is splitting didCheck into two functions: didCheckSucceeded() and didCheckFailed(). In the current usage of Chrome, Chrome calls this function for a couple of purposes: notifying Chrome finishes the given text-check request successfully and notifying Chrome has an error while processing the request. Chrome needs to erase existing markers only when it finishes checking text successfully. On the other hand, Chrome should not erase existing markers when it fails checking the text. Would it be possible to give me your thoughts about this idea? Regards, Hironori Bono (In reply to comment #5) > Can we tie this fix to the usage of TextCheckingClient::requestCheckingOfString instead of adding new flag? It's non-obvious that we should be calling this function with a flag when some other port start using this API.
(In reply to comment #6) > Even though this is just a random thought, one option is splitting didCheck into two functions: didCheckSucceeded() and didCheckFailed(). In the current usage of Chrome, Chrome calls this function for a couple of purposes: notifying Chrome finishes the given text-check request successfully and notifying Chrome has an error while processing the request. Chrome needs to erase existing markers only when it finishes checking text successfully. On the other hand, Chrome should not erase existing markers when it fails checking the text. Would it be possible to give me your thoughts about this idea? That makes sense to me at least. Maybe jpu and morrita have some opinions here?
(In reply to comment #7) > (In reply to comment #6) > > Even though this is just a random thought, one option is splitting didCheck into two functions: didCheckSucceeded() and didCheckFailed(). In the current usage of Chrome, Chrome calls this function for a couple of purposes: notifying Chrome finishes the given text-check request successfully and notifying Chrome has an error while processing the request. Chrome needs to erase existing markers only when it finishes checking text successfully. On the other hand, Chrome should not erase existing markers when it fails checking the text. Would it be possible to give me your thoughts about this idea? > > That makes sense to me at least. Maybe jpu and morrita have some opinions here? Sounds making sense to me either. If didCheckFailed() implies leaving the marker, something like didCheckCancel could be a better name.
Created attachment 137041 [details] A patch v1 Greetings Niwa-san and Morita-san, Thanks for your comments. I have added didCheckSucceeded and didCheckCanceled to the SpellChecker class as suggested by Morita-san. (These functions calls SpellChecker::didCheck() to share its code.) Regards, Hironori Bono
Comment on attachment 137041 [details] A patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=137041&action=review > Source/WebCore/editing/SpellChecker.cpp:188 > + if (eraseMarkers) { > + unsigned markers = 0; > + if (m_processingRequest->mask() & TextCheckingTypeSpelling) > + markers |= DocumentMarker::Spelling; > + if (m_processingRequest->mask() & TextCheckingTypeGrammar) > + markers |= DocumentMarker::Grammar; > + if (markers) > + m_frame->document()->markers()->removeMarkers(m_processingRequest->checkingRange().get(), markers); > + } Can't we add this code in didCheckSucceeded instead?
Greetings Niwa-san, Thanks for your comment. To move this code to didCheckSucceeded, we need to copy a sequence check 'if (m_processingRequest->sequence() != sequence)...' in SpellChecker::didCheck() to prevent removing existing markers by mistakes. Is it OK to copy it? Regards, Hironori Bono (In reply to comment #10) > (From update of attachment 137041 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137041&action=review > > > Source/WebCore/editing/SpellChecker.cpp:188 > > + if (eraseMarkers) { > > + unsigned markers = 0; > > + if (m_processingRequest->mask() & TextCheckingTypeSpelling) > > + markers |= DocumentMarker::Spelling; > > + if (m_processingRequest->mask() & TextCheckingTypeGrammar) > > + markers |= DocumentMarker::Grammar; > > + if (markers) > > + m_frame->document()->markers()->removeMarkers(m_processingRequest->checkingRange().get(), markers); > > + } > > Can't we add this code in didCheckSucceeded instead?
(In reply to comment #11) > Greetings Niwa-san, > > Thanks for your comment. To move this code to didCheckSucceeded, we need to copy a sequence check 'if (m_processingRequest->sequence() != sequence)...' in SpellChecker::didCheck() to prevent removing existing markers by mistakes. Is it OK to copy it? But there's also an assertion that ASSERT(m_processingRequest->sequence() == sequence); It's very odd that we assert and then bail out if that were not the case. Anyway, I don't think duplicating that code is that big of a deal. It's probably better than adding an anxious boolean flag.
s/anxious/obnoxious/
Created attachment 137069 [details] Patch v2 (applied a comment) Greetings Niwa-san, Thanks for your comment. I have removed the 'eraseMarkers' parameter from my change. Regards, Hironori Bono
Comment on attachment 137069 [details] Patch v2 (applied a comment) Attachment 137069 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12392895
Comment on attachment 137069 [details] Patch v2 (applied a comment) Please make sure it builds & tests pass on cr-linux.
Created attachment 137485 [details] Patch v3 Greetings Niwa-san, Thanks for your review. As far as I have tested on my Linux PC, I can compile it without errors. I would like to upload my change again to test it on the bot. Regards, Hironori Bono (In reply to comment #16) > (From update of attachment 137069 [details]) > Please make sure it builds & tests pass on cr-linux.
Comment on attachment 137485 [details] Patch v3 LGTM for WebKit API change.
Comment on attachment 137485 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=137485&action=review > Source/WebCore/ChangeLog:5 > + Split SpellChecker::didCheck() to SpellChecker::didCheckSucceeded() and > + SpellChecker::didCheckCanceled() > + https://bugs.webkit.org/show_bug.cgi?id=83748 I don't think we normally wrap the bug summary like this. > Source/WebKit/chromium/ChangeLog:5 > + Split SpellChecker::didCheck() to SpellChecker::didCheckSucceeded() and > + SpellChecker::didCheckCanceled() > + https://bugs.webkit.org/show_bug.cgi?id=83748 Ditto about the wrapping. > Source/WebKit/mac/ChangeLog:5 > + Split SpellChecker::didCheck() to SpellChecker::didCheckSucceeded() and > + SpellChecker::didCheckCanceled() > + https://bugs.webkit.org/show_bug.cgi?id=83748 Ditto. > Tools/ChangeLog:5 > + Split SpellChecker::didCheck() to SpellChecker::didCheckSucceeded() and > + SpellChecker::didCheckCanceled() > + https://bugs.webkit.org/show_bug.cgi?id=83748 Ditto. > LayoutTests/ChangeLog:5 > + Split SpellChecker::didCheck() to SpellChecker::didCheckSucceeded() and > + SpellChecker::didCheckCanceled() > + https://bugs.webkit.org/show_bug.cgi?id=83748 Ditto. > LayoutTests/platform/chromium/editing/spelling/delete-misspelled-word.html:1 > +<html> No DOCTYPE? > LayoutTests/platform/chromium/editing/spelling/delete-misspelled-word.html:16 > +// Insert a misspelled word to initialize a spellchecker. We disable asynchronous > +// spellchecking now to wait until the spellchecker is initialized. I don't think this comment is useful. > LayoutTests/platform/chromium/editing/spelling/delete-misspelled-word.html:21 > +document.execCommand("InsertText", false, 'z'); > +document.execCommand("InsertText", false, 'z'); > +document.execCommand("InsertText", false, ' '); It would have been better if these text insertions were wrapped in some function, insertText('z'); etc... and then included inside shouldBeTrue as in: shouldBeTrue('insertText('z');insertText('z');insertText(' ');internals.hasSpellingMarker(document, 0, 2)'); so that we can see this in the expected result. > LayoutTests/platform/chromium/editing/spelling/delete-misspelled-word.html:31 > +layoutTestController.setAsynchronousSpellCheckingEnabled(true); > +internals.settings.setUnifiedTextCheckingEnabled(true); > + > +layoutTestController.execCommand("DeleteBackward"); > +layoutTestController.execCommand("DeleteBackward"); > +document.execCommand("InsertText", false, ' '); Similarly, we can use debug() function to output some information about what happens here. e.g. debug('Enable asynchronous spellchecking, delete two characters, and insert a space'); > LayoutTests/platform/chromium/editing/spelling/delete-misspelled-word.html:38 > + sleepPeriod *= 2; Exponential back off? I guess it's not bad to wait 1s in the worst case senario. Hopefully, this test won't be flaky. > LayoutTests/platform/chromium/editing/spelling/delete-misspelled-word.html:45 > + if (hasMarker) > + testFailed('internals.hasSpellingMarker(document, 0, 1) should be false. Was true.'); > + else > + testPassed('internals.hasSpellingMarker(document, 0, 1) is false'); Can't we just use shouldBeTrue here?
Created attachment 137816 [details] Patch v4 (applied comments) Greetings Niwa-san and Tamura-san, Thanks for your review and comments. I have quickly updated my change to apply your comments. Would it be possible to take another look? Regards, Hironori Bono
Comment on attachment 137816 [details] Patch v4 (applied comments) View in context: https://bugs.webkit.org/attachment.cgi?id=137816&action=review > LayoutTests/platform/chromium/editing/spelling/delete-misspelled-word.html:1 > +<html> No DOCTYPE?
Created attachment 137826 [details] A patch v5 (added DOCTYPE) Greetings Niwa-san, Thanks for your quick response. I totally forgot adding a DOCTYPE element. Regards, Hironori Bono
Comment on attachment 137826 [details] A patch v5 (added DOCTYPE) Clearing flags on attachment: 137826 Committed r114605: <http://trac.webkit.org/changeset/114605>
All reviewed patches have been landed. Closing bug.