Bug 47885

Summary: [Win][DRT] should have LayoutTestController.hasSpellingMarker()
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: Tools / TestsAssignee: 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 Flags
Patch
none
Patch
none
Patch tkent: review+

Description Hajime Morrita 2010-10-18 23:15:57 PDT
Implement LayoutTestController.hasSpellingMarker() on windows port.
Comment 1 Hajime Morrita 2010-10-19 06:50:43 PDT
Created attachment 71162 [details]
Patch
Comment 2 Hajime Morrita 2010-10-19 06:51:55 PDT
The word list is ported from Chromium's MockSpellChecker which is used to Chromium DRT 
and known to work.
Comment 3 Ryosuke Niwa 2010-10-19 09:33:11 PDT
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?
Comment 4 Hajime Morrita 2010-10-19 20:24:29 PDT
Created attachment 71242 [details]
Patch
Comment 5 Hajime Morrita 2010-10-19 20:25:45 PDT
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 6 Ryosuke Niwa 2010-10-19 20:32:16 PDT
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 7 Kent Tamura 2010-10-21 19:10:16 PDT
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.
Comment 8 Hajime Morrita 2010-10-21 20:49:01 PDT
Created attachment 71522 [details]
Patch
Comment 9 Hajime Morrita 2010-10-21 20:50:00 PDT
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 10 Kent Tamura 2010-10-21 22:28:43 PDT
Comment on attachment 71522 [details]
Patch

Looks good.
Comment 11 Hajime Morrita 2010-10-21 22:35:38 PDT
Committed r70291: <http://trac.webkit.org/changeset/70291>