Summary: | Expectation of /editing/spelling/spelling-textarea.html is confusing | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||
Component: | Tools / Tests | Assignee: | Hajime Morrita <morrita> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | jamesr, rniwa | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Hajime Morrita
2010-11-01 20:35:20 PDT
Created attachment 72637 [details]
Patch
Comment on attachment 72637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72637&action=review > LayoutTests/editing/spelling/spelling-contenteditable.html:26 > + // Test the above 'zz' is marked as misspelled. > + shouldBeTrue("layoutTestController.hasSpellingMarker(0, 2)"); > } > > var successfullyParsed = true; Does this even need to be a script test? There seems to be only one test case in this. We should make this a regular dump-as-text test or runDumpAsTextEditingTest test. > LayoutTests/editing/spelling/spelling-textarea.html:23 > - // Test the above 'zz' is marked as misspelled. We test it only on platforms that implement > - // textInputController.hasSpellingMarker() to avoid exceptions. > - logResult(layoutTestController.hasSpellingMarker(0, 2)); > + // Test the above 'zz' is marked as misspelled. > + shouldBeTrue("layoutTestController.hasSpellingMarker(0, 2)"); Ditto for this test as well. You could combine these two tests into one file. Then it's perfectly valid to keep it as a script test. Created attachment 72918 [details]
Patch
Hi Ryosuke, thank you for taking a look!
> Does this even need to be a script test? There seems to be only one test case in this. We should make this a regular dump-as-text test or runDumpAsTextEditingTest test.
Make these two test into one.
On the name spelling-hasspellingmarker, these tests are introduced to check if
LayoutTestController.hasSpellingMarker() works. The name reflects it.
Comment on attachment 72918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72918&action=review > LayoutTests/editing/spelling/script-tests/spelling-hasspellingmarker.js:18 > +shouldBeTrue("hasMarked(\"<input id='test' type='text'>\");"); Is this a new test? If so, you probably say why you're adding this. > LayoutTests/editing/spelling/script-tests/spelling-hasspellingmarker.js:20 > +shouldBeFalse("hasMarked(\"<div id='test' contentEditable='true' spellcheck='false'></div>\");"); This seems to be new as well. Created attachment 73025 [details]
Patch
> (From update of attachment 72918 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72918&action=review
>
> > LayoutTests/editing/spelling/script-tests/spelling-hasspellingmarker.js:18
> > +shouldBeTrue("hasMarked(\"<input id='test' type='text'>\");");
>
> Is this a new test? If so, you probably say why you're adding this.
Yes, it' new for make it more comprehensive.
I added an explanation on the ChangeLog.
Comment on attachment 73025 [details]
Patch
LGTM.
Comment on attachment 73025 [details]
Patch
Looks good except the ChangeLog diff doesn't start at the beginning of the file.
Committed r71392: <http://trac.webkit.org/changeset/71392> |