[webkit-reviews] review denied: [Bug 68074] [GTK][WEBKIT2] Add support for title property in WebKitWebView : [Attachment 109609] title property added

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Nayan Kumar K
<nayankk at motorola.com>'s request for review:
Bug 68074: [GTK][WEBKIT2] Add support for title property in WebKitWebView
https://bugs.webkit.org/show_bug.cgi?id=68074

Attachment 109609: title property added
https://bugs.webkit.org/attachment.cgi?id=109609&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109609&action=review


Looking good in general, but we really need to be careful not to bloat our
testing code. I think my comments below will prevent you from copying a lot of
code from the loading test. In cases where we do need very similar code we
should be abstracting out into helper functions and classes.

Remeber that comments in WebKit should be full sentences that end with a
period. Sometimes we miss instances of that, but the style guide is pretty
clear about it. I think the only exception is if you are labeling some
anonymous parameter.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:53
>      WebKitWebContext* context;
>  
> +    /* Loader Client */
>      GRefPtr<WebKitWebLoaderClient> loaderClient;
> +
> +    /* WebKitWebView properties */
> +    CString title;

Apologies. I don't think my comment was very clear upon re-reading it.
Essentially I just suggested that you keep properties and client in separate
blocks like:

WebKitWebContext* context;
CString title;
...other properties here later...

I think you can omit the comments altogether.

GRefPtr<WebKitWebLoaderClient> loaderClient;
...other clients here later...

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:59
> +    _WebKitWebViewPrivate()
> +    {
> +	   /* Initialize the tile to empty string */
> +	   title = CString("");
> +    }

In WebKit we always use initializer fields. No need for a comment. I think you
can remove this also (see below).

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:143
> +	* Returns: the main frame title of this #WebKitWebView. If the title
has not been received yet, it will be an empty string.

Sorry to nitpick again. :) Please check that the length of this line is
consistent with the other documentation. Also, I'm pretty sure that properties
don't use the "Returns" keyword. You should double-check with other GTK+ doc
though.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:149
> +							   _("Returns the main
frame title of #WebKitWebView"),

I'd rephrase to say "Main frame document title"

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:151
> +							   "",
> +							  
static_cast<GParamFlags>(WEBKIT_PARAM_READABLE)));

Can't you add G_PARAM_CONSTRUCT_ONLY and avoid haivng a _WebKitWebViewPrivate
constuctor above?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:154
> +void webkitWebViewSetTitle(WebKitWebView *webView, WKStringRef titleRef)

You asterisk is in the wrong place here. Please use const char for the title
and convert in the caller.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:304
> + * Returns the property #Object:title.
> + *
> + * Returns: the main frame title of @web_view.

Hrm. It's a bit funky to refer to the property here.  I think what we want to
do is if the text is just a little blurb, reproduce it in both the property and
getter and setter. If there's a longer description, have it in the getter. In
the property documentation we probably just want to have a link that says "See
webkit_web_view_get_title"

I don't think you need to have a line that says "Returns" twice.  Please say
"the main frame document title..."

> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:31
> +#define HTML_TITLE "WelCome to WebKit-GTK+"
> +/* This string has to be rather big because of the cancelled test - it
> + * looks like soup refuses to send or receive a too small chunk */

This comment indicates that you don't need the large string below...there is no
cancelled test here.  We don't need the huge HTML string. Additionally, I
really want these strings not be defined as preprocessor constants. In WebKit
we always prefer using static const char* for things like this. They should
also be defined in the tightest scope possible.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:48
> +/* For real request testing */
> +static void serverCallback(SoupServer* server, SoupMessage* msg, const char*
path, GHashTable* query, SoupClientContext* context, gpointer data)
> +{
> +    if (msg->method != SOUP_METHOD_GET) {
> +	   soup_message_set_status(msg, SOUP_STATUS_NOT_IMPLEMENTED);
> +	   return;
> +    }
> +
> +    soup_message_set_status(msg, SOUP_STATUS_OK);
> +
> +    if (g_str_equal(path, "/"))
> +	   soup_message_body_append(msg->response_body, SOUP_MEMORY_STATIC,
HTML_STRING, strlen(HTML_STRING));
> +
> +    soup_message_body_complete(msg->response_body);
> +}

Instead of doing this, let's wait until the load_html patch lands and avoid
having to use SoupServer.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:101
> +    /* Assert if title is not empty */
> +    g_assert_cmpstr(webkit_web_view_get_title(fixture->webView), ==, "");

I think you can just omit the comment here.  If you feel the code is not
explicit enough, feel free to be more explicit at the expense of redudancy. For
instance:

g_assert_cmpstr(webkit_web_view_get_title(fixture->webView), !=, 0);
g_assert_cmpstr(webkit_web_view_get_title(fixture->webView), ==, "");

> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:110
> +    /* Assert if title is not received */
> +    g_assert(fixture->hasReceivedTitle);

Instead of storing this information in the fixture, why not just assert that
the title is correct here.