[webkit-reviews] review denied: [Bug 113583] [GTK] Add API for remembering visited links : [Attachment 195746] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Giovanni Campagna
<scampa.giovanni at gmail.com>'s request for review:
Bug 113583: [GTK] Add API for remembering visited links
https://bugs.webkit.org/show_bug.cgi?id=113583

Attachment 195746: Patch
https://bugs.webkit.org/attachment.cgi?id=195746&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=195746&action=review


Hey Giovanni, thanks for the patch. I have a few comments and some questions
too. Patches adding new API should include unit tests, see:

http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

> Source/WebKit2/UIProcess/API/gtk/WebKitHistoryClient.cpp:24
> +#include <wtf/gobject/GRefPtr.h>

This is not needed since GRefPtr is not used in this file.

> Source/WebKit2/UIProcess/API/gtk/WebKitHistoryClient.cpp:28
> +static void populateVisitedLinks(WKContextRef apiContext, const void
*clientInfo)

We typically use wkContext instead of apiContext for C API variable names, in
this case, you can even omit the parameter name since it's unused.

const void *clientInfo -> const void* clientInfo

> Source/WebKit2/UIProcess/API/gtk/WebKitHistoryClient.cpp:34
> +
> +    webContext =
static_cast<WebKitWebContext*>(const_cast<void*>(clientInfo));
> +
> +    webkitWebContextPopulateVisitedLinks(webContext);

Use GObject casts instead of C++ casts for GObjects, this function
implementation could just be:

webkitWebContextPopulateVisitedLinks(WEBKIT_WEB_CONTEXT(clientInfo));

> Source/WebKit2/UIProcess/API/gtk/WebKitHistoryClient.cpp:46
> +	   0, // didNavigateWithNavigationData
> +	   0, // didPerformClientRedirect
> +	   0, // didPerformServerRedirect
> +	   0, // didUpdateHistoryTitle
> +	   populateVisitedLinks,

Is it enough with populate-visited-links signal + add_visited_link method to
maintain an external visited link storage? I guess we also need to implement
didNavigateWithNavigationData to let the user know when the visited links
storage should be updated, no?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:82
> +    POPULATE_VISITED_LINKS,

Do we plan to implement all other callbacks of the history client? Maybe we
could move this to a separate object WebKitHistoryManager, or just
WebKitHistory simialr to the cookie manager, geolocation provider or favicon
database. Maybe we could use different objects one for visited links,
WebKitVisitedLinksManager/Provider and another for the global history, that
would allow us to implement only the visited links API now and decide about the
global history in the future.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:179
>> +	 * visited links from an external history database.
> 
> This could probably be expanded slightly. Should the user only call
webkit_web_context_add_visited_link in this signal handler? If so that should
be mentioned both here and in the documentation for
webkit_web_context_add_visited_link.

Yes, the documentation should also say when this signal is supposed to be
called.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:180
> +	*/

Add Since: 2.2

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:186
>> +			 g_cclosure_marshal_VOID__VOID,
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent? 
[whitespace/indent] [3]

The WebKit2 API has been designed based on defaults that just work, so I wonder
if we could provide a default implementation for this that could work for most
of the users who simply want to keep track of visited links across multiple
sessions. We could make this signal true handled, if the user doesn't implement
it, we could use our own visited links storage. The users could prevent this
from happening just connecting to the signal and returning TRUE or implement
their own storage and return TRUE too.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:814
> + * @link: the URI of the link

Use uri or link_uri.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:818
> + */

Since 2.2