Bug 47885 – [Win][DRT] should have LayoutTestController.hasSpellingMarker()

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Hajime Morrita]

Implement LayoutTestController.hasSpellingMarker() on windows port.

[Hajime Morrita]

Created attachment 71162 [details]
Patch

[Hajime Morrita]

The word list is ported from Chromium's MockSpellChecker which is used to Chromium DRT 
and known to work.

[Ryosuke Niwa]

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?

[Hajime Morrita]

Created attachment 71242 [details]
Patch

[Hajime Morrita]

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.

[Ryosuke Niwa]

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.

[Kent Tamura]

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.

[Hajime Morrita]

Created attachment 71522 [details]
Patch

[Hajime Morrita]

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.

[Kent Tamura]

Comment on attachment 71522 [details]
Patch

Looks good.

[Hajime Morrita]

Committed r70291: <http://trac.webkit.org/changeset/70291>