Bug 32169 – Implement TextCodec for WINCE port

• Component: Text
• Status: RESOLVED FIXED

[Yong Li]

Patch will be post soon

[Yong Li]

Created attachment 44414 [details]
The patch (style issues fixed)

fixed style issues: 1) header file order, 2) new namespace indentation style

See bug 27371

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/wince/TextCodecWince.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/text/wince/TextCodecWince.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/text/wince/TextCodecWince.cpp:125:  S_OK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3

[Adam Barth]

> WebCore/platform/text/wince/TextCodecWince.cpp:125:  S_OK is incorrectly named.
> Don't use underscores in your identifier names.  [readability/naming] [4]

False positive filed: https://bugs.webkit.org/show_bug.cgi?id=32225

[Patrick R. Gansterer]

attachment 44414 [details]:
Two headers are missing in TextCodecWince.cpp:
#include <wtf/HashMap.h>
#include <wtf/HashSet.h>

[David Kilzer (:ddkilzer)]

Comment on attachment 44414 [details]
The patch (style issues fixed)

I think Alexey should also have a look at this, but I'll review the first pass.

> diff --git a/WebCore/platform/text/wince/TextCodecWince.cpp b/WebCore/platform/text/wince/TextCodecWince.cpp
> [...]
> +#include "ce_textcodecs.h"
> +#include "CString.h"
> +#include <mlang.h>
> +#include "PlatformString.h"
> +#include "StringHash.h"
> +#include <winbase.h>
> +#include <winnls.h>
> +#include <wtf/unicode/UTF8.h>

The <mlang.h> header should go between "StringHash.h" and <winbse.h>.

Apparently some #include statements are missing here per Comment #4:

#include <wtf/HashMap.h>
#include <wtf/HashSet.h>

> +namespace WebCore {
> +
> +extern IMultiLanguage* getMultiLanguageInterface();

Why isn't this method declared in a header?  Is it defined in WebCore/platform/graphics/wince/FontCacheWince.cpp.  If it's a utility method, I think it should be moved to its own header and source file.

> +// Usage: a lookup table used to get CharsetInfo with code page ID)
> +// Key: code page ID. Value: charset information

There should be a period at the end of the sentences in the comments.

> +// Usage: a map that stores charsets that are supported by system. Sorted by name.
> +// Key: charset. Value: code page ID

Missing a period on the last comment.

> +LanguageManager& languageManager()
> +{
> +    static LanguageManager lm;
> +    return lm;
> +}

This method should be static, too, since it isn't used outside this source file.  (Or won't that work with the friend declaration in LanguageManager?)

> +    // MS says the flag must be 0 for the following code pages

Period at the end of the comment.  Would be nice to spell out "MS" as "Microsoft".


> +    if (codePage == CP_UTF8) {
> +        if (canBeFirstTime) {
> +            // Handle BOM

Need a period at the end of the comment.

> +        // process ascii characters at beginning

Please capitalize "Process" and add a period to the comment.

> +    // FIXME: we need to implement UnencodableHandling: QuestionMarksForUnencodables, EntitiesForUnencodables
> +    // , and URLEncodedEntitiesForUnencodables

Please put this comment on one line and add a period at the end.

> diff --git a/WebCore/platform/text/wince/TextCodecWince.h b/WebCore/platform/text/wince/TextCodecWince.h
> +    virtual String decode(const char*, size_t length, bool flush, bool stopOnError, bool& sawError);
> +    virtual CString encode(const UChar*, size_t length, UnencodableHandling);
> +
> +

Extra blank line here is not needed.

> +    struct EncodingInfo {
> +        String m_encoding;
> +        String m_friendlyName;
> +    };
> +
> +    struct EncodingReceiver {
> +        // Return false to stop enumerating
> +        virtual bool receive(const char* encoding, const wchar_t* friendlyName, unsigned int codePage) = 0;
> +    };

Why is receive() a pure virtual function?  It's implementation doesn't seem optional in enumerateSupportedEncodings().

The "Return false..." comment is a sentence and should end with a period.

r- to address getMultiLanguageInterface() declaration and EncodingReceiver::receive() being pure virtual.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> (From update of attachment 44414 [details])
> > +        // process ascii characters at beginning
> 
> Please capitalize "Process" and add a period to the comment.

"ASCII" should be all-caps as well.

[Patrick R. Gansterer]

Created attachment 46925 [details]
The patch (additonal style issues fixed)

(In reply to comment #5)
> Why isn't this method declared in a header?  Is it defined in
> WebCore/platform/graphics/wince/FontCacheWince.cpp.  If it's a utility method,
> I think it should be moved to its own header and source file.
Where is the best place for this method? Isn't it better to add it as static function in TextCodec or FontCache?

> Why is receive() a pure virtual function?  It's implementation doesn't seem
> optional in enumerateSupportedEncodings().
What is wrong with an abstract class at this place?

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/wince/TextCodecWince.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 46925 [details]
The patch (additonal style issues fixed)

WinCE should be capitalized as WinCE, not Wince, in file names and class names.

The function from FontCacheWinCE.cpp should be declared in FontCacheWinCE.h.

[Patrick R. Gansterer]

Created attachment 47422 [details]
The patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/wince/TextCodecWince.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 47422 [details]
The patch

This looks non-harmful.  I can't be sure it's error-free, but that's what testing is for.  We have various TextCodec tests already.

When is WINCE going to have a working DumpRenderTree? And a buildbot?  These tools are needed before we can land many more WINCE patches.

[Eric Seidel (no email)]

Comment on attachment 47422 [details]
The patch

Nevermind.  You never fixed Darin's requested rename.  r-

[Patrick R. Gansterer]

(In reply to comment #12)
> When is WINCE going to have a working DumpRenderTree? And a buildbot?  These
> tools are needed before we can land many more WINCE patches.
The main problem is the missing build system: https://lists.webkit.org/pipermail/webkit-dev/2010-January/011160.html. Maybe you can get an answer...

(In reply to comment #13)
> Nevermind.  You never fixed Darin's requested rename.  r-
If we rename this file we have to rename all files which are alredy in the tree. Why nobody complained at the other files?

[Patrick R. Gansterer]

Comment on attachment 47422 [details]
The patch

Setting back to r? since I didn't get a response for months. :-/

[Yong Li]

(In reply to comment #15)
> (From update of attachment 47422 [details])
> Setting back to r? since I didn't get a response for months. :-/

Could you mention me in the Change Log? Thanks.

[Patrick R. Gansterer]

Created attachment 52880 [details]
The patch

(In reply to comment #16)
> Could you mention me in the Change Log? Thanks.
Done!

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/wince/TextCodecWince.cpp:28:  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.

[Yong Li]

(In reply to comment #17)
> Created an attachment (id=52880) [details]
> The patch
> (In reply to comment #16)
> > Could you mention me in the Change Log? Thanks.
> Done!

thanks a lot

[Alexey Proskuryakov]

Comment on attachment 52880 [details]
The patch

I think the question you mentioned is this:

> If we rename this file we have to rename all files which are alredy in the
> tree. 

That seems like a good thing to do, yes. It doesn't have to be done at once. Definitely not in this patch.

> Why nobody complained at the other files?

We should do better this time, now that someone noticed the problem.

[Patrick R. Gansterer]

(In reply to comment #20)
> (From update of attachment 52880 [details])
> I think the question you mentioned is this:
> 
> > If we rename this file we have to rename all files which are alredy in the
> > tree. 
> 
> That seems like a good thing to do, yes. It doesn't have to be done at once.
> Definitely not in this patch.
> 
> > Why nobody complained at the other files?
> 
> We should do better this time, now that someone noticed the problem.
Should I create a patch for renaming all WinCE files?

[Alexey Proskuryakov]

You should feel free to make a patch if you want to, yes. There are some caveats with changing file name case in subversion, but if I remember correctly, the problems only occur when landing from a case insensitive file system.

[Patrick R. Gansterer]

Created attachment 52884 [details]
The patch (changed Wince to WinCE)

I now changed the name to WinCE and also changed "CString.h" to <wtf/text/CString.h> (http://trac.webkit.org/changeset/56825)

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/wince/TextCodecWinCE.cpp:28:  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.

[Patrick R. Gansterer]

(In reply to comment #22)
> You should feel free to make a patch if you want to, yes. There are some
> caveats with changing file name case in subversion, but if I remember
> correctly, the problems only occur when landing from a case insensitive file
> system.
This problem occurs only on windows because of the data in the ".svn" folder.
One question before I start: Are two big patches (WTF/JSC + WebCore) changing all WinCE filenames and classnames ok?

[David Kilzer (:ddkilzer)]

(In reply to comment #25)
> This problem occurs only on windows because of the data in the ".svn" folder.
> One question before I start: Are two big patches (WTF/JSC + WebCore) changing
> all WinCE filenames and classnames ok?

It depends on how big each patch is.  Smaller patches are easier to review.  There should be a balance between one patch per file rename, and one patch per JavaScriptCore or WebCore directory.

[Patrick R. Gansterer]

(In reply to comment #26)
> It depends on how big each patch is.  Smaller patches are easier to review. 
> There should be a balance between one patch per file rename, and one patch per
> JavaScriptCore or WebCore directory.
See bug 37287.

[Kwang Yul Seo]

The patch includes native support for EUC-JP and Thai codecs (CP874, TIS620 and MACTHAI). I remember these codecs are moved to libce, but I can't find the repository of libce. Patrick, do you know where libce is hosted?

[Patrick R. Gansterer]

(In reply to comment #28)
> The patch includes native support for EUC-JP and Thai codecs (CP874, TIS620 and
> MACTHAI). I remember these codecs are moved to libce, but I can't find the
> repository of libce. Patrick, do you know where libce is hosted?
No, I don't know. I use the stuff from the old Torch Mobile at http://code.staikos.net/ (offline in the meantime).

[Kwang Yul Seo]

(In reply to comment #29)
> (In reply to comment #28)
> > The patch includes native support for EUC-JP and Thai codecs (CP874, TIS620 and
> > MACTHAI). I remember these codecs are moved to libce, but I can't find the
> > repository of libce. Patrick, do you know where libce is hosted?
> No, I don't know. I use the stuff from the old Torch Mobile at
> http://code.staikos.net/ (offline in the meantime).

You can see the reason why EUC_JP.cpp and Thai.cpp are moved out from bug 27371. To make it build, we need libce which has these two files. Unfortunately, it seems libce is no longer hosted by Torch Mobile.

We have two options here:

1) Remove native supports of EUC_JP and Thai from the patch. This makes sense because these codecs are seldom used.

2) Find libce which include these codecs.

[Patrick R. Gansterer]

(In reply to comment #30)
> You can see the reason why EUC_JP.cpp and Thai.cpp are moved out from bug
> 27371. To make it build, we need libce which has these two files.
> Unfortunately, it seems libce is no longer hosted by Torch Mobile.
> 
> We have two options here:
> 
> 1) Remove native supports of EUC_JP and Thai from the patch. This makes sense
> because these codecs are seldom used.
> 
> 2) Find libce which include these codecs.
I don't like these non-native codecs too. I'll remove them and post a new patch.

[Adam Barth]

Comment on attachment 52884 [details]
The patch (changed Wince to WinCE)

Based on the discussion above, this patch appears to have been r+ed by Eric Seidel provisional on adopting Darin's renaming comment, which it appears you have done.  Thanks.

[WebKit Commit Bot]

Comment on attachment 52884 [details]
The patch (changed Wince to WinCE)

Clearing flags on attachment: 52884

Committed r61839: <http://trac.webkit.org/changeset/61839>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.