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

Kent Tamura
Reported 2010-04-20 23:15:47 PDT
[DRT/Chromium] Import MockSpellCheck from Chromium It will resolve about 90 unexpected failures.
Attachments
Proposed patch (12.81 KB, patch)
2010-04-20 23:22 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Proposed patch (rev.2) (14.36 KB, patch)
2010-04-21 02:52 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Proposed patch (rev.3) (13.89 KB, patch)
2010-04-21 03:35 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Kent Tamura
Comment 1 2010-04-20 23:22:22 PDT
Created attachment 53920 [details] Proposed patch
Shinichiro Hamaji
Comment 2 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?
Kent Tamura
Comment 3 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.
Tony Chang
Comment 4 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?
Kent Tamura
Comment 5 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.
Kent Tamura
Comment 6 2010-04-21 02:52:48 PDT
Created attachment 53936 [details] Proposed patch (rev.2)
Shinichiro Hamaji
Comment 7 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.
Kent Tamura
Comment 8 2010-04-21 03:35:01 PDT
Created attachment 53938 [details] Proposed patch (rev.3)
Kent Tamura
Comment 9 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().
Shinichiro Hamaji
Comment 10 2010-04-21 03:43:46 PDT
Comment on attachment 53938 [details] Proposed patch (rev.3) Looks good!
Kent Tamura
Comment 11 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>
Kent Tamura
Comment 12 2010-04-21 07:43:28 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.