[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