Bug 45716 - [BREWMP] Port unicode
Summary: [BREWMP] Port unicode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 33564
  Show dependency treegraph
 
Reported: 2010-09-13 15:56 PDT by Kwang Yul Seo
Modified: 2010-10-20 02:52 PDT (History)
4 users (show)

See Also:


Attachments
Patch (10.26 KB, patch)
2010-09-13 16:07 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (13.79 KB, patch)
2010-09-13 16:08 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (13.79 KB, patch)
2010-09-13 16:55 PDT, Kwang Yul Seo
tkent: review-
Details | Formatted Diff | Diff
Patch (15.18 KB, patch)
2010-10-13 17:37 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (13.82 KB, patch)
2010-10-13 17:39 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2010-09-13 15:56:47 PDT
Port unicode.
Comment 1 Kwang Yul Seo 2010-09-13 16:07:12 PDT
Created attachment 67485 [details]
Patch

Brew MP port uses only the subset of ICU library to reduce the binary size. Follow the WinCE's implementation.
Comment 2 Kwang Yul Seo 2010-09-13 16:08:17 PDT
Created attachment 67486 [details]
Patch

Oops. Uploaded the wrong patch.
Comment 3 WebKit Review Bot 2010-09-13 16:42:50 PDT
Attachment 67486 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/unicode/brew/UnicodeBrew.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Kwang Yul Seo 2010-09-13 16:55:39 PDT
Created attachment 67490 [details]
Patch

Fix style errors.
Comment 5 Kwang Yul Seo 2010-09-17 15:40:49 PDT
CC'ing Eric Seidel who reviewed bug 27305. This patch follows the WinCE approach.
Comment 6 Kent Tamura 2010-10-13 17:11:34 PDT
Comment on attachment 67490 [details]
Patch

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

r- because of style errors.

> JavaScriptCore/wtf/unicode/brew/UnicodeBrew.cpp:84
> +    int remainingCharacters = 0;

remainingCharacters should be declared just before it is used.

> JavaScriptCore/wtf/unicode/brew/UnicodeBrew.cpp:85
> +    if (sourceLength <= resultLength)

You should enclose this block with {} because the block contains two physical lines.

> JavaScriptCore/wtf/unicode/brew/UnicodeBrew.cpp:88
> +    else

ditto.

> JavaScriptCore/wtf/unicode/brew/UnicodeBrew.cpp:101
> +int toUpper(UChar* result, int resultLength, const UChar* source, int sourceLength, bool* isError)

Same comments as toLower().
Comment 7 Kwang Yul Seo 2010-10-13 17:37:52 PDT
Created attachment 70689 [details]
Patch

Fix style errors.
Comment 8 Kwang Yul Seo 2010-10-13 17:39:02 PDT
Created attachment 70690 [details]
Patch

Oops. Remove duplicated ChangeLog entry.
Comment 9 Kent Tamura 2010-10-13 17:44:55 PDT
Comment on attachment 70690 [details]
Patch

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

> JavaScriptCore/wtf/unicode/brew/UnicodeBrew.cpp:94
> +        remainingCharacters += sourceEnd - sourceIterator;

+= looks curious here.
  int remainingCharacters = sourceIterator < sourceEnd ? sourceEnd - sourceIterator : 0;
would be better.
Comment 10 Kwang Yul Seo 2010-10-13 18:02:36 PDT
Manually committed r69723: http://trac.webkit.org/changeset/69723
Comment 11 Kwang Yul Seo 2010-10-13 18:03:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Patrick R. Gansterer 2010-10-15 07:23:29 PDT
Do you think we can share the implementation between BREWMP and WinCE?
Do you have an example of "UnicodeFromICU.h"? Is it a "copy" of ce_unicode.h|cpp?
(I'm not very happy with the current WinCE implementation, maybe we can do it better)

IMHO we can share the implementaions as a "UnicodeMinimalCopyOfICU". I don't think that there's something BREW specific in this files.

PS: Use "svn cp" if you copy UnicodeBrew from UnicodeWinCE. This helps when looking at the history.
Comment 13 Kwang Yul Seo 2010-10-19 17:51:27 PDT
(In reply to comment #12)
> Do you think we can share the implementation between BREWMP and WinCE?

Yes, I just copied the implementation of WinCE only with minor style changes.

> Do you have an example of "UnicodeFromICU.h"? Is it a "copy" of ce_unicode.h|cpp?
> (I'm not very happy with the current WinCE implementation, maybe we can do it better)

Yes, it is just a copy of ce_unicode.h|cpp with slight changes.

> 
> IMHO we can share the implementaions as a "UnicodeMinimalCopyOfICU". I don't think that there's something BREW specific in this files.
> 

I agree. There is no Brew MP specific code in these files.

> PS: Use "svn cp" if you copy UnicodeBrew from UnicodeWinCE. This helps when looking at the history.

Thanks for the tip.
Comment 14 Patrick R. Gansterer 2010-10-20 02:52:24 PDT
Created bug 47977 for merging the unicode implementations.