[webkit-reviews] review denied: [Bug 19898] [Gtk] WebKitWebHistoryItem: Fixes and improvements : [Attachment 22492] updated patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jul 26 15:56:18 PDT 2008


Christian Dywan <christian at imendio.com> has denied Jan Alonzo
<jmalonzo at gmail.com>'s request for review:
Bug 19898: [Gtk] WebKitWebHistoryItem: Fixes and improvements
https://bugs.webkit.org/show_bug.cgi?id=19898

Attachment 22492: updated patch
https://bugs.webkit.org/attachment.cgi?id=22492&action=edit

------- Additional Comments from Christian Dywan <christian at imendio.com>
>-static void webkit_history_item_add(WebKitWebHistoryItem* webHistoryItem,
WebCore::HistoryItem* historyItem)
>+static void webkit_history_item_add(WebKitWebHistoryItem* webHistoryItem,
RefPtr<HistoryItem> historyItem)
> {
>     g_return_if_fail(WEBKIT_IS_WEB_HISTORY_ITEM(webHistoryItem));
>+    g_return_if_fail(historyItem != NULL);
> 
>     GHashTable* table = webkit_history_items();

You shouldn't use g_return_if_fail in static functions. That macro is meant to
warn about errorneous use of the library. Use ASSERT instead.

>-/* Helper function to create a new WebHistoryItem instance when needed */
>-WebKitWebHistoryItem*
webkit_web_history_item_new_with_core_item(WebCore::HistoryItem* item)
>+/* Helper function to create a new WebHistoryItem instance if needed */
>+WebKitWebHistoryItem* webkit_web_history_item_get_item(RefPtr<HistoryItem>
item)
> {
>+    g_return_val_if_fail(item != NULL, NULL);
>+
>     WebKitWebHistoryItem* webHistoryItem = kit(item);
> 
>     if (!webHistoryItem) {

Dito.

> WebKitWebHistoryItem* webkit_web_history_item_new_with_data(const gchar* uri,
const gchar* title)
> {
>-    WebCore::KURL historyUri(uri);
>-    WebCore::String historyTitle(title);
>+    if (uri == NULL)
>+	  uri = "";
>+
>+    if (title == NULL)
>+	  title = "";
>+
>+    KURL historyUri(uri);
>+    String historyTitle(title);

I would have written that title ? title: "" so it's shorter. But I suppose
that's a matter of personal preference. :)

>@@ -312,18 +322,23 @@ WebKitWebHistoryItem*
webkit_web_history_item_new_with_data(const gchar* uri, co
>  * @webHistoryItem: a #WebKitWebHistoryItem
>  *
>  * Returns the page title of @webHistoryItem
>+ *
>+ * Return value: the title of the @webHistoryItem or %NULL if history item
>+ *		   is %NULL. The string is owned by @webHistoryItem and must
not
>+ *		   be modified or freed.
>  */

As mentioned before, the wording suggests a NULL webHistoryItem were allowed
which it isn't. NULL is just the _fail return value and not an expected value.

Also, while you are at fixing documentation, "webHistoryItem" is an internal
identifier. The API argument is called "web_history_item". I'd appreciate if
you could fix that where needed.

>     return priv->alternateTitle;
>@@ -358,15 +376,22 @@ G_CONST_RETURN gchar*
webkit_web_history_item_get_alternate_title(WebKitWebHisto
>  * @webHistoryItem: a #WebKitWebHistoryItem
>  * @title: the alternate title for @this history item
>  *
>- * Sets an alternate title for @webHistoryItem
>+ * Sets an alternate title for @webHistoryItem. If @title is NULL, this
>+ * function will set the title to an empty string.
>  */
> void webkit_web_history_item_set_alternate_title(WebKitWebHistoryItem*
webHistoryItem, const gchar* title)
> {
>     g_return_if_fail(WEBKIT_IS_WEB_HISTORY_ITEM(webHistoryItem));
> 
>-    WebCore::HistoryItem* item = core(webHistoryItem);
>+    RefPtr<HistoryItem> item = core(webHistoryItem);
>+
>+    if (!item)
>+	  return;

What was the intention here? Silently failing is definitely wrong here.

>  */
> G_CONST_RETURN gchar* webkit_web_history_item_get_uri(WebKitWebHistoryItem*
webHistoryItem)
> {
>     g_return_val_if_fail(WEBKIT_IS_WEB_HISTORY_ITEM(webHistoryItem), NULL);
> 
>-    WebCore::HistoryItem* item =
core(WEBKIT_WEB_HISTORY_ITEM(webHistoryItem));
>+    RefPtr<HistoryItem> item = core(WEBKIT_WEB_HISTORY_ITEM(webHistoryItem));

>
>     g_return_val_if_fail(item != NULL, NULL);
> 
>-    WebCore::String uri = item->urlString();
>     WebKitWebHistoryItemPrivate* priv = webHistoryItem->priv;
>     g_free(priv->uri);
>+
>+    String uri = item->urlString();
>     priv->uri = g_strdup(uri.utf8().data());

That g_return_if_fail tests a condition after creating a HistoryItem pointer
internally, which API users can't influence. Looks like an ASSERT is
appropriate here. 

> G_CONST_RETURN gchar*
webkit_web_history_item_get_original_uri(WebKitWebHistoryItem* webHistoryItem)
> {
>     g_return_val_if_fail(WEBKIT_IS_WEB_HISTORY_ITEM(webHistoryItem), NULL);
> 
>-    WebCore::HistoryItem* item =
core(WEBKIT_WEB_HISTORY_ITEM(webHistoryItem));
>+    RefPtr<HistoryItem> item = core(WEBKIT_WEB_HISTORY_ITEM(webHistoryItem));

> 
>     g_return_val_if_fail(item != NULL, NULL);
> 
>-    WebCore::String originalUri = item->originalURLString();
>     WebKitWebHistoryItemPrivate* priv = webHistoryItem->priv;
>     g_free(priv->originalUri);

Same as above, wrong use of g_return_if_fail.

>     return webHistoryItem->priv->originalUri;
>@@ -424,13 +455,13 @@ G_CONST_RETURN gchar*
webkit_web_history_item_get_original_uri(WebKitWebHistoryI
>  *
>  * Returns the last time @webHistoryItem was visited
>  *
>- * Return value: the time in seconds this @webHistoryItem was last visited
>+ * Return value: the time in seconds this @webHistoryItem was last visited or
0 if @webHistoryItem is NULL
>  */
> gdouble webkit_web_history_item_get_last_visited_time(WebKitWebHistoryItem*
webHistoryItem)

The history item being NULL is an undefined case of wrong API usage that should
not be documented like an expected condition.


More information about the webkit-reviews mailing list