Instead of having COMPILE_ASSERT_MATCHING_ENUM macros, WebKitGTK+ should have functions that can map between different types. For example: inline WKSameDocumentNavigationType toAPI(SameDocumentNavigationType type) { WKFrameNavigationType wkType = kWKSameDocumentNavigationAnchorNavigation; switch (type) { case SameDocumentNavigationAnchorNavigation: wkType = kWKSameDocumentNavigationAnchorNavigation; break; case SameDocumentNavigationSessionStatePush: wkType = kWKSameDocumentNavigationSessionStatePush; break; case SameDocumentNavigationSessionStateReplace: wkType = kWKSameDocumentNavigationSessionStateReplace; break; case SameDocumentNavigationSessionStatePop: wkType = kWKSameDocumentNavigationSessionStatePop; break; } return wkType; } This allows us to change the enum values in WebCore without breaking the builds.
Created attachment 223899 [details] Patch
Comment on attachment 223899 [details] Patch 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.
Thanks for the review, Martin. I'd like to have opinions from you and Carlos about the way I handled the maintenance of the targetList, because I'm not sure if I took the right approach or if it can be improved: Before the patch, the inner PasteboardHelper::defaultPasteboardHelper()->targetList() was returned to the client app using WebKitGTK+. It worked because the values held by that GtkTargetList were of type PasteboardHelper::PasteboardTargetType, which was cast-compatible with WebKitWebViewTargetInfo (the type the client app expects). That inner GtkTargetList was owned and maintained by PasteBoardHelper. The client app just used it without worrying. Now we have to maintain a converted GtkTargetList holding WebKitWebViewTargetInfo items and mirroring the original GtkTargetList in PasteBoardTargetType. I didn't examine the code deep enough to understand the implications of mirroring the inner list. In particular, I dont know if the original list: - Is created once and never changes. I'm re-creating a new list at each API request. This means that if the client asks for it twice, it will have two different lists! (maybe with different values). If the list never changes, I can save some code and use a single centralized mirror list. - Is read-only or, on the contrary, is expected to be modified by the client app. I only synchronized the lists in the PasteBoardHelper --> WebKitWebView direction, not the opposite. If the client adds values to the list, they will pass unnoticed. Moreover, due to the nature of GtkTargetList, I don't have any way of knowing if the client adds new elements and to trigger a synchronization in the the WebKitWebView --> PasteBoardHelper direction. So, please, can I have your opinion about all this? Thanks!
Created attachment 224000 [details] Patch
Created attachment 224013 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 224745 [details] Patch
Created attachment 224756 [details] Patch
Created attachment 224759 [details] Patch
Comment on attachment 224759 [details] Patch Probably a bit late but I'd replace the "inline static" by "static inline" which is much more used
Stop the presses! Okay. That sounds like a reasonable change.
Created attachment 224772 [details] Patch
Created attachment 224774 [details] Patch
Comment on attachment 224774 [details] Patch Let's go!!!
Comment on attachment 224774 [details] Patch Clearing flags on attachment: 224774 Committed r164438: <http://trac.webkit.org/changeset/164438>
All reviewed patches have been landed. Closing bug.
Comment on attachment 224774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224774&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitFindController.h:69 > + /*< private >*/ > + WEBKIT_FIND_OPTIONS_SHOW_OVERLAY = 1 << 5, > + WEBKIT_FIND_OPTIONS_SHOW_FIND_INDICATOR = 1 << 6, > + WEBKIT_FIND_OPTIONS_SHOW_HIGHLIGHT = 1 << 7, I don't think we should expose these here, even when using the private comment.
(In reply to comment #17) > > Source/WebKit2/UIProcess/API/gtk/WebKitFindController.h:69 > > + /*< private >*/ > > + WEBKIT_FIND_OPTIONS_SHOW_OVERLAY = 1 << 5, > > + WEBKIT_FIND_OPTIONS_SHOW_FIND_INDICATOR = 1 << 6, > > + WEBKIT_FIND_OPTIONS_SHOW_HIGHLIGHT = 1 << 7, > > I don't think we should expose these here, even when using the private comment. Then, what should we do in toWebKitFindOptions() when WebKitFindOptions has FindOptionsShowOverlay, FindOptionsShowFindIndicator or FindOptionsShowHighlight enabled? Assert?
(In reply to comment #18) > (In reply to comment #17) > > > > Source/WebKit2/UIProcess/API/gtk/WebKitFindController.h:69 > > > + /*< private >*/ > > > + WEBKIT_FIND_OPTIONS_SHOW_OVERLAY = 1 << 5, > > > + WEBKIT_FIND_OPTIONS_SHOW_FIND_INDICATOR = 1 << 6, > > > + WEBKIT_FIND_OPTIONS_SHOW_HIGHLIGHT = 1 << 7, > > > > I don't think we should expose these here, even when using the private comment. > > Then, what should we do in toWebKitFindOptions() when WebKitFindOptions has FindOptionsShowOverlay, FindOptionsShowFindIndicator or FindOptionsShowHighlight enabled? Assert? You should not call the convert functions with those values, since they are used internally.
(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > > > > Source/WebKit2/UIProcess/API/gtk/WebKitFindController.h:69 > > > > + /*< private >*/ > > > > + WEBKIT_FIND_OPTIONS_SHOW_OVERLAY = 1 << 5, > > > > + WEBKIT_FIND_OPTIONS_SHOW_FIND_INDICATOR = 1 << 6, > > > > + WEBKIT_FIND_OPTIONS_SHOW_HIGHLIGHT = 1 << 7, > > > > > > I don't think we should expose these here, even when using the private comment. > > > > Then, what should we do in toWebKitFindOptions() when WebKitFindOptions has FindOptionsShowOverlay, FindOptionsShowFindIndicator or FindOptionsShowHighlight enabled? Assert? > > You should not call the convert functions with those values, since they are used internally. I agree with Carlos FWIW
(In reply to comment #17) > (From update of attachment 224774 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224774&action=review > > > Source/WebKit2/UIProcess/API/gtk/WebKitFindController.h:69 > > + /*< private >*/ > > + WEBKIT_FIND_OPTIONS_SHOW_OVERLAY = 1 << 5, > > + WEBKIT_FIND_OPTIONS_SHOW_FIND_INDICATOR = 1 << 6, > > + WEBKIT_FIND_OPTIONS_SHOW_HIGHLIGHT = 1 << 7, > > I don't think we should expose these here, even when using the private comment. I've created https://bugs.webkit.org/show_bug.cgi?id=129263 to address this matter.