Bug 27371 – [WINCE] Add WinCE specific platform/text files to WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Adam Treat]

The following patch contains the WinCE specific platform/text files for WebCore.

[Adam Treat]

Created attachment 32939 [details]
Adds platform/text/wince files to WebCore

I've run them through cpplint and fixed them up by hand... fwiw.

[Kwang Yul Seo]

Japanese and Thai are natively supported with libiconv while Chinese and Korean are not. Any reason for this?

[Yong Li]

Created attachment 33103 [details]
code clean-up

modified for obvious coding style conflicts

[Yong Li]

(In reply to comment #3)
> Created an attachment (id=33103) [details]
> code clean-up
> 
> modified for obvious coding style conflicts

I forgot to list 2 files in ChangeLog: TextCodecWince.h and Thai.cpp. But they are included in the patch.

[Adam Treat]

Created attachment 33203 [details]
Latest version with latest cpplint cleanup

This version also has all licensing info included in the patch files.  They are all compatible.

[Joe Mason]

We try to use the WinCE's os support as much as possible to avoid duplication.  These Japanese and Thai codecs aren't supported by the os so we have to implement them ourself.

[Eric Seidel (no email)]

Comment on attachment 33203 [details]
Latest version with latest cpplint cleanup

This is too large for me to review.

Why are we adding:
platform/text/wince/Euc_jp.cpp
?

Now that you all have your magical UnicodeCE library, is this something that shoudl go in that?  Does the rest of WebCore want this Euc_jp.cpp file?  How is it generated, where does it come from?

[Kwang Yul Seo]

Korean and Chinese codecs aren't supported on most devices too.

IMultiLanguage::EnumCodePages is the only to way to know what codecs are supported under WINCE. The list of WINCE OS supported codecs are different among devices.


I think there are three options here:

1) Remove Euc_JP and That and use only the OS supported codecs.

2) Use libiconv enitrely so that make sure most codecs are supported.

3) Make each codec as a plugin, so more codecs can be installed later.

I think the option 1 is good enough because other options can be done anyway by browser vendors anyway.

[George Staikos]

We can move them out but the more we move out, the more external dependencies there will be in order to get it to build.  We have no desire to link in large libraries.  We only take what is necessary.

[Yong Li]

(In reply to comment #8)
> Korean and Chinese codecs aren't supported on most devices too.
> 
> IMultiLanguage::EnumCodePages is the only to way to know what codecs are
> supported under WINCE. The list of WINCE OS supported codecs are different
> among devices.
> 
> 
> I think there are three options here:
> 
> 1) Remove Euc_JP and That and use only the OS supported codecs.
> 
> 2) Use libiconv enitrely so that make sure most codecs are supported.
> 
> 3) Make each codec as a plugin, so more codecs can be installed later.
> 
> I think the option 1 is good enough because other options can be done anyway by
> browser vendors anyway.

You do not only just want the codecs, but also want to display them with correct fonts. Is it helpful providing Korean and Chinese codecs without installing necessary fonts?

Most codecs of displayable languages are provided by OS and font provider. So we don't have to take care of them.

euc_jp is a special one. It's not because we only do this for Japanese language, but because euc_jp codec is not provided even by Windows Mobile Japanese version, neither by some 3rd-party Japanese language packs.

Thai is similar, but not that important, because most Thai websites use UTF-8.

[Yong Li]

Created attachment 34875 [details]
modified

Thai.cpp and Euc_jp.cpp moved out

[Eric Seidel (no email)]

Comment on attachment 34875 [details]
modified

This line is in the middle of nowhere:
 5  * All rights reserved.

Not true:
 2  * This file is part of the DOM implementation for KDE.

Why?
 246 #undef isSentenceStop

I'm not sure what this syntax does:
 59     friend LanguageManager& languageManager();

Why std::map instead of HashMap?
static std::map<UINT, std::string>& codePageCharsets()
 65 {
 66     static std::map<UINT, std::string> cc;
 67     return cc;
 68 }

Indent:
214 TextCodecWince::TextCodecWince(const TextEncoding& encoding)
 215 : m_encoding(encoding)

Comments are likely needed here:
 231     if (codePage > 50000) {
 232         if ((codePage >= 50220 && codePage <= 50222)
 233             || codePage == 50225
 234             || codePage == 50227
 235             || codePage == 50229
 236             || codePage == 52936
 237             || codePage == 54936
 238             || (codePage >= 57002 && codePage <= 57001)
 239             || codePage == 65000 // UTF-7

We generally use full english words for variable names:
 293                         int rtn = encMbToWc(dst, (const unsigned char*)bytes, srcEnd - bytes);
 294                         if (rtn >= 0)
(obviously we may have to adjust to avoid keywords here)

I dont' think explicit construction is needed here:
66     if (resultLength <= 0)
 467         return CString("?");

r-, mostly needing explanation as to why the std:: types, generally we only use std:: algorithms, not types in WebCore.  IIRC that's due to compiling without exceptions.

[Yong Li]

(In reply to comment #12)

> I'm not sure what this syntax does:
>  59     friend LanguageManager& languageManager();

Only this function can construct the object.

Thanks for reviewing. Will upload a new patch soon.

[Yong Li]

Created attachment 38902 [details]
modified as Eric suggested

[Eric Seidel (no email)]

Comment on attachment 38902 [details]
modified as Eric suggested

Style:
 247     static WordBreakIterator *iterator = 0;

Also, these lines can be combined:
247     static WordBreakIterator *iterator = 0;
 248     if (!iterator)
 249         iterator = new WordBreakIterator;

static WordBreakIterator* iterator = new WordBreakIterator;

that will only run once on the function's first run.  Likewise you could just use:

static WordBreakIterator iterator; and return &iterator instead.  Although you might need to use the DEFINE_STATIC_LOCAL macro.

Please add a contructor for your BreakIterators sine they all seem to share the same initialization code.

Longer variable names are preferrd:
int textBreakFirst(TextBreakIterator* bi)
 298 {
 299     return bi->first();
 300 }

Why WebCore::?
 420         WebCore::decode(result, m_encoding.name(), bytes, length, &left, m_decodeBuffer.isEmpty(), sawInvalidChar);

Why L'' here?
 427         result.append(L'?');

[Eric Seidel (no email)]

This code is large and complicated enough that it's difficutl to approve, so I expect we're going to go through this "you posting a patch and me picking out style nits" cycles for a while.  Would help to make it smaller, or to go over it yourself with a fine tooth comb to make sure that there is as little as possible code duplication and that it conforms as best as you know how to the style rules.

[Yong Li]

> 
> Why WebCore::?
>  420         WebCore::decode(result, m_encoding.name(), bytes, length, &left,
> m_decodeBuffer.isEmpty(), sawInvalidChar);

because we want to use the static function but not the method. if there's no "WebCore::", compiler gives an error.

> 
> Why L'' here?
>  427         result.append(L'?');

L'' means it's wide char, the same type as UChar.

[Yong Li]

Created attachment 39106 [details]
modified again

[Adam Barth]

Comment on attachment 39106 [details]
modified again

I'm sorry, but this patch is too large to review properly.  Please create smaller patches.  Also, are there any tests for this code so we can see that it's correct?

[Yong Li]

Created attachment 41318 [details]
TextBreakIterator

[Yong Li]

Created attachment 41319 [details]
(2) TextBoundaries

[Yong Li]

Created attachment 41321 [details]
(3) TextCodec

[Yong Li]

(In reply to comment #19)
> (From update of attachment 39106 [details])
> I'm sorry, but this patch is too large to review properly.  Please create
> smaller patches.  Also, are there any tests for this code so we can see that
> it's correct?

I've separated it into 3 parts.

I don't think the wince port can build yet. As you can see, upstreaming is still in process.

But this code is used for Iris Browser at least in 1.1.9, and works well and stably.

[Eric Seidel (no email)]

Comment on attachment 41318 [details]
TextBreakIterator

Looks sane.

[Eric Seidel (no email)]

Comment on attachment 41319 [details]
(2) TextBoundaries

OK.

[Eric Seidel (no email)]

Comment on attachment 41321 [details]
(3) TextCodec

I am not really capable of reviewing this patch.

[WebKit Commit Bot]

Comment on attachment 41318 [details]
TextBreakIterator

Rejecting patch 41318 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

[WebKit Commit Bot]

Comment on attachment 41319 [details]
(2) TextBoundaries

Rejecting patch 41319 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

[Eric Seidel (no email)]

(In reply to comment #27)
> (From update of attachment 41318 [details])
> Rejecting patch 41318 from commit-queue.
> 
> [e-mail] does not have committer permissions according to
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

It looks like you have more than one bugzilla account. :(  The email address field in the Committer constructor can take an array.  You should feel free to commit a patch which adds your second email address to Committer("Yong Li", ["[e-mail]", "[e-mail]"])

[Eric Seidel (no email)]

I've added your second bugzilla account to committers.py in:
http://trac.webkit.org/changeset/50821

[Adam Barth]

Checking style for patch 41321 from bug 27371.
Running check-webkit-style
WebCore/platform/text/wince/TextCodecWince.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/text/wince/TextCodecWince.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Done processing WebCore/platform/text/wince/TextCodecWince.h
WebCore/platform/text/wince/TextCodecWince.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Done processing WebCore/platform/text/wince/TextCodecWince.cpp
Total errors found: 3

[WebKit Commit Bot]

Comment on attachment 41318 [details]
TextBreakIterator

Clearing flags on attachment: 41318

Committed r51164: <http://trac.webkit.org/changeset/51164>

[WebKit Commit Bot]

Comment on attachment 41319 [details]
(2) TextBoundaries

Clearing flags on attachment: 41319

Committed r51165: <http://trac.webkit.org/changeset/51165>

[Adam Barth]

Attachment 41321 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/wince/TextCodecWince.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/text/wince/TextCodecWince.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Done processing WebCore/platform/text/wince/TextCodecWince.h
WebCore/platform/text/wince/TextCodecWince.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Done processing WebCore/platform/text/wince/TextCodecWince.cpp
Total errors found: 3

[Adam Barth]

Comment on attachment 41321 [details]
(3) TextCodec

This patch hasn't been touched in a month and has style problem.  Would you be willing to fix the style issues and re-post the patch, ideally on a new bug?

[Yong Li]

(In reply to comment #35)
> (From update of attachment 41321 [details])
> This patch hasn't been touched in a month and has style problem.  Would you be
> willing to fix the style issues and re-post the patch, ideally on a new bug?

Yes. I'll fix the style issues, and raise a new bug.

[Adam Treat]

(In reply to comment #36)
> (In reply to comment #35)
> > (From update of attachment 41321 [details] [details])
> > This patch hasn't been touched in a month and has style problem.  Would you be
> > willing to fix the style issues and re-post the patch, ideally on a new bug?
> 
> Yes. I'll fix the style issues, and raise a new bug.

You don't need to fix the namespace indentation issue.  The style guide has changed since the time you posted this for review.  As such it has been grandfathered in as far as I'm concerned.

This is why changing the style guide at whimsy is folly.

[Adam Barth]

> You don't need to fix the namespace indentation issue.

Yep, I agree.  Although, you could if you wanted to.  :)