Bug 27305 - [WINCE] add WinCE-specific unicode implementation
Summary: [WINCE] add WinCE-specific unicode implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 23154
  Show dependency treegraph
 
Reported: 2009-07-15 09:41 PDT by Joe Mason
Modified: 2009-08-07 14:08 PDT (History)
4 users (show)

See Also:


Attachments
patch adding unicode/wince subdir (525.55 KB, patch)
2009-07-15 09:45 PDT, Joe Mason
no flags Details | Formatted Diff | Diff
unicode/wince subdir with style fixes, minus generated files (29.56 KB, patch)
2009-08-03 16:09 PDT, Joe Mason
no flags Details | Formatted Diff | Diff
generated files for unicode/wince subdir (498.25 KB, patch)
2009-08-03 16:09 PDT, Joe Mason
eric: review-
Details | Formatted Diff | Diff
patch without ICU data files (18.42 KB, patch)
2009-08-06 13:28 PDT, Joe Mason
no flags Details | Formatted Diff | Diff
patch without ICU data files, and updated copyright info (16.72 KB, patch)
2009-08-06 13:38 PDT, Joe Mason
eric: review-
Details | Formatted Diff | Diff
same patch with style fixes (16.69 KB, patch)
2009-08-07 08:22 PDT, Joe Mason
eric: review+
eric: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joe Mason 2009-07-15 09:41:40 PDT
For size reasons, we don't want to link to the full ICU on Wince.  Use a stripped down implementation taking some data structures and routines from ICU and calling WinCE API's directly for others.
Comment 1 Joe Mason 2009-07-15 09:45:50 PDT
Created attachment 32792 [details]
patch adding unicode/wince subdir
Comment 2 Eric Seidel (no email) 2009-07-15 16:02:01 PDT
Comment on attachment 32792 [details]
patch adding unicode/wince subdir

This seems like a bad idea.  My first thought would be that TorchMobile should distribute some sort of support library for this sort of thing.
Comment 3 Yong Li 2009-07-15 19:37:59 PDT
(In reply to comment #2)
> (From update of attachment 32792 [details])
> This seems like a bad idea.  My first thought would be that TorchMobile should
> distribute some sort of support library for this sort of thing.

Why? Other platforms also use either ICU or QT or pango. UnicodeGlib also contains macros from ICU. What we need additionally is just unicode property tables. It seems redundant to create those tables from scratch.
Comment 4 George Staikos 2009-07-16 15:42:59 PDT
I think the main point is that we don't want to require people to use too much outside of the webkit svn repo.  It's quite possible that what we've done for unicode can be reused by other ports later also.
Comment 5 Joe Mason 2009-07-27 07:19:11 PDT
Eric: what specifically about this design do you not like?  Is it the three generated files pulled from ICU?

I will be making a pass for style corrections - what else needs to change to get this in?
Comment 6 Joe Mason 2009-08-03 16:09:03 PDT
Created attachment 34013 [details]
unicode/wince subdir with style fixes, minus generated files

I ran the patch through check-webkit-style and fixed some variable names, and split out the generated files into a separate patch so it's easier to review
Comment 7 Joe Mason 2009-08-03 16:09:52 PDT
Created attachment 34014 [details]
generated files for unicode/wince subdir
Comment 8 Eric Seidel (no email) 2009-08-04 09:47:10 PDT
Comment on attachment 34014 [details]
generated files for unicode/wince subdir

I don't think we should check in these generated files.  I think the wince port should use a support library instead to follow the model of the other ports in the tree.  Alternatively, we could find a way to generate these files at compile time.
Comment 9 Eric Seidel (no email) 2009-08-04 09:49:40 PDT
Comment on attachment 34013 [details]
unicode/wince subdir with style fixes, minus generated files

The project isn't KJS anymore. :)

KJS_UNICODE_WINCE_H

Do we know the ICU license to be compatible with WebCore?  Why is it OK to re-license ICU code under LGPL as you seem to have done?
Comment 10 Joe Mason 2009-08-06 13:28:41 PDT
Created attachment 34223 [details]
patch without ICU data files

Here it is again, with the ICU datafiles and the support code that uses them moved to a support library.  (They are now part of libce at git://code.staikos.net/srv/git/WebKit-CE/libce, which this patch depends on.)
Comment 11 Joe Mason 2009-08-06 13:38:43 PDT
Created attachment 34225 [details]
patch without ICU data files, and updated copyright info

forgot to remove the ICU copyright notice
Comment 12 Eric Seidel (no email) 2009-08-06 18:25:35 PDT
Comment on attachment 34225 [details]
patch without ICU data files, and updated copyright info

Style:
1     const UChar *sourceIterator = source;
 82     const UChar *sourceEnd = source + sourceLength;
 83     UChar *resultIterator = result;
 84     UChar *resultEnd = result + resultLength;

Argument names which don't provide clarity should be removed from headers per our style:
 160         bool isSpace(wchar_t c);
 161         bool isLetter(wchar_t c);
 162         bool isPrintableChar(wchar_t c);
 163         bool isUpper(wchar_t c);
 164         bool isLower(wchar_t c);
 165         bool isPunct(wchar_t c);
 166         bool isDigit(wchar_t c);

I assume these are already covered by other tests?

Thank you very much for posting this much smaller patch.

Unless george is volunteering to fix these when landing, I'm going to r- this and ask you to post a copy with fixed style since you don't have commit-bit yet (to my knowledge).

You will find the "check-webkit-style" script helpful in validating that the style is correct in these files.
Comment 13 Joe Mason 2009-08-07 08:22:28 PDT
Created attachment 34277 [details]
same patch with style fixes

Oops, was using an old version of check-webkit-style
Comment 14 Eric Seidel (no email) 2009-08-07 12:42:31 PDT
Comment on attachment 34277 [details]
same patch with style fixes

Looks OK.

The copy/paste code for toUpper/toLower is bad.  But I'm not gonna fight you about it.
Comment 15 Adam Barth 2009-08-07 14:08:23 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/wtf/Platform.h
	M	JavaScriptCore/wtf/unicode/Unicode.h
	A	JavaScriptCore/wtf/unicode/wince/UnicodeWince.cpp
	A	JavaScriptCore/wtf/unicode/wince/UnicodeWince.h
Committed r46911

Not sure why, but bugzilla-tool had trouble parsing the ChangeLog for this patch.