[Webkit-unassigned] [Bug 83748] [Chromium] Remove existing markers when updating markers.

Wed Apr 18 09:47:31 PDT 2012

https://bugs.webkit.org/show_bug.cgi?id=83748

--- Comment #19 from Ryosuke Niwa <rniwa at webkit.org>  2012-04-18 09:47:30 PST ---
(From update of attachment 137485)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.