Bug 37910

Summary: [DRT/Chromium] Import MockSpellCheck from Chromium
Product: WebKit Reporter: Kent Tamura <tkent>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, hamaji
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: All   
Bug Depends on:    
Bug Blocks: 35902    
Attachments:
Description Flags
Proposed patch
none
Proposed patch (rev.2)
none
Proposed patch (rev.3) none

Description Kent Tamura 2010-04-20 23:15:47 PDT
[DRT/Chromium] Import MockSpellCheck from Chromium

It will resolve about 90 unexpected failures.
Comment 1 Kent Tamura 2010-04-20 23:22:22 PDT
Created attachment 53920 [details]
Proposed patch
Comment 2 Shinichiro Hamaji 2010-04-20 23:36:30 PDT
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?
Comment 3 Kent Tamura 2010-04-20 23:41:47 PDT
(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.
Comment 4 Tony Chang 2010-04-20 23:44:07 PDT
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?
Comment 5 Kent Tamura 2010-04-21 02:51:59 PDT
(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.
Comment 6 Kent Tamura 2010-04-21 02:52:48 PDT
Created attachment 53936 [details]
Proposed patch (rev.2)
Comment 7 Shinichiro Hamaji 2010-04-21 03:13:57 PDT
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.
Comment 8 Kent Tamura 2010-04-21 03:35:01 PDT
Created attachment 53938 [details]
Proposed patch (rev.3)
Comment 9 Kent Tamura 2010-04-21 03:36:46 PDT
> > +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().
Comment 10 Shinichiro Hamaji 2010-04-21 03:43:46 PDT
Comment on attachment 53938 [details]
Proposed patch (rev.3)

Looks good!
Comment 11 Kent Tamura 2010-04-21 07:43:20 PDT
Comment on attachment 53938 [details]
Proposed patch (rev.3)

Clearing flags on attachment: 53938

Committed r57981: <http://trac.webkit.org/changeset/57981>
Comment 12 Kent Tamura 2010-04-21 07:43:28 PDT
All reviewed patches have been landed.  Closing bug.