[Webkit-unassigned] [Bug 68074] [GTK][WEBKIT2] Add support for title property in WebKitWebView

Sat Oct 1 11:30:33 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=68074

Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #109283|review?                     |review-
               Flag|                            |

--- Comment #13 from Martin Robinson <mrobinson at webkit.org>  2011-10-01 11:30:33 PST ---
(From update of attachment 109283)
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->view.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!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!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!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!</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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.