Summary: | [Win][DRT] should have LayoutTestController.hasSpellingMarker() | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||
Component: | Tools / Tests | Assignee: | Hajime Morrita <morrita> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | rniwa, tkent, tony | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Hajime Morrita
2010-10-18 23:15:57 PDT
Created attachment 71162 [details]
Patch
The word list is ported from Chromium's MockSpellChecker which is used to Chromium DRT and known to work. Comment on attachment 71162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71162&action=review > WebKitTools/DumpRenderTree/win/EditingDelegate.cpp:357 > +static int findIndexOfAlpha(const TCHAR* str) Please spell out alphabet. indexOfFirstAlphabet sounds better. > WebKitTools/DumpRenderTree/win/EditingDelegate.cpp:365 > +static int findIndexOfNonAlpha(const TCHAR* str) Ditto. > WebKitTools/DumpRenderTree/win/EditingDelegate.cpp:410 > + // The following words are used by unit tests. > + TEXT("ifmmp"), > + TEXT("qwertyuiopasd"), > + TEXT("qwertyuiopasdf"), Aren't these chromium specific misspelled words? Created attachment 71242 [details]
Patch
Hi Ryosuke, thank you for taking a look! I updated the patch. > > WebKitTools/DumpRenderTree/win/EditingDelegate.cpp:357 > > +static int findIndexOfAlpha(const TCHAR* str) > > Please spell out alphabet. indexOfFirstAlphabet sounds better. Done. > > > WebKitTools/DumpRenderTree/win/EditingDelegate.cpp:365 > > +static int findIndexOfNonAlpha(const TCHAR* str) > > Ditto. Done. > > > WebKitTools/DumpRenderTree/win/EditingDelegate.cpp:410 > > + // The following words are used by unit tests. > > + TEXT("ifmmp"), > > + TEXT("qwertyuiopasd"), > > + TEXT("qwertyuiopasdf"), > > Aren't these chromium specific misspelled words? Yes, I removed them. Comment on attachment 71242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71242&action=review > WebKitTools/DumpRenderTree/win/EditingDelegate.cpp:371 > +static int indexOfFirstNonAlphabet(const TCHAR* text) > +{ > + const TCHAR* cursor = text; > + while (*cursor && isalpha(*cursor)) > + ++cursor; > + return *cursor ? (cursor - text) : -1; > +}; Might be better to rename text to startOfWord and rename this function to lengthOfWord? "first non-alphabet" sounds confusing. If we do that, we can modify this function to always return cursor - text. > WebKitTools/DumpRenderTree/win/EditingDelegate.cpp:416 > + if (-1 == wordLength) > + wordLength = textString.size() - wordStart; so that we can get rid of these two statements. Comment on attachment 71242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71242&action=review > WebKitTools/DumpRenderTree/win/EditingDelegate.cpp:357 > +static int indexOfFirstAlphabet(const TCHAR* text) The function names should be indexOfFirstWordCharacter because misspelled words might contain digits in the future. We don't need to change the implementation for now. > WebKitTools/DumpRenderTree/win/EditingDelegate.cpp:365 > +static int indexOfFirstNonAlphabet(const TCHAR* text) ditto. This should be indexOfFirstNonWordCharacter, or lengthOfWord as Niwa-san's suggestion. Created attachment 71522 [details]
Patch
Nisa-san, Kent-san, thank you for reviewing this! I update the patch. > > WebKitTools/DumpRenderTree/win/EditingDelegate.cpp:357 > > +static int indexOfFirstAlphabet(const TCHAR* text) > > The function names should be indexOfFirstWordCharacter because misspelled words might contain digits in the future. > We don't need to change the implementation for now. Agreed and renamed. > > > WebKitTools/DumpRenderTree/win/EditingDelegate.cpp:365 > > +static int indexOfFirstNonAlphabet(const TCHAR* text) > > ditto. This should be indexOfFirstNonWordCharacter, or lengthOfWord as Niwa-san's suggestion. Done. Comment on attachment 71522 [details]
Patch
Looks good.
Committed r70291: <http://trac.webkit.org/changeset/70291> |