[webkit-reviews] review denied: [Bug 127800] WebKitGTK+ should stop using COMPILE_ASSERT_MATCHING_ENUM macros : [Attachment 223899] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Enrique Ocaña
<eocanha at igalia.com>'s request for review:
Bug 127800: WebKitGTK+ should stop using COMPILE_ASSERT_MATCHING_ENUM macros
https://bugs.webkit.org/show_bug.cgi?id=127800

Attachment 223899: Patch
https://bugs.webkit.org/attachment.cgi?id=223899&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.