[Webkit-unassigned] [Bug 127800] WebKitGTK+ should stop using COMPILE_ASSERT_MATCHING_ENUM macros
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Feb 11 14:55:37 PST 2014
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.
More information about the webkit-unassigned
mailing list