| Summary: | [WPE] Add gtk-doc | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||||||||||
| Component: | WPE WebKit | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | aperez, berto, bugs-noreply, cadubentzen, cgarcia, commit-queue, mcatanzaro, webkit-bug-importer | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | Other | ||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||
| OS: | Linux | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Michael Catanzaro
2017-10-26 17:05:58 PDT
Forgot to add: I'll need to add documentation for WPEBackends as well. Let's not block on this. Hi Michael! Have you made some progress on this one? If not, I would like to give it a go. Go for it! I'm no longer planning to tackle this one. Created attachment 352756 [details]
Patch
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? 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? 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? Created attachment 352758 [details]
Fix build error
(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+. 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. Created attachment 353013 [details]
Sorting WPE symbols like WebKitGTK+
(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. (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 ¯\_(ツ)_/¯ Created attachment 355328 [details]
ready for review
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? (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+. Created attachment 355403 [details]
Patch
ping reviewers. This looks good to me. I've held off on r+ because Carlos Garcia wanted to review it before landing. (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. Comment on attachment 355403 [details]
Patch
LGTM
Created attachment 356486 [details]
patch for landing
Comment on attachment 356486 [details] patch for landing Clearing flags on attachment: 356486 Committed r238853: <https://trac.webkit.org/changeset/238853> All reviewed patches have been landed. Closing bug. 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 Created attachment 358595 [details]
Patch
(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. 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?
Created attachment 358611 [details]
Patch
(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. Comment on attachment 358611 [details]
Patch
Normally capital letters sort before non-caps, right?
Anyway, this is fine.
Comment on attachment 358611 [details] Patch Clearing flags on attachment: 358611 Committed r239886: <https://trac.webkit.org/changeset/239886> All reviewed patches have been landed. Closing bug. |