Bug 127800 – WebKitGTK+ should stop using COMPILE_ASSERT_MATCHING_ENUM macros

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Anders Carlsson]

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.

[Enrique Ocaña]

Created attachment 223899 [details]
Patch

[Martin Robinson]

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.

[Enrique Ocaña]

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!

[Enrique Ocaña]

Created attachment 224000 [details]
Patch

[Enrique Ocaña]

Created attachment 224013 [details]
Patch

[WebKit Commit Bot]

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

[Enrique Ocaña]

Created attachment 224745 [details]
Patch

[Enrique Ocaña]

Created attachment 224756 [details]
Patch

[Enrique Ocaña]

Created attachment 224759 [details]
Patch

[Sergio Villar Senin]

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

[Martin Robinson]

Stop the presses! Okay. That sounds like a reasonable change.

[Enrique Ocaña]

Created attachment 224772 [details]
Patch

[Enrique Ocaña]

Created attachment 224774 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 224774 [details]
Patch

Let's go!!!

[WebKit Commit Bot]

Comment on attachment 224774 [details]
Patch

Clearing flags on attachment: 224774

Committed r164438: <http://trac.webkit.org/changeset/164438>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

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.

[Enrique Ocaña]

(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?

[Carlos Garcia Campos]

(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.

[Sergio Villar Senin]

(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

[Enrique Ocaña]

(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.