[webkit-reviews] review denied: [Bug 32169] Implement TextCodec for WINCE port : [Attachment 44414] The patch (style issues fixed)

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Yong Li
<yong.li.webkit at gmail.com>'s request for review:
Bug 32169: Implement TextCodec for WINCE port
https://bugs.webkit.org/show_bug.cgi?id=32169

Attachment 44414: The patch (style issues fixed)
https://bugs.webkit.org/attachment.cgi?id=44414&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
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.