[Webkit-unassigned] [Bug 113583] [GTK] Add API for remembering visited links

[bugzilla-daemon at webkit.org]

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


Carlos Garcia Campos <cgarcia at igalia.com> changed:

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




--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-03-30 03:41:35 PST ---
(From update of attachment 195746)
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

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