Bug 104484 – [GTK] Add sections documentation to WebKit2 GTK+ API

• Component: WebKit2
• Status: RESOLVED FIXED

[Simon Pena]

WebKit2 GTK+ API is currently missing most of the sections documentation.

[Simon Pena]

Created attachment 178423 [details]
Patch

[WebKit Review 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

[Martin Robinson]

Comment on attachment 178423 [details]
Patch

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

Nice! I have a few small suggestions.

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:37
> + * A history item consists out of a title and a uri. It can be part of

uri-> URI

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:39
> + * the #WebKitBackForwardList and the global history. The global
> + * history is used for coloring the links of visited sites.

Does WebKit2 have the concept of the global history as well?

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:46
> + * and do it itself.

maybe "handle the download process itself" ? "it itself" is a bit hard to read.

> Source/WebKit2/UIProcess/API/gtk/WebKitError.cpp:36
> + * final user, trying to work around them, or taking any other

What do you mean by "final user" ? It seems that this section could probably just be omitted. The name WebKitError is fairly self-explanatory.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:46
> + * WebKit will automatically look for available icons in link elements

You probably want to change link to be <link> for the sake of clarity.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:51
> + * If #WebKitSettings:enable-private-browsing is %TRUE new icons won't

Nit: missing a comma after TRUE.

> Source/WebKit2/UIProcess/API/gtk/WebKitPlugin.cpp:35
> + * various usual directories. This object can be used to get more

instead of "various usual directories" maybe you should say "various platform plugin directcories".

> Source/WebKit2/UIProcess/API/gtk/WebKitURIResponse.cpp:39
> + * status code, the content length, the mime type, the https status or

https status -> HTTP status

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:39
> + * JavaScriptDebugger. Using this class one can get a #GtkWidget which

JavaScript debugger

> Source/WebKit2/UIProcess/API/gtk/WebKitWebResource.cpp:40
> + * A web resource encapsulates the data of the download, as well as
> + * the URI and the #WebKitURIResponse.

The use of the word "download" here might be misleading. WebKitWebResource don't just describe downloads, but they describe a particular resource at the end of a particular URL. Loading a page will trigger the creation one one resource for every separate image and stylesheet on the page.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:77
> + * API. #WebKitWebView is scrollable by itself, so you don't need to
> + * embed it in a #GtkScrolledWindow. It is responsible for managing

You can probably omit the discussion about GtkScrolledWindow.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:78
> + * the drawing of the content, forwarding of events. You can load any

drawing of the content and forwarding of events

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:79
> + * URI into the #WebKitWebView or any kind of data string. With

Maybe just omit "any kind" here

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:84
> +/**
> + * SECTION: WebKitWebViewBase
> + * @Short_description: The base class for #WebKitWebView
> + * @Title: WebKitWebViewBase
> + *
> + * Base class for #WebKitWebView.
> + *
> + */
> +

Maybe this should be omitted from the documentation entirely?

[Simon Pena]

Comment on attachment 178423 [details]
Patch

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

I agree with most of your comments, and already commented in those I had doubts. Thanks for reviewing!

>> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:39
>> + * history is used for coloring the links of visited sites.
> 
> Does WebKit2 have the concept of the global history as well?

Hmmm. When I adapted this from the WK1 documentation, I thought that I had grepped the code for recent mentions of that global history. However, I can't find any reference to it now, so it seems that WK2 doesn't have it.

>> Source/WebKit2/UIProcess/API/gtk/WebKitError.cpp:36
>> + * final user, trying to work around them, or taking any other
> 
> What do you mean by "final user" ? It seems that this section could probably just be omitted. The name WebKitError is fairly self-explanatory.

I basically meant that you could even show some kind of "Network Error" (let's say) to someone using a browser using webkit. But yes, it sounds a bit weird. I would still leave the section, even repeating the short description in the long one, since it gives an idea of the errors at a glance.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebResource.cpp:40
>> + * the URI and the #WebKitURIResponse.
> 
> The use of the word "download" here might be misleading. WebKitWebResource don't just describe downloads, but they describe a particular resource at the end of a particular URL. Loading a page will trigger the creation one one resource for every separate image and stylesheet on the page.

I will try to figure out a better description for this, based on your comment.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:77
>> + * embed it in a #GtkScrolledWindow. It is responsible for managing
> 
> You can probably omit the discussion about GtkScrolledWindow.

This is one of the key differences between WK1 and WK2, and I already saw some questions about it in the mailing lists. However, it's true that in the future, people using the WK2 API without former WK1 experience won't need this information at all. Maybe I can remove it from the top, and leave it as an additional comment at the end of the description.

[Martin Robinson]

Comment on attachment 178423 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:77
>>> + * embed it in a #GtkScrolledWindow. It is responsible for managing
>> 
>> You can probably omit the discussion about GtkScrolledWindow.
> 
> This is one of the key differences between WK1 and WK2, and I already saw some questions about it in the mailing lists. However, it's true that in the future, people using the WK2 API without former WK1 experience won't need this information at all. Maybe I can remove it from the top, and leave it as an additional comment at the end of the description.

Yes, or even a migration guide. :) By the way, I love this patch. :)

[Simon Pena]

Created attachment 179043 [details]
Patch

[WebKit Review Bot]

Comment on attachment 179043 [details]
Patch

Clearing flags on attachment: 179043

Committed r137469: <http://trac.webkit.org/changeset/137469>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.