[Webkit-unassigned] [Bug 127800] WebKitGTK+ should stop using COMPILE_ASSERT_MATCHING_ENUM macros

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

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




--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2014-02-11 14:52:53 PST ---
(From update of attachment 223899)
View in context: https://bugs.webkit.org/attachment.cgi?id=223899&action=review

Phwew! Great effort. Just needs a little bit of cleanup, though Carlos might have some more comments.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:89
> +static inline WebCore::FindOptions core(WebKitFindOptions type)

Let's use the WebKit2 style name and call this toWebCoreFindOptions.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:38
> -    WebFindOptionsStartInSelection = 1 << 5
> +    WebFindOptionsStartInSelection = 1 << 5,

Is this change actually necessary?

> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:108
> +inline int kit(int errorCode)

Ditto about the name here.

> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:115
> +    case NetworkErrorFailed:
> +        wkErrorCode = WEBKIT_NETWORK_ERROR_FAILED;
> +        break;

Just return WEBKIT_NETWORK_ERROR_FAILED and do the same for the rest. You can avoid having wkErrorCode entirely as well as the break statements.

> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:162
> +    default:
> +        ASSERT_NOT_REACHED();

You should probably pick a default, just in case something goes wrong.

> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1159
> -    return !(error.isCancellation() || error.errorCode() == WEBKIT_POLICY_ERROR_FRAME_LOAD_INTERRUPTED_BY_POLICY_CHANGE || error.errorCode() == WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD);
> +    return !(error.isCancellation() || error.errorCode() == PolicyErrorFrameLoadInterruptedByPolicyChange || error.errorCode() == PluginErrorWillHandleLoad);
>  }

Great catch.

> Source/WebKit/gtk/webkit/webkitwebnavigationaction.cpp:369
> -WebKitWebNavigationReason kit(WebCore::NavigationType type)
> +inline WebKitWebNavigationReason kit(WebCore::NavigationType type)
>  {
> -    return (WebKitWebNavigationReason)type;
> +    WebKitWebNavigationReason wkType(WEBKIT_WEB_NAVIGATION_REASON_LINK_CLICKED);
> +
> +    switch(type) {

Please apply the same suggestions here and for the rest of the new methods.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5616
> +GtkTargetList* kit(GtkTargetList* coreGtkTargetList)

Probably best to call this something like copyGtkTargetListConvertingWebCoreEnumValuesToWebKitEnumValues.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5618
> +    GtkTargetList* kitGtkTargetList(0);

GtkTargetList* kitGtkTargetList = nullptr;

It'd be better to call this something like targetListWithWebKitEnumValues.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5620
> +    if (coreGtkTargetList) {

Use an early return here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5621
> +        gint tableSize(0);

Use int here and don't use a constructor to initialize a primitive.

So int tableSize = 0;

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5624
> +        for (gint i = 0; i < tableSize; i++)

Here as well.

> Source/WebKit/gtk/webkit/webkitwebviewprivate.h:103
> +    GtkTargetList* targetList;

Using a GRefPtr will avoid all uses of gtk_target_list_ref and gtk_target_list_unref.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:72
> +inline static WebKitAuthenticationScheme webCoreProtectionSpaceAuthenticationSchemeToWebKitAuthenticationScheme(WebCore::ProtectionSpaceAuthenticationScheme coreScheme)

You can probably omit the first part of the name of this function so that it is something like toWebKitAuthenticationScheme. I recommend this for all following functions too.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrivate.cpp:26
> +#include "ErrorsGtk.h"
> +#include "WebKitError.h"
> +
>  #include <gdk/gdk.h>

These includes should be all one block.

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