Bug 37910 - [DRT/Chromium] Import MockSpellCheck from Chromium
Summary: [DRT/Chromium] Import MockSpellCheck from Chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 35902
  Show dependency tree / graph
 
Reported: 2010-04-20 23:15 PDT by Kent Tamura
Modified: 2010-04-21 07:43 PDT (History)
2 users (show)

See Also:

Attachments
Proposed patch (12.81 KB, patch)
2010-04-20 23:22 PDT, Kent Tamura 		no flags Details | Formatted Diff | Diff
Proposed patch (rev.2) (14.36 KB, patch)
2010-04-21 02:52 PDT, Kent Tamura 		no flags Details | Formatted Diff | Diff
Proposed patch (rev.3) (13.89 KB, patch)
2010-04-21 03:35 PDT, Kent Tamura 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

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