Bug 47885 - [Win][DRT] should have LayoutTestController.hasSpellingMarker()
Summary: [Win][DRT] should have LayoutTestController.hasSpellingMarker()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-18 23:15 PDT by Hajime Morrita
Modified: 2010-10-21 22:35 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.61 KB, patch)
2010-10-19 06:50 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (7.39 KB, patch)
2010-10-19 20:24 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (8.30 KB, patch)
2010-10-21 20:49 PDT, Hajime Morrita
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>