Bug 57944 – [GTK] Implement WKView with a common widget that can be used by both C and gtk APIs

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

In the patch attached to bug #57820 I removed WKView because I thought we wanted to wrap the C API to provide a high level API similar to the webkit1 one. As Martin pointed out we actually want to support both APIs, so we need a common widget to support both WKView (C API) and WebKitWebView (gtk high level API).

[Carlos Garcia Campos]

Created attachment 88405 [details]
Patch

Move gtk API from UIProcess/gtk to UIProcess/API/gtk. WebView and WebViewWidget have been merged into WebKitWebViewBase, the common abstract class that will be used to create view widgets. PageClient implementation has been moved to PageClientImpl and it's used by WebKitWebViewBase. WKView provides a gtk widget for the C API inheriting from WebKitWebViewBase.

[WebKit Review Bot]

Attachment 88405 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:6:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:9:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:15:  The parameter name "webViewBase" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:15:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:16:  The parameter name "webViewBase" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:16:  The parameter name "contextRef" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:16:  The parameter name "pageGroupRef" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:17:  The parameter name "webViewBase" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:29:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:46:  The parameter name "webView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:141:  webkit_web_view_base_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:291:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:351:  webkit_web_view_base_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:52:  Declaration has space between type name and * in WebKitWebViewBase *webViewBase  [whitespace/declaration] [3]
Source/WebKit2/UIProcess/API/C/gtk/WKView.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/gtk/ChunkedUpdateDrawingAreaProxyGtk.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/C/gtk/WKViewPrivate.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 19 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 88416 [details]
Patch fixing coding style issues

Same patch simply fixing coding style, except GObject generated methods.

[WebKit Review Bot]

Attachment 88416 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:142:  webkit_web_view_base_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:292:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:352:  webkit_web_view_base_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alejandro G. Castro]

Comment on attachment 88416 [details]
Patch fixing coding style issues

Looks good, I'm can not see any issue in the C API handling the widget directly instead of just getting it from a function, so it is a good option.

Still not sure if we should have the widget in the C API directory, because in theory the C API should be the base API so I guess it should not depend at all in the gtk API files. Also what about using WKWebViewWidget instead of WebKitWebViewBase, to make it more like the other C API files.

View in context: https://bugs.webkit.org/attachment.cgi?id=88416&action=review

> Source/WebKit2/UIProcess/API/C/gtk/WKView.cpp:38
> +G_DEFINE_TYPE(WKView, wk_view, WEBKIT_TYPE_WEB_VIEW_BASE)

Currently we are implementing directly the get type functions to make the code more WebKit style, that would avoid some of the style warnings.

> Source/WebKit2/UIProcess/API/C/gtk/WKView.cpp:48
> +// Public API

Add a period to the end of the comment.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:49
> +G_DEFINE_ABSTRACT_TYPE(WebKitWebViewBase, webkit_web_view_base, GTK_TYPE_CONTAINER)

Ditto, this will fix the style warning.

> Source/WebKit2/UIProcess/API/gtk/webkitdefines.h:42
> +/*
> + * Copyright (C) 2007 Holger Hans Peter Freyther
> + * Copyright (C) 2008 Collabora Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + */
> +
> +#ifndef webkitdefines_h
> +#define webkitdefines_h
> +
> +#include <glib.h>
> +
> +#ifdef G_OS_WIN32
> +    #ifdef BUILDING_WEBKIT
> +        #define WEBKIT_API __declspec(dllexport)
> +    #else
> +        #define WEBKIT_API __declspec(dllimport)
> +    #endif
> +    #define WEBKIT_OBSOLETE_API WEBKIT_API
> +#else
> +    #define WEBKIT_API __attribute__((visibility("default")))
> +    #define WEBKIT_OBSOLETE_API WEBKIT_API __attribute__((deprecated))
> +#endif
> +
> +#ifndef WEBKIT_API
> +    #define WEBKIT_API
> +#endif
> +
> +#endif

Honestly I'm not sure if we should just forget about reusing code between this API and the webkit1 or try to share some code. At this point I think trying to share code could be overkill at some point so I'm ok with copying the files but open to proposals.

[Martin Robinson]

Comment on attachment 88416 [details]
Patch fixing coding style issues

View in context: https://bugs.webkit.org/attachment.cgi?id=88416&action=review

> Source/WebKit2/ChangeLog:24
> +        * UIProcess/API/gtk/PageClientImpl.cpp: Added.

Minor bikeshed: I think it would make more sense for the directory to be UIProcess/API/gobject.

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 88416 [details])
> Looks good, I'm can not see any issue in the C API handling the widget directly instead of just getting it from a function, so it is a good option.
> 
> Still not sure if we should have the widget in the C API directory, because in theory the C API should be the base API so I guess it should not depend at all in the gtk API files. Also what about using WKWebViewWidget instead of WebKitWebViewBase, to make it more like the other C API files.

WebKitWebViewBase is something in the middle of both the C and the gtk API. Martin was worried that this widget will appear in the object hierarchy in the docs of the gtk API, so I decided to use the prefix of the gtk API for consistency. Both Mac and Qt have their view widgets in UIProcess/API/mac|qt so I tried to be consistent with other ports too. And regarding the name, using the word Widget in the name of class inheriting from GtkWidget sounded a bit redundant to me, but I don't mind to rename it if you think it's better. 

> View in context: https://bugs.webkit.org/attachment.cgi?id=88416&action=review
> 
> > Source/WebKit2/UIProcess/API/C/gtk/WKView.cpp:38
> > +G_DEFINE_TYPE(WKView, wk_view, WEBKIT_TYPE_WEB_VIEW_BASE)
> 
> Currently we are implementing directly the get type functions to make the code more WebKit style, that would avoid some of the style warnings.

I think some exceptions to the coding style make sense, like this one, G_DEFINE_TYPE implementation has changed several times, using the macro improves the maintainability, and it introduces only a couple of funtions that don't follow the coding style. But again, if you think it's not worth it, I'll change it. 

> > Source/WebKit2/UIProcess/API/C/gtk/WKView.cpp:48
> > +// Public API
> 
> Add a period to the end of the comment.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:49
> > +G_DEFINE_ABSTRACT_TYPE(WebKitWebViewBase, webkit_web_view_base, GTK_TYPE_CONTAINER)
> 
> Ditto, this will fix the style warning.
> 
> > Source/WebKit2/UIProcess/API/gtk/webkitdefines.h:42
> > +/*
> > + * Copyright (C) 2007 Holger Hans Peter Freyther
> > + * Copyright (C) 2008 Collabora Ltd.
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Library General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Library General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Library General Public License
> > + * along with this library; see the file COPYING.LIB.  If not, write to
> > + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> > + * Boston, MA 02110-1301, USA.
> > + */
> > +
> > +#ifndef webkitdefines_h
> > +#define webkitdefines_h
> > +
> > +#include <glib.h>
> > +
> > +#ifdef G_OS_WIN32
> > +    #ifdef BUILDING_WEBKIT
> > +        #define WEBKIT_API __declspec(dllexport)
> > +    #else
> > +        #define WEBKIT_API __declspec(dllimport)
> > +    #endif
> > +    #define WEBKIT_OBSOLETE_API WEBKIT_API
> > +#else
> > +    #define WEBKIT_API __attribute__((visibility("default")))
> > +    #define WEBKIT_OBSOLETE_API WEBKIT_API __attribute__((deprecated))
> > +#endif
> > +
> > +#ifndef WEBKIT_API
> > +    #define WEBKIT_API
> > +#endif
> > +
> > +#endif
> 
> Honestly I'm not sure if we should just forget about reusing code between this API and the webkit1 or try to share some code. At this point I think trying to share code could be overkill at some point so I'm ok with copying the files but open to proposals.

Yes, I wan't sure either. 

Thank you very much for the coments

[Carlos Garcia Campos]

(In reply to comment #6)
> (From update of attachment 88416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88416&action=review
> 
> > Source/WebKit2/ChangeLog:24
> > +        * UIProcess/API/gtk/PageClientImpl.cpp: Added.
> 
> Minor bikeshed: I think it would make more sense for the directory to be UIProcess/API/gobject.

WebKitWebViewBase inherits from GtkContainer, and we are using gtk there.

[Carlos Garcia Campos]

Created attachment 89758 [details]
Updated patch

Patch updated to current git master.

[WebKit Review Bot]

Attachment 89758 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:144:  webkit_web_view_base_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:294:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:354:  webkit_web_view_base_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 89758 [details]
Updated patch

I'm going to r- this patch. On discussing this with Alex, it seems that we should be able to leave our current WebKit2 C-API design mostly unchanged. Why remove the WebView class?  It seems there'd be much less code churn if we simply turn the existing WebViewWidget into the base widget. If that's not possible, let's discuss alternatives from there.

[Carlos Garcia Campos]

(In reply to comment #11)
> (From update of attachment 89758 [details])
> I'm going to r- this patch. On discussing this with Alex, it seems that we should be able to leave our current WebKit2 C-API design mostly unchanged.

what design?

> Why remove the WebView class? 

First, I think everything related to the API should be under API dir, for consistency. WebView seems confusing to me, the name WebView makes me think it's a widget, but it's not although it includes some methods that we wouldn't need if it were a widget: getWebViewWindow(), paint(), setSise(). Other methods, however, use the view word like if the view were another thing displayView(), scrollView(), isViewFocused(), etc. This is because this class is mixing PageClient (it inherits from it) and WebView. That's why I moved the bits that belong to the view to the widget implementation and added PageClientImpl.cpp for the PageClient implementation, which is what mac and qt ports do. 

Then I renamed WebViewWidget to WebKitViewBase, for consistency with the gtk1 api, because you were worried about such class appearing in the docs. I made it an abstract class without public methods, so that WKView and WebKitWebView could inherit from it and share the implementation.

> It seems there'd be much less code churn if we simply turn the existing WebViewWidget into the base widget.

That's exactly what I did. I don't see why it's less code, I simply moved code to PageclientImpl and WebKitWebViewBase. It's even less code since I removed unneeded methods like getWebViewWindow(), paint(), setSise().

> If that's not possible, let's discuss alternatives from there.

what's wrong with my alternative?

[Martin Robinson]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 89758 [details] [details])
> > I'm going to r- this patch. On discussing this with Alex, it seems that we should be able to leave our current WebKit2 C-API design mostly unchanged.
> 
> what design?

This patch does a lot of different things at once:

1. It moves the WebKitViewWidget class to a different directory and gives it a different name.
2. It moves a lot of the logic from the PageClient / WebKit into the WebKitViewWidget. 
3. It changes the interface that's exposed in the existing API. Instead of a WKViewRef being an opaque C++ class pointer, it's now a GtkWidget.

I think that some of these things are a good idea and I'm not yet convinced about others. Can you split this patch up a bit so we can discuss each bit separately? First let's move the WebKitViewWidget GObject. Then we can approach each of the other tasks separately.

In particular, I want to move the logic for handling key bindings to a separate class so WebKit1 and WebKit2 can share it. Once I finish that, the second part of this patch will be much smaller.

As for the third point, I feel we should discuss it and reach consensus before making the change.

> what's wrong with my alternative?

It's hard to answer this question, because there are multiple things happening in your patch. It took me quite a while to review it because it was so large. Would you be willing to follow the approach to splitting this patch I suggested above? It will be easier for me and perhaps others to make decisions about your suggested changes if orthogonal parts are in separate patches and bugs. First you could just move and rename the WebKitViewWidget and we could go from there on different bugs.

[Carlos Garcia Campos]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (From update of attachment 89758 [details] [details] [details])
> > > I'm going to r- this patch. On discussing this with Alex, it seems that we should be able to leave our current WebKit2 C-API design mostly unchanged.
> > 
> > what design?
> 
> This patch does a lot of different things at once:

yes, I didn't split it into different patches because it doesn't introduce new code, it simply moves existing one, so you don't need to review the code. 

> 1. It moves the WebKitViewWidget class to a different directory and gives it a different name.
> 2. It moves a lot of the logic from the PageClient / WebKit into the WebKitViewWidget. 
> 3. It changes the interface that's exposed in the existing API. Instead of a WKViewRef being an opaque C++ class pointer, it's now a GtkWidget.

Yes, that's what the webkit2 design docs says: "WKView[Ref]: Native view that hooks into the platform's toolkit. On Windows, this wraps a HWND. On the Mac, it inherits from NSView." So for GTK+ looks natural to me that WKView inherits from a GtkWidget. We discussed it already and we agreed on that.

> 
> I think that some of these things are a good idea and I'm not yet convinced about others.

which ones? it's also difficult to rewrite a patch without knowing what's wrong.

> Can you split this patch up a bit so we can discuss each bit separately? First let's move the WebKitViewWidget GObject. Then we can approach each of the other tasks separately.

Sure, I'll do it.

> In particular, I want to move the logic for handling key bindings to a separate class so WebKit1 and WebKit2 can share it. Once I finish that, the second part of this patch will be much smaller.

Makes a lot of sense.

> As for the third point, I feel we should discuss it and reach consensus before making the change.

Ok, let's do it again in different bugs.

> > what's wrong with my alternative?
> 
> It's hard to answer this question, because there are multiple things happening in your patch. It took me quite a while to review it because it was so large. Would you be willing to follow the approach to splitting this patch I suggested above? It will be easier for me and perhaps others to make decisions about your suggested changes if orthogonal parts are in separate patches and bugs. First you could just move and rename the WebKitViewWidget and we could go from there on different bugs.

Sure.

[Carlos Garcia Campos]

Comment on attachment 89758 [details]
Updated patch

This bug is now a metabug, new patches are attached to bugs blocking this one.

[Carlos Garcia Campos]

All bugs blocking this one are fixed now.