[Chromium] Remove existing markers when updating markers.

[Hironori Bono]

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

[Hironori Bono]

Created attachment 136835 [details]
A patch v0

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Ryosuke Niwa]

Comment on attachment 136835 [details]
A patch v0

How are other ports dealing with this problem? Or is this Chromium specific feature?

[Hironori Bono]

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?

[Ryosuke Niwa]

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

[Hironori Bono]

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.

[Ryosuke Niwa]

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

[Hajime Morrita]

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

[Hironori Bono]

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

[Ryosuke Niwa]

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?

[Hironori Bono]

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?

[Ryosuke Niwa]

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

[Ryosuke Niwa]

s/anxious/obnoxious/

[Hironori Bono]

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

[WebKit Review Bot]

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

[Ryosuke Niwa]

Comment on attachment 137069 [details]
Patch v2 (applied a comment)

Please make sure it builds & tests pass on cr-linux.

[Hironori Bono]

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.

[Kent Tamura]

Comment on attachment 137485 [details]
Patch v3

LGTM for WebKit API change.

[Ryosuke Niwa]

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?

[Hironori Bono]

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

[Ryosuke Niwa]

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?

[Hironori Bono]

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

[WebKit Review Bot]

Comment on attachment 137826 [details]
A patch v5 (added DOCTYPE)

Clearing flags on attachment: 137826

Committed r114605: <http://trac.webkit.org/changeset/114605>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.