Bug 47990 - [GTK] Replace encodings multi-dimensional array with calls to registrar method
Summary: [GTK] Replace encodings multi-dimensional array with calls to registrar method
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 47805
  Show dependency treegraph
Reported: 2010-10-20 09:38 PDT by Carlos Garcia Campos
Modified: 2010-10-21 01:08 PDT (History)
1 user (show)

See Also:

Patch (31.27 KB, patch)
2010-10-20 09:42 PDT, Carlos Garcia Campos
mrobinson: review-
Details | Formatted Diff | Diff
Updated patch (32.93 KB, patch)
2010-10-21 00:49 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2010-10-20 09:38:49 PDT
The complicated multidimensional array used for encodings and aliases in TextCodecGtk could be replaced by calls to registrar. It would also fix the coding style since current variable names contain underscores.
Comment 1 Carlos Garcia Campos 2010-10-20 09:42:00 PDT
Created attachment 71300 [details]
Comment 2 Martin Robinson 2010-10-20 12:28:11 PDT
Comment on attachment 71300 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=71300&action=review

I love this patch. Just a couple small nits.

> WebCore/platform/text/gtk/TextCodecGtk.cpp:92
> +bool TextCodecGtk::registerEncodingAliasIfAvailable(EncodingNameRegistrar registrar, const char* canonicalName, const char* aliasName)

It looks like you never check the return value of this method. Can it just return void?

> WebCore/platform/text/gtk/TextCodecGtk.h:60
> +        static bool registerEncodingNameIfAvailable(EncodingNameRegistrar registrar, const char* canonicalName);
> +        static bool registerEncodingAliasIfAvailable(EncodingNameRegistrar registrar, const char* canonicalName, const char* aliasName);
> +        static void registerCodecIfAvailable(TextCodecRegistrar registrar, const char* codecName);

Could these these can just be static functions in TextCodecGtk.cpp?
Comment 3 Carlos Garcia Campos 2010-10-21 00:49:48 PDT
Created attachment 71400 [details]
Updated patch
Comment 4 Martin Robinson 2010-10-21 00:57:36 PDT
Comment on attachment 71400 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71400&action=review

Great! Thank you.

> WebCore/platform/text/gtk/TextCodecGtk.cpp:62
> +static bool isEncodingAvailable(const gchar* encName)

encName should be replaced with encodingName, since we are avoiding abbreviations in new code. I'll fix this and land it myself. :)
Comment 5 Martin Robinson 2010-10-21 01:08:40 PDT
Committed r70211: <http://trac.webkit.org/changeset/70211>