Bug 178900 – [WPE] Add gtk-doc

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Michael Catanzaro]

We don't want WPE itself to depend on GTK+, but as a matter of practicality, it will have to use gtk-doc for its documentation, since we want the documentation to be shared with WebKitGTK+. So gtk-doc will need to become a dependency for DEVELOPER_MODE, to ensure developers run the same checks to avoid broken documentation that are already run for WebKitGTK+. It's not any sort of problem: gtk-doc does not depend on GTK+ itself, but vice-versa. And gtk-doc is already the only reasonable way to document GObject APIs, even if they do not use GTK+.

I'll need to figure out how to get working, shared documentation with WebKitGTK+, and also figure out how to publish it on our release domain an part of the release process.

[Michael Catanzaro]

Forgot to add: I'll need to add documentation for WPEBackends as well.

[Michael Catanzaro]

Let's not block on this.

[Carlos Bentzen]

Hi Michael! Have you made some progress on this one?

If not, I would like to give it a go.

[Michael Catanzaro]

Go for it! I'm no longer planning to tackle this one.

[Carlos Bentzen]

Created attachment 352756 [details]
Patch

[Carlos Bentzen]

Michael Catanzaro and Carlos Garcia seem to be the ones who usually update the gtk-doc files for WebKitGTK+.

Any tips on ordering of symbols in the -section.txt file?

[Carlos Bentzen]

There is one problem with the shared code: many .cpp files in Source/WebKit/UIProcess/API/glib actually have documentation referring to WebKitGTK+, while they are used for WPE too.

E.g WebKitWebView which causes the short description of WebKitWebView to be "The central class of the WebKit2GTK+ API".

For only the short description it could be just fixed but some useful notes for WebKitGTK+ can't be just deleted, as,

 * Note that #WebKitWebView is scrollable by itself, so you don't need
 * to embed it in a #GtkScrolledWindow.

The API documentation is supposed to be in .c and .cpp files, right? Any ideas on solving this problem?

[Carlos Bentzen]

Also, it can be seen that many documentation is missing from the WPE public API (see wpe-0.1-undocumented.txt in the build directory).  I think adding this documentation can be a follow-up patch for keeping this patch not too big.

Or do you think I should fill up the documentation text already?

[Carlos Bentzen]

Created attachment 352758 [details]
Fix build error

[Michael Catanzaro]

(In reply to Carlos Eduardo Ramalho from comment #7) 
>  * Note that #WebKitWebView is scrollable by itself, so you don't need
>  * to embed it in a #GtkScrolledWindow.
> 
> The API documentation is supposed to be in .c and .cpp files, right? Any
> ideas on solving this problem?

The path of least resistance is to just accept that the documentation will refer to both WebKitGTK+ and WPE WebKit:

"Note that #WebKitWebView is scrollable by itself, so in WebKitGTK+ you don't need to embed it in a #GtkScrolledWindow."

Or:

"The central class of the WebKit API"

(In reply to Carlos Eduardo Ramalho from comment #8)
> Also, it can be seen that many documentation is missing from the WPE public
> API (see wpe-0.1-undocumented.txt in the build directory).  I think adding
> this documentation can be a follow-up patch for keeping this patch not too
> big.
> 
> Or do you think I should fill up the documentation text already?

I would do it in a follow-up patch to keep this one small.

Regarding ordering, I'd keep the same order as used by WebKitGTK+.

[Adrian Perez]

I agree with Michael, let's go ahead and keep the notion that the
API documentation will be the same for both ports (GTK+ and WPE).
It's not only the path of least resistance: it also avoids having
duplicate documentation strings and/or conditional documentation,
which is better for maintainability.

[Carlos Bentzen]

Created attachment 353013 [details]
Sorting WPE symbols like WebKitGTK+

[Carlos Bentzen]

(In reply to Michael Catanzaro from comment #10)
> The path of least resistance is to just accept that the documentation will
> refer to both WebKitGTK+ and WPE WebKit:
> 
> "Note that #WebKitWebView is scrollable by itself, so in WebKitGTK+ you
> don't need to embed it in a #GtkScrolledWindow."
> 
> Or:
> 
> "The central class of the WebKit API"

(In reply to Adrian Perez from comment #11)
> I agree with Michael, let's go ahead and keep the notion that the
> API documentation will be the same for both ports (GTK+ and WPE).
> It's not only the path of least resistance: it also avoids having
> duplicate documentation strings and/or conditional documentation,
> which is better for maintainability.

Ok. In some places there is a long explanation about GDK things and this text will be carried over for WPE docs as well. But I agree that the easiest is accepting that both docs will share these pieces and rewrite/add some sentences on things only applicable for one port or the other.

> (In reply to Carlos Eduardo Ramalho from comment #8)
> > Also, it can be seen that many documentation is missing from the WPE public
> > API (see wpe-0.1-undocumented.txt in the build directory).  I think adding
> > this documentation can be a follow-up patch for keeping this patch not too
> > big.
> > 
> > Or do you think I should fill up the documentation text already?
> 
> I would do it in a follow-up patch to keep this one small.

Ok. I'll do it in a follow-up for adding missing documentation and fix texts only applicable to wkgtk.

> 
> Regarding ordering, I'd keep the same order as used by WebKitGTK+.

Ok. Done.

[Adrian Perez]

(In reply to Carlos Eduardo Ramalho from comment #13)
> (In reply to Michael Catanzaro from comment #10)
> > The path of least resistance is to just accept that the documentation will
> > refer to both WebKitGTK+ and WPE WebKit:
> > 
> > "Note that #WebKitWebView is scrollable by itself, so in WebKitGTK+ you
> > don't need to embed it in a #GtkScrolledWindow."
> > 
> > Or:
> > 
> > "The central class of the WebKit API"
> 
> (In reply to Adrian Perez from comment #11)
> > I agree with Michael, let's go ahead and keep the notion that the
> > API documentation will be the same for both ports (GTK+ and WPE).
> > It's not only the path of least resistance: it also avoids having
> > duplicate documentation strings and/or conditional documentation,
> > which is better for maintainability.
> 
> Ok. In some places there is a long explanation about GDK things and this
> text will be carried over for WPE docs as well. But I agree that the easiest
> is accepting that both docs will share these pieces and rewrite/add some
> sentences on things only applicable for one port or the other.

The GDK explanations won't apply to GTK+ 4 either once we support that.
I think we'll just have to live with the fact that the GDK bits will
appear in the documentation and word them in some way that it's clear
that these comments apply to the GTK+ port, and when using GTK+ < 4.0

¯\_(ツ)_/¯

[Carlos Bentzen]

Created attachment 355328 [details]
ready for review

[Michael Catanzaro]

Comment on attachment 355328 [details]
ready for review

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

Looks sane to me.

> Source/WebKit/PlatformWPE.cmake:152
> +# FIXME: Separate web-extension and DOM headers in two different lists for building separate documentations.

Actually I don't think it's required for WPE, since we have so few DOM headers. Probably easier to keep them together, unlike GTK. So you can remove this FIXME.

(GTK has a huge legacy DOM API that is not exposed by WPE.)

> Source/WebKit/PlatformWPE.cmake:339
> +# FIXME: WebExtension APIs are not included yet. The list of headers for WebExtension also has DOM headers.

We should create a separate documentation bundle for WebExtension/DOM.

> Source/WebKit/UIProcess/API/wpe/WebKitWebContext.h:252
> +WEBKIT_API void
> +webkit_web_context_set_sandbox_enabled              (WebKitWebContext               *context,
> +                                                     gboolean                       enabled);

Why was this moved?

The spacing is off by one (enabled should be aligned with the c of context.)

> Source/WebKit/UIProcess/API/wpe/docs/wpe-0.1-sections.txt:1467
> +<SECTION>
> +<FILE>WebKitDOMObject</FILE>
> +<TITLE>WebKitDOMObject</TITLE>
> +WebKitDOMObject

It should be simple to split the web process APIs into separate documentation, right?

> Source/WebKit/UIProcess/API/wpe/docs/wpe-docs.sgml:70
> +  <!-- FIXME: Move WebKitDOM to its own documentation if the number of DOM APIs increases -->

Let's just have one manual for UI process, and a second manual for web extensions (including DOM).

> Source/WebKit/WebProcess/InjectedBundle/API/wpe/WebKitWebPage.h:79
> +WEBKIT_API WebKitDOMDocument *
> +webkit_web_page_get_dom_document (WebKitWebPage *web_page);
> +

Why is this moved?

[Carlos Bentzen]

(In reply to Michael Catanzaro from comment #16)
> Actually I don't think it's required for WPE, since we have so few DOM
> headers. Probably easier to keep them together, unlike GTK. So you can
> remove this FIXME.
> 
> (GTK has a huge legacy DOM API that is not exposed by WPE.)

Ok.
 
> > Source/WebKit/PlatformWPE.cmake:339
> > +# FIXME: WebExtension APIs are not included yet. The list of headers for WebExtension also has DOM headers.
> 
> We should create a separate documentation bundle for WebExtension/DOM.

Actually this comment sneaked in from while the patch was WIP. I added WebExtensions/DOM already, but it is in the same bundle as the UIProcess APIs.

I will move it to a different bundle then.

> > Source/WebKit/UIProcess/API/wpe/WebKitWebContext.h:252
> > +WEBKIT_API void
> > +webkit_web_context_set_sandbox_enabled              (WebKitWebContext               *context,
> > +                                                     gboolean                       enabled);
> 
> Why was this moved?

To be in the same relative position as in the WebKitGTK+ API. They are the only functions not in the same order as WebKitGTK+.

> 
> The spacing is off by one (enabled should be aligned with the c of context.)

Ok.

> 
> > Source/WebKit/UIProcess/API/wpe/docs/wpe-0.1-sections.txt:1467
> > +<SECTION>
> > +<FILE>WebKitDOMObject</FILE>
> > +<TITLE>WebKitDOMObject</TITLE>
> > +WebKitDOMObject
> 
> It should be simple to split the web process APIs into separate
> documentation, right?
> 
> > Source/WebKit/UIProcess/API/wpe/docs/wpe-docs.sgml:70
> > +  <!-- FIXME: Move WebKitDOM to its own documentation if the number of DOM APIs increases -->
> 
> Let's just have one manual for UI process, and a second manual for web
> extensions (including DOM).
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/wpe/WebKitWebPage.h:79
> > +WEBKIT_API WebKitDOMDocument *
> > +webkit_web_page_get_dom_document (WebKitWebPage *web_page);
> > +
> 
> Why is this moved?

To be in the same relative position as in the WebKitGTK+ API. It is the only function not in the same order as WebKitGTK+.

[Carlos Bentzen]

Created attachment 355403 [details]
Patch

[Carlos Bentzen]

ping reviewers.

[Michael Catanzaro]

This looks good to me. I've held off on r+ because Carlos Garcia wanted to review it before landing.

[Carlos Bentzen]

(In reply to Michael Catanzaro from comment #20)
> This looks good to me. I've held off on r+ because Carlos Garcia wanted to
> review it before landing.

Great! Thanks.

I'll probably need to rebase it, but I'll wait for Carlos Garcia's comments first.

[Carlos Garcia Campos]

Comment on attachment 355403 [details]
Patch

LGTM

[Carlos Bentzen]

Created attachment 356486 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 356486 [details]
patch for landing

Clearing flags on attachment: 356486

Committed r238853: <https://trac.webkit.org/changeset/238853>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alberto Garcia]

I believe that you forgot to add Tools/gtkdoc to manifest.txt.in, I can't build 2.23.2 because of the missing generate-gtkdoc:

[ 91%] Generating ../docs-build.stamp
/usr/bin/cmake -E env CC=/usr/bin/cc "CFLAGS=-Wextra -Wall -Wno-expansion-to-defined -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -g -O2 -fdebug-prefix-map=/tmp/webkit2gtk-2.23.2=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -Wall -DNDEBUG -DG_DISABLE_CAST_CHECKS -fno-strict-aliasing -fno-exceptions -Wno-unused-parameter" /tmp/webkit2gtk-2.23.2/Tools/gtkdoc/generate-gtkdoc --gtk
No such file or directory

[Carlos Bentzen]

Created attachment 358595 [details]
Patch

[Carlos Bentzen]

(In reply to Alberto Garcia from comment #26)
> I believe that you forgot to add Tools/gtkdoc to manifest.txt.in, I can't
> build 2.23.2 because of the missing generate-gtkdoc:
> 
Yeah.. I forgot indeed. In fact, I didn't know about the existence of this file.

Sorry for the mistake.

[Michael Catanzaro]

Comment on attachment 358595 [details]
Patch

I would put it down under Tools/TestWebKitAPI to keep it semi-alphabetical.

And what about # FIXME: We are not currently generating documentation for WPE right below?

[Carlos Bentzen]

Created attachment 358611 [details]
Patch

[Carlos Bentzen]

(In reply to Michael Catanzaro from comment #29)
> Comment on attachment 358595 [details]
> Patch
> 
> I would put it down under Tools/TestWebKitAPI to keep it semi-alphabetical.

I don't see how putting it under Tools/TestWebKitAPI would be more sorted.

> 
> And what about # FIXME: We are not currently generating documentation for
> WPE right below?

Good catch! Removed the FIXME and uncommented the lines to copy documentation artifacts to the release. Didn't notice that before.

[Michael Catanzaro]

Comment on attachment 358611 [details]
Patch

Normally capital letters sort before non-caps, right?

Anyway, this is fine.

[WebKit Commit Bot]

Comment on attachment 358611 [details]
Patch

Clearing flags on attachment: 358611

Committed r239886: <https://trac.webkit.org/changeset/239886>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.