Summary: | [DRT/Chromium] Import MockSpellCheck from Chromium | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||
Component: | Tools / Tests | Assignee: | 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
Kent Tamura
2010-04-20 23:15:47 PDT
Created attachment 53920 [details]
Proposed patch
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? (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 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?
(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. Created attachment 53936 [details]
Proposed patch (rev.2)
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. Created attachment 53938 [details]
Proposed patch (rev.3)
> > +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 on attachment 53938 [details]
Proposed patch (rev.3)
Looks good!
Comment on attachment 53938 [details] Proposed patch (rev.3) Clearing flags on attachment: 53938 Committed r57981: <http://trac.webkit.org/changeset/57981> All reviewed patches have been landed. Closing bug. |