Bug 113583 – [GTK] Add API for remembering visited links

• Component: WebKitGTK
• Status: UNCONFIRMED

[Giovanni Campagna]

Currently webkitgtk apps don't remember visited links across sessions (see https://bugzilla.gnome.org/show_bug.cgi?id=685664).
New API is needed to integrate an external history db with the internal hash table of visited links.

Now, different ports already implement this in various ways (using PlatformStrategies):

- WebCore has a default impleementation in PageGroup, which is in memory only
- Qt/WebKit1 hooks into isVisitedLink() and queries its own DB, failing back to the internal hash table on miss
- Chromium overrides PageGroup handling completely
- WebKit2 uses a shared hashtable that is maintained from the WebContext in the UI process, and has a callback in WKHistoryClient to populate the table
  - EFL exposes the callback into its own API
  - Safari/WK2 and Qt/WK2 don't expose it, but maybe they use the C API directly

For Gtk, there are two approaches possible:
- Expose the populateVisitedLinks callback like EFL does
- Add new injected bundle API used by the VisitedLinkStrategy, that hooks to the application and then uses the shared table on miss

The second approach would be probably faster, as it reduces the IPC overhead and avoids preloading the entire history DB at startup, but in my proof-of-concept patch I chose the second one because it's simpler.

[Giovanni Campagna]

Created attachment 195746 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Review Bot]

Attachment 195746 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/UIProcess/API/gtk/WebKitHistoryClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitHistoryClient.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContextPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt']" exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:183:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:184:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:185:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:186:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:187:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitHistoryClient.cpp:30:  Declaration has space between type name and * in WebKitWebContext *webContext  [whitespace/declaration] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitHistoryClient.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h"
Total errors found: 6 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 195746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195746&action=review

Nice patch. What do you plan to use it for? The only issue I see are some small style errors and documentation that could use a little more love.

> Source/WebKit2/ChangeLog:7
> +        https://bugs.webkit.org/show_bug.cgi?id=113583
> +
> +        Reviewed by NOBODY (OOPS!).
> +

This could use small description of the usecase and implementation.

> Source/WebKit2/ChangeLog:19
> +        * GNUmakefile.list.am:
> +        * UIProcess/API/gtk/WebKitHistoryClient.cpp: Added.
> +        (populateVisitedLinks):
> +        (attachHistoryClientToContext):
> +        * UIProcess/API/gtk/WebKitHistoryClient.h: Added.
> +        * UIProcess/API/gtk/WebKitWebContext.cpp:
> +        (createDefaultWebContext):
> +        (webkit_web_context_add_visited_link):
> +        (webkitWebContextPopulateVisitedLinks):
> +        * UIProcess/API/gtk/WebKitWebContext.h:
> +        * UIProcess/API/gtk/WebKitWebContextPrivate.h:
> +        * UIProcess/API/gtk/docs/webkit2gtk-sections.txt:

Normally we put small descriptions of each change after the colons here.

>> Source/WebKit2/UIProcess/API/gtk/WebKitHistoryClient.cpp:30
>> +    WebKitWebContext *webContext;
> 
> Declaration has space between type name and * in WebKitWebContext *webContext  [whitespace/declaration] [3]

Please make sure to fix all style errors. You can verify your patch before uploading it by using the webkit-patch tool in Tools/Scripts or check-webkit-style.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:179
> +     * This signal can be connected to populate the list of
> +     * 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.

[Carlos Garcia Campos]

Comment on attachment 195746 [details]
Patch

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

[Giovanni Campagna]

Created attachment 224698 [details]
Patch

[WebKit Commit Bot]

Attachment 224698 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:218:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:219:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:220:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:221:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:222:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitHistoryClient.cpp:30:  Declaration has space between type name and * in WebKitWebContext *webContext  [whitespace/declaration] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitHistoryClient.h"
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:593:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:593:  Declaration has space between type name and * in cairo_surface_t *makeReferenceSurface  [whitespace/declaration] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:594:  Declaration has space between type name and * in cairo_surface_t *reference  [whitespace/declaration] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:595:  Declaration has space between type name and * in cairo_t *cx  [whitespace/declaration] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:604:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:605:  Declaration has space between type name and * in WebKitWebContext *context  [whitespace/declaration] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:630:  Declaration has space between type name and * in cairo_surface_t *surface  [whitespace/declaration] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:637:  Declaration has space between type name and * in cairo_surface_t *reference  [whitespace/declaration] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h"
Total errors found: 14 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Giovanni Campagna]

Comment on attachment 224698 [details]
Patch

Ops, I'm not sure I integrated all the comments, only did the tests...

[Giovanni Campagna]

(In reply to comment #5)
> (From update of attachment 195746 [details])
> > 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.

So, this API is designed for epiphany, which already has its history storage.
I don't believe the other history client callbacks are useful, they duplicate other signals in WebKitWebView such as resource-load-started and decide-policy. (That epiphany has a history at all is proof)

Which also means that having visited links just work is problematic, because my simple design relies on the app to do the storage. We would need to reimplement the history database using sqlite in webkitgtk, and I'd like to avoid that, at least in the initial implementation.

I believe this API is as simple as it can get, and would allow epiphany to finally have visited links support with minimal changes.

[Giovanni Campagna]

Created attachment 224700 [details]
Patch

[WebKit Commit Bot]

Attachment 224700 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:224:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:225:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:226:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:227:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:228:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitHistoryClient.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h"
Total errors found: 5 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 224700 [details]
Patch

I'm going to r- this - we're trying to get rid of the notion of visited links on the context. See the _WKVisitedLinkProvider in the Objective-C API.