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

Sat Oct 1 11:30:32 PDT 2011

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 109283: Title property added to WebKitWebView.
https://bugs.webkit.org/attachment.cgi?id=109283&action=review

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

> Source/WebKit2/ChangeLog:8
> +	   Support for 'title' property is added in WebKitWebView.
> +	   Functions to get the value of this property is provided.
> +

Typically the description goes under the review line.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:131
> +    webkitWebViewNotifyTitleReceived(webView, title);

This can just be
webkitWebViewNotifyTitleReceived(WEBKIT_WEB_LOADER_CLIENT(clientInfo)->priv->vi
ew.get(), title);

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

Let's keep all the properties and loader clients in separate groups. The order
isn't important beyond organization.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:134
> +	* Returns: the @web_view's document title.

I think you should be more explicit here. This is the title of the main frame.
also why are you using @web_view there's no parameter. I recommend using
#WebKitWebView explicitly instead:

Returns: the main frame title of this #WebKitWebView. If the title has not been
received yet it will be an empty string.

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

I'd prefer this to be called webkitWebViewSetTitle and to take a const char*.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:295
>> + * Returns the @web_view's document title.
>> + *
>> + * Returns: the title of @web_view.
>> + */
>> +const gchar* webkit_web_view_get_title(WebKitWebView* webView)
> 
> webkit_web_view_get_title is incorrectly named. Don't use underscores in your
identifier names.  [readability/naming] [4]

Here you have the same problem but it's worse even becaues the name of the
paramter is @webView.  We really need a build step which verifies the sanity of
the documentation. I'd like to have zero warnings when generating it.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:301
> +
> +    WebKitWebViewPrivate* priv = webView->priv;
> +
> +    return priv->title.data();

This can all be one line.  Ensure that an empty CString does not return null.

>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99
>>> +webkit_web_view_get_title	       (WebKitWebView	      *webView);
>> 
>> The parameter name "webView" adds no information, so it should be removed. 
[readability/parameter_name] [5]
> 
> Extra space before ( in function call  [whitespace/parens] [4]

Shoudl be *web_view.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h:36
> +G_BEGIN_DECLS
> +
> +void webkitWebViewNotifyTitleReceived(WebKitWebView *, WKStringRef);
> +
> +G_END_DECLS

There's no need to specify that these are unmangled. In fact, it's probably
better that private API stays mangled.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:38
> +#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 */
> +#define HTML_STRING
"<html><head><title>"HTML_TITLE"</title></head><body>Testing!Testing!Testing!Te
sting!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!T
esting!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!
Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing
!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testin
g!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testi
ng!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Test
ing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Tes
ting!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Te
sting!Testing!Testing!Testing!</body></html>"
> +
> +/* 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);

Please don't just copy and paste the entire loading test. The test could be
quite simple you could do a notify on the title property of WebView, load a
simple string and then assert that that the title is what you think it is.
Don't forget to assert the proper title before loading anything too.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:195
> +    server = soup_server_new(SOUP_SERVER_PORT, 0, NULL);
> +    soup_server_run_async(server);
> +
> +    soup_server_add_handler(server, NULL, serverCallback, NULL, NULL);
> +
> +    baseURI = soup_uri_new("http://127.0.0.1/");
> +    soup_uri_set_port(baseURI, soup_server_get_port(server));

I'm almost certain you don't need to make a SoupServer here.