Bug 32169 - Implement TextCodec for WINCE port
Summary: Implement TextCodec for WINCE port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-04 13:07 PST by Yong Li
Modified: 2010-06-25 01:30 PDT (History)
8 users (show)

See Also:


Attachments
The patch (style issues fixed) (20.76 KB, patch)
2009-12-07 06:55 PST, Yong Li
ddkilzer: review-
Details | Formatted Diff | Diff
The patch (additonal style issues fixed) (20.97 KB, patch)
2010-01-19 10:38 PST, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
The patch (23.02 KB, patch)
2010-01-26 11:12 PST, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
The patch (23.70 KB, patch)
2010-04-08 10:57 PDT, Patrick R. Gansterer
ap: review-
Details | Formatted Diff | Diff
The patch (changed Wince to WinCE) (23.71 KB, patch)
2010-04-08 11:58 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2009-12-04 13:07:54 PST
Patch will be post soon
Comment 1 Yong Li 2009-12-07 06:55:46 PST
Created attachment 44414 [details]
The patch (style issues fixed)

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

See bug 27371
Comment 2 WebKit Review Bot 2009-12-07 06:58:11 PST
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
Comment 3 Adam Barth 2009-12-07 09:15:02 PST
> 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
Comment 4 Patrick R. Gansterer 2009-12-26 23:33:27 PST
attachment 44414 [details]:
Two headers are missing in TextCodecWince.cpp:
#include <wtf/HashMap.h>
#include <wtf/HashSet.h>
Comment 5 David Kilzer (:ddkilzer) 2010-01-06 08:23:42 PST
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.
Comment 6 David Kilzer (:ddkilzer) 2010-01-06 08:25:53 PST
(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.
Comment 7 Patrick R. Gansterer 2010-01-19 10:38:20 PST
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?
Comment 8 WebKit Review Bot 2010-01-19 10:40:49 PST
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.
Comment 9 Darin Adler 2010-01-19 11:56:53 PST
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.
Comment 10 Patrick R. Gansterer 2010-01-26 11:12:55 PST
Created attachment 47422 [details]
The patch
Comment 11 WebKit Review Bot 2010-01-26 11:19:37 PST
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.
Comment 12 Eric Seidel (no email) 2010-01-26 14:34:15 PST
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.
Comment 13 Eric Seidel (no email) 2010-01-26 14:34:51 PST
Comment on attachment 47422 [details]
The patch

Nevermind.  You never fixed Darin's requested rename.  r-
Comment 14 Patrick R. Gansterer 2010-01-27 00:42:36 PST
(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?
Comment 15 Patrick R. Gansterer 2010-04-08 10:12:59 PDT
Comment on attachment 47422 [details]
The patch

Setting back to r? since I didn't get a response for months. :-/
Comment 16 Yong Li 2010-04-08 10:28:08 PDT
(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.
Comment 17 Patrick R. Gansterer 2010-04-08 10:57:43 PDT
Created attachment 52880 [details]
The patch

(In reply to comment #16)
> Could you mention me in the Change Log? Thanks.
Done!
Comment 18 WebKit Review Bot 2010-04-08 10:58:19 PDT
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.
Comment 19 Yong Li 2010-04-08 11:01:48 PDT
(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
Comment 20 Alexey Proskuryakov 2010-04-08 11:24:22 PDT
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.
Comment 21 Patrick R. Gansterer 2010-04-08 11:28:55 PDT
(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?
Comment 22 Alexey Proskuryakov 2010-04-08 11:55:06 PDT
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.
Comment 23 Patrick R. Gansterer 2010-04-08 11:58:20 PDT
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)
Comment 24 WebKit Review Bot 2010-04-08 12:01:50 PDT
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.
Comment 25 Patrick R. Gansterer 2010-04-08 12:19:22 PDT
(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?
Comment 26 David Kilzer (:ddkilzer) 2010-04-08 21:05:05 PDT
(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.
Comment 27 Patrick R. Gansterer 2010-04-09 01:04:46 PDT
(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.
Comment 28 Kwang Yul Seo 2010-04-11 23:53:12 PDT
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?
Comment 29 Patrick R. Gansterer 2010-04-12 01:42:24 PDT
(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).
Comment 30 Kwang Yul Seo 2010-04-12 02:52:11 PDT
(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.
Comment 31 Patrick R. Gansterer 2010-04-14 04:48:26 PDT
(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.
Comment 32 Adam Barth 2010-06-20 10:23:51 PDT
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.
Comment 33 WebKit Commit Bot 2010-06-25 01:30:17 PDT
Comment on attachment 52884 [details]
The patch (changed Wince to WinCE)

Clearing flags on attachment: 52884

Committed r61839: <http://trac.webkit.org/changeset/61839>
Comment 34 WebKit Commit Bot 2010-06-25 01:30:24 PDT
All reviewed patches have been landed.  Closing bug.