Bug 46958 - Add additional WKString API
Summary: Add additional WKString API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-30 20:53 PDT by Sam Weinig
Modified: 2010-10-01 15:00 PDT (History)
2 users (show)

See Also:

Attachments
Patch (4.65 KB, patch)
2010-09-30 20:55 PDT, Sam Weinig 		no flags Details | Formatted Diff | Diff
Updated, now includes tests! (11.26 KB, patch)
2010-10-01 13:51 PDT, Sam Weinig 		kenneth: review+
Details | Formatted Diff | Diff
Show Obsolete (1) 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 Sam Weinig 2010-09-30 20:53:30 PDT 
Add additional WKString API to make it a bit more useful.
Comment 1 Sam Weinig 2010-09-30 20:55:57 PDT 
Created attachment 69418 [details]
Patch
Comment 2 Sam Weinig 2010-10-01 13:51:48 PDT 
Created attachment 69511 [details]
Updated, now includes tests!

Updated to include tests. (I am not thrilled with the test, it doesn't do any fancy utf8 stuff, but it is a first step).
Comment 3 WebKit Review Bot 2010-10-01 14:00:29 PDT 
Attachment 69511 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/TestWebKitAPI/Tests/WebKit2/WKString.cpp:26:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKitTools/TestWebKitAPI/Tests/WebKit2/WKString.cpp:45:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Adam Roben (:aroben) 2010-10-01 14:02:58 PDT 
This seems to be duplicating some work that's being done in bug 45393
Comment 5 Kenneth Rohde Christiansen 2010-10-01 14:41:57 PDT 
Comment on attachment 69511 [details]
Updated, now includes tests!

View in context: https://bugs.webkit.org/attachment.cgi?id=69511&action=review

Looks good. r=me

> WebKit2/Shared/WebString.h:62
> +        WTF::Unicode::ConversionResult result = WTF::Unicode::convertUTF16ToUTF8(&d, d + m_string.length(), &p, p + bufferSize - 1, true);

What does the true mean? maybe add a comment like /* ... */ in front of it?

> WebKitTools/TestWebKitAPI/Tests/WebKit2/WKString.cpp:34
> +    WKStringRef string = WKStringCreateWithUTF8CString("hello");

Shouldn't you try constructing it using a "string" containing UTF8 specific chars?
Comment 6 Sam Weinig 2010-10-01 14:47:47 PDT 
(In reply to comment #5)
> (From update of attachment 69511 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69511&action=review
> 
> Looks good. r=me
> 
> > WebKit2/Shared/WebString.h:62
> > +        WTF::Unicode::ConversionResult result = WTF::Unicode::convertUTF16ToUTF8(&d, d + m_string.length(), &p, p + bufferSize - 1, true);
> 
> What does the true mean? maybe add a comment like /* ... */ in front of it?
> 
> > WebKitTools/TestWebKitAPI/Tests/WebKit2/WKString.cpp:34
> > +    WKStringRef string = WKStringCreateWithUTF8CString("hello");
> 
> Shouldn't you try constructing it using a "string" containing UTF8 specific chars?

Yep. I should.  I am going to ask ap to help me construct some more test cases (he is quite good at that).
Comment 7 Sam Weinig 2010-10-01 15:00:12 PDT 
Landed in r68930.