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

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-09-29 23:25:27 PST ---
(From update of attachment 109188)
View in context: https://bugs.webkit.org/attachment.cgi?id=109188&action=review

Thanks for the patch!.

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

This callback should emit a signal like the other callbacks, and the view can just connect to it to update the title. That way we don't private API to notify the view.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:35
> +#include <wtf/text/WTFString.h>

You are using CString, so include CString.h instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:133
> +     * WebKitWebView:title

This should end with :

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:135
> +     * The title associated with WebView's document.

Use the same documentation than webkit1 for things that don't change, this way we have the same strings for translators and API users know this is the same thing.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:150
> +    g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));

Don't use g_return* macros in private methods.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:296
> + * Returns the #Object:title associated with #WebKitWebView.

It should be Returns: Use the same documentation than webkit1 here too.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:308
> +    WebKitWebViewPrivate* priv = webView->priv;
> +    WebPageProxy* page = webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(webView));
> +    String title = page->pageTitle();
> +    if(priv->title != title.utf8())
> +        priv->title = title.utf8();
> +
> +    return priv->title.data();

We are already notified when the title changes, we shouldn't need to check it here again. Or can it change after document is loaded?

> Tools/ChangeLog:10
> +        * GtkLauncher/main.c:
> +        (updateTitle):
> +        (createBrowser):
> +

I wouldn't change the GtkLauncher code. We added this GtkLauncher2 when we wanted to use the same api than webkit1, ideally the same code would work and current #ifdefs were just provisional until new api was added. That's not the case anymore, and we don't want GtkLauncher code with a lot of #ifdefs to support both wk1 and wk2. For now, it's better to add unit tests for every new API. Once we have enough API, we will port MiniBrowser to use the GTK+ API and we'll remove webkit2 support from GtkLauncher.

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