[Webkit-unassigned] [Bug 32169] Implement TextCodec for WINCE port

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=32169


David Kilzer (ddkilzer) <ddkilzer at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #44414|review?                     |review-
               Flag|                            |




--- Comment #5 from David Kilzer (ddkilzer) <ddkilzer at webkit.org>  2010-01-06 08:23:42 PST ---
(From update of attachment 44414)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.