Bug 37910 – [DRT/Chromium] Import MockSpellCheck from Chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Kent Tamura]

[DRT/Chromium] Import MockSpellCheck from Chromium

It will resolve about 90 unexpected failures.

[Kent Tamura]

Created attachment 53920 [details]
Proposed patch

[Shinichiro Hamaji]

Comment on attachment 53920 [details]
Proposed patch

> +        Import webkit/tools/test_shell/mock_spell_check.{cc,h} rev.37241 of Chromium.

Thanks for this line. I found this was very helpful comment. With this, even I could review this patch :)

By the way, it seems we don't have underscores between "spell" and "check". Could you fix this before you land?

[Kent Tamura]

(In reply to comment #2)
> By the way, it seems we don't have underscores between "spell" and "check".
> Could you fix this before you land?

Oh, that's right.  I'll fix it before landing.
Thank you for reviewing.

[Tony Chang]

Comment on attachment 53920 [details]
Proposed patch

Instead of using string16, can we WebString or WebCore::String?  Instead of using map, can we use WTF::HashMap?

[Kent Tamura]

(In reply to comment #4)
> (From update of attachment 53920 [details])
> Instead of using string16, can we WebString or WebCore::String? 
> Instead of using map, can we use WTF::HashMap?

WebString has no find*() methods and I thought we should not use WebCore classes directly.
However String is moving to WTF.  Ok, I'll update the patch.

[Kent Tamura]

Created attachment 53936 [details]
Proposed patch (rev.2)

[Shinichiro Hamaji]

Comment on attachment 53936 [details]
Proposed patch (rev.2)

> +using namespace std;

Do we still need this?

> +    // Convert to String because WebString doesn't have find*() functions.
> +    const String text16(text.data(), text.length());

Is text16 good naming? Also, the comment seems to be stale.

> +    const char wordCharacters[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";

static const would be better. Also, I think we can use isalpha or something instead of this.

[Kent Tamura]

Created attachment 53938 [details]
Proposed patch (rev.3)

[Kent Tamura]

> > +using namespace std;
> 
> Do we still need this?

No.  Removed.

> > +    // Convert to String because WebString doesn't have find*() functions.
> > +    const String text16(text.data(), text.length());
> 
> Is text16 good naming? Also, the comment seems to be stale.

Renamed it to stringText, and updated the comment.

> > +    const char wordCharacters[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
> 
> static const would be better. Also, I think we can use isalpha or something
> instead of this.

Right!  I have changed it to WTF::isASCIIAlpha().

[Shinichiro Hamaji]

Comment on attachment 53938 [details]
Proposed patch (rev.3)

Looks good!

[Kent Tamura]

Comment on attachment 53938 [details]
Proposed patch (rev.3)

Clearing flags on attachment: 53938

Committed r57981: <http://trac.webkit.org/changeset/57981>

[Kent Tamura]

All reviewed patches have been landed.  Closing bug.