Bug 27899 – [Gtk] Expose a database API

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Jan Alonzo]

Some use cases include quota tracking and setting of database storage paths for embedders to modify if they want to.

[Jan Alonzo]

Created attachment 34428 [details]
work in progress patch

[Martin Robinson]

Created attachment 34544 [details]
First pass at a database API

I'm attaching my first pass to solicit comments from more experienced people. I'd love to get some feedback before I start preparing this patch for review.

[Martin Robinson]

Created attachment 34675 [details]
Updated patch

Added an updated patch that:

1. Fixes some bugs
2. Exposes webkit_security_origin_get_database(...)
3. Integrates the database API into DumpRenderLayout, which causes all storage tests to pass now.

[Jan Alonzo]

Comment on attachment 34675 [details]
Updated patch


> +#include "config.h"
> +#include "webkitwebdatabase.h"
> +#include "webkitprivate.h"

There should be a space between webdatabase and private.

> +
> +#include <glib/gi18n-lib.h>

Third-party includes go after the WebCore ones.

> +#include "CString.h"
> +#include "PlatformString.h"
> +#include "DatabaseTracker.h"
> +
> +/**
> + * SECTION:webkitwebdatabase
> + * @short_description: A security boundary for web sites

What does security boundary mean? Not sure if it's describing database or securityOrigin.

> + *
> + * Database quotas and usages are also defined per security origin. The
> + * cumulative disk usage of an origin's databases may be retrieved with
> + * #webkit_security_origin_get_database_usage. An origin's quota can be
> + * adjusted with #webkit_security_origin_set_database_quota.

I think it's best if we move the database stuff out of SecurityOrigin and move them to WebDatabase. Is that more realistic or developers really should expect that SecurityOrigin should know about database quotas and such?

> +
> +static void webkit_security_origin_finalize(GObject* object)
> +{
> +    WebKitSecurityOrigin* securityOrigin = WEBKIT_SECURITY_ORIGIN(object);
> +    WebKitSecurityOriginPrivate* priv = securityOrigin->priv;
> +
> +    if (!priv->disposed) {
> +        priv->coreOrigin->deref();
> +        g_hash_table_destroy(priv->webDatabases);
> +        priv->disposed = true;
> +    }

I think this should go to webkit_security_origin_dispose. 

Also I'm not sure if we should copy WebHistoryItem here. WebHistoryItem was meant to be shared between the WebBackForwardList and a future WebHistory. Does WebDatabase/WebSecurityOrigin have the same semantics? 

> +GHashTable* webkit_security_origins()
> +{
> +    static GHashTable* securityOrigins = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, g_object_unref);
> +    return securityOrigins;
> +}
> +
> +void webkit_security_origin_add(WebKitSecurityOrigin* origin, WebCore::SecurityOrigin* coreOrigin)
> +{
> +    g_return_if_fail(WEBKIT_IS_SECURITY_ORIGIN(origin));
> +
> +    GHashTable* table = webkit_security_origins();
> +    g_hash_table_insert(table, coreOrigin, origin);
> +}
> +
> +     g_object_class_install_property(gobject_class, PROP_PORT,
> +                                     g_param_spec_uint("port",
> +                                                       _("Port"),
> +                                                       _("The port of the security origin"),
> +                                                       0, G_MAXUINT, 0,
> +                                                       WEBKIT_PARAM_READABLE));

port can just be a gushort. So this should be G_MAXUSHORT.

> +                                      g_param_spec_uint64(
> +                                          "database-quota",

No need for the linebreak.

> +G_CONST_RETURN gchar* webkit_security_origin_get_protocol(WebKitSecurityOrigin* security_origin)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_SECURITY_ORIGIN(security_origin), NULL);
> +
> +    WebKitSecurityOriginPrivate* priv = security_origin->priv;
> +    WebCore::String protocol =  priv->coreOrigin->protocol();
> +
> +    if (!protocol.isEmpty()) {
> +        g_free(priv->protocol);
> +        priv->protocol = g_strdup(protocol.utf8().data());
> +        return priv->protocol;
> +    } else {
> +        return "";
> +    }

We usually do early exits so this would become: if protocol.isEmpty return "".

> +    if (!host.isEmpty()) {
> +        g_free(priv->host);
> +        priv->host = g_strdup(host.utf8().data());
> +        return priv->host;
> +    } else {
> +        return "";
> +    }

Same here.

> +WebKitWebDatabase* webkit_security_origin_get_database(WebKitSecurityOrigin* security_origin, const gchar* database_name)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_SECURITY_ORIGIN(security_origin), NULL);
> +
> +    WebKitSecurityOriginPrivate* priv = security_origin->priv;
> +    GHashTable* databaseHash = priv->webDatabases;
> +    WebKitWebDatabase* database = (WebKitWebDatabase*) g_hash_table_lookup(databaseHash, database_name);
> +
> +    if (!database) {
> +        database =  WEBKIT_WEB_DATABASE(g_object_new(WEBKIT_TYPE_WEB_DATABASE,
> +                                       "origin", security_origin,
> +                                       "name", database_name,
> +                                        NULL));
> +        g_hash_table_insert(databaseHash, g_strdup(database_name), database);
> +    }

This getter should return NULL if no WebDatabase exist for the security origin.

> +GList* webkit_security_origin_get_databases(WebKitSecurityOrigin* security_origin)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_SECURITY_ORIGIN(security_origin), NULL);
> +    GList* databases = NULL;
> +
> +#if ENABLE(DATABASE)
> +    WebCore::SecurityOrigin* coreOrigin = core(security_origin);
> +    Vector<WebCore::String> databaseNames;
> +
> +    if (!WebCore::DatabaseTracker::tracker().databaseNamesForOrigin(coreOrigin, databaseNames))
> +        return databases;

Please be more explicit by just returning NULL.

> +    for (unsigned i = 0; i < databaseNames.size(); ++i) {
> +        WebKitWebDatabase* database = webkit_security_origin_get_database(security_origin, databaseNames[i].utf8().data());
> +        databases = g_list_append(databases, database);

I'm not sure if we should be creating databases if they don't exist in security origin. Is it ok to create databases if they're not in origin?

> +
> +#ifndef __WEBKIT_SECURITY_ORIGIN_H__
> +#define __WEBKIT_SECURITY_ORIGIN_H__
> +

We don't usually add double underscores to the name.

> +WEBKIT_API guint64
> +webkit_security_origin_get_database_usage (WebKitSecurityOrigin* security_origin);
> +
> +WEBKIT_API guint64
> +webkit_security_origin_get_database_quota (WebKitSecurityOrigin* security_origin);
> +
> +WEBKIT_API void 
> +webkit_security_origin_set_database_quota (WebKitSecurityOrigin* security_origin, guint64 quota);
> +
> +WEBKIT_API WebKitWebDatabase*
> +webkit_security_origin_get_database       (WebKitSecurityOrigin* security_origin, const char* database_name);
> +
> +WEBKIT_API GList*
> +webkit_security_origin_get_databases      (WebKitSecurityOrigin* security_origin);

Not sure if these database methods should be here. Would it make sense to move the webkitwebdatabase?


> +#include "config.h"
> +#include "webkitwebdatabase.h"
> +#include "webkitprivate.h"

Please separate this two.

> +#include <glib/gi18n-lib.h>

Move this after the WebCore includes.

> + * SECTION:webkitwebdatabase
> + * @short_description: A WebKit web application database
> + *
> + * #WebKitWebDatabase is a representation of a Web Database database. The
> + * upcoming Web Database standard introduces support for SQL databases that web
> + * sites can create and access on a local computer through JavaScript.

Is it a standard or a proposed standard? Also it might be helpful to provide a link so devs can read more if they want to.

> + * To get access to all databases defined by a security origin, use
> + * #webkit_security_origin_get_databases. Each database has an internal
> + * name, as well as a user-friendly display name.

webkit_security_origin_get_databases -> webkit_database_from_security_origin? Also, we currently don't provide an API for the internal name so I'm not sure if it's worth mentioning here.

> + * 
> + * WebKit uses SQLite to create and access the local SQL databases. The location
> + * of a #WebKitWebDatabase can be accessed wth #webkit_web_database_get_path.
> + * You can configure the location of all databases with
> + * #webkit_set_database_directory_path.
> + * 
> + * For each database the web site can define an esimated size which can be

esimated -> estimated.

> + * accessed with #webkit_web_database_get_expected_size. The current size of the
> + * database in bytes is returned by #webkit_web_database_get_size.
> + * 
> + * For more information refer to the Web Database specification proposal at
> + *  http://dev.w3.org/html5/webdatabase
> + */
> +
> +using namespace WebKit;
> +
> +enum {
> +    PROP_0,
> +
> +    PROP_ORIGIN,
> +    PROP_NAME,
> +    PROP_DISPLAY_NAME,
> +    PROP_EXPECTED_SIZE,
> +    PROP_SIZE,
> +    PROP_PATH
> +};
> +
> +G_DEFINE_TYPE(WebKitWebDatabase, webkit_web_database, G_TYPE_OBJECT)
> +
> +struct _WebKitWebDatabasePrivate {
> +    WebKitSecurityOrigin* origin;
> +    gchar* name;
> +    gchar* display_name;
> +    gchar* path;
> +};
> +
> +static gchar* webkit_database_directory_path = NULL;
> +static guint64 webkit_default_database_quota = 5 * 1024 * 1024;
> +
> +#define WEBKIT_WEB_DATABASE_GET_PRIVATE(obj) (G_TYPE_INSTANCE_GET_PRIVATE((obj), WEBKIT_TYPE_WEB_DATABASE, WebKitWebDatabasePrivate))
> +
> +static void webkit_web_database_set_origin(WebKitWebDatabase* web_database, WebKitSecurityOrigin* security_origin);
> +
> +static void webkit_web_database_set_name(WebKitWebDatabase* web_database, const gchar* name);
> +
> +static void webkit_web_database_finalize(GObject* object)
> +{
> +    WebKitWebDatabase* webDatabase = WEBKIT_WEB_DATABASE(object);
> +    WebKitWebDatabasePrivate* priv = webDatabase->priv;
> +
> +    if (priv->origin)
> +        g_object_unref(priv->origin);
> +
> +    G_OBJECT_CLASS(webkit_web_database_parent_class)->finalize(object);
> +}
> +
> +static void webkit_web_database_set_property(GObject* object, guint prop_id, const GValue* value, GParamSpec* pspec)
> +{
> +    WebKitWebDatabase* web_database = WEBKIT_WEB_DATABASE(object);
> +
> +    switch (prop_id) {
> +    case PROP_ORIGIN:
> +        webkit_web_database_set_origin(web_database, WEBKIT_SECURITY_ORIGIN(g_value_get_object(value)));
> +        break;
> +    case PROP_NAME:
> +        webkit_web_database_set_name(web_database, g_value_get_string(value));
> +        break;
> +    default:
> +        G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> +        break;
> +    }
> +}
> +
> +static void webkit_web_database_get_property(GObject* object, guint prop_id, GValue* value, GParamSpec* pspec)
> +{
> +    WebKitWebDatabase* webDatabase = WEBKIT_WEB_DATABASE(object);
> +    WebKitWebDatabasePrivate* priv = webDatabase->priv;
> +
> +    switch (prop_id) {
> +    case PROP_ORIGIN:
> +        g_value_set_object(value, priv->origin);
> +        break;
> +    case PROP_NAME:
> +        g_value_set_string(value, webkit_web_database_get_name(webDatabase));
> +        break;
> +    case PROP_DISPLAY_NAME:
> +        g_value_set_string(value, webkit_web_database_get_display_name(webDatabase));
> +        break;
> +    case PROP_EXPECTED_SIZE:
> +        g_value_set_uint64(value, webkit_web_database_get_expected_size(webDatabase));
> +        break;
> +    case PROP_SIZE:
> +        g_value_set_uint64(value, webkit_web_database_get_size(webDatabase));
> +        break;
> +    case PROP_PATH:
> +        g_value_set_string(value, webkit_web_database_get_path(webDatabase));
> +        break;
> +    default:
> +        G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> +        break;
> +    }
> +}
> +
> +static void webkit_web_database_class_init(WebKitWebDatabaseClass* klass)
> +{
> +    GObjectClass* gobject_class = G_OBJECT_CLASS(klass);
> +    gobject_class->finalize = webkit_web_database_finalize;
> +    gobject_class->set_property = webkit_web_database_set_property;
> +    gobject_class->get_property = webkit_web_database_get_property;
> +
> +     /**
> +      * WebKitWebDatabase:origin:
> +      *
> +      * The security origin of the database.
> +      *
> +      * Since: 1.1.13
> +      */
> +     g_object_class_install_property(gobject_class, PROP_ORIGIN,
> +                                     g_param_spec_object("origin",
> +                                                         _("Origin"),
> +                                                         _("The security origin of the database"),
> +                                                         WEBKIT_TYPE_SECURITY_ORIGIN,
> +                                                         (GParamFlags) (G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY)));
> +
> +     /**
> +      * WebKitWebDatabase:name:
> +      *
> +      * The name of the Web Database database.
> +      *
> +      * Since: 1.1.13
> +      */
> +     g_object_class_install_property(gobject_class, PROP_NAME,
> +                                     g_param_spec_string("name",
> +                                                         _("Name"),
> +                                                         _("The name of the Web Database database"),
> +                                                         NULL,
> +                                                         (GParamFlags) (G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY)));
> +
> +     /**
> +      * WebKitWebDatabase:display-name:
> +      *
> +      * The display name of the Web Database database.
> +      *
> +      * Since: 1.1.13
> +      */
> +     g_object_class_install_property(gobject_class, PROP_DISPLAY_NAME,
> +                                     g_param_spec_string("display-name",
> +                                                         _("Display Name"),
> +                                                         _("The display name of the Web Stroage database"),
> +                                                         NULL,
> +                                                         WEBKIT_PARAM_READABLE));
> +
> +     /**
> +     * WebKitWebDatabase:expected-size:
> +     *
> +     * The expected size of the database in bytes as defined by the web author.
> +     *
> +     * Since: 1.1.13
> +     */
> +     g_object_class_install_property(gobject_class, PROP_EXPECTED_SIZE,
> +                                     g_param_spec_uint64(
> +                                         "expected-size",
> +                                         _("Expected Size"),
> +                                         _("The expected size of the Web Database database"),
> +                                         0, G_MAXUINT64, 0,
> +                                         WEBKIT_PARAM_READABLE));
> +     /**
> +     * WebKitWebDatabase:size:
> +     *
> +     * The current size of the database in bytes.
> +     *
> +     * Since: 1.1.13
> +     */
> +     g_object_class_install_property(gobject_class, PROP_SIZE,
> +                                     g_param_spec_uint64(
> +                                         "size",
> +                                         _("Size"),
> +                                         _("The current size of the Web Database database"),
> +                                         0, G_MAXUINT64, 0,
> +                                         WEBKIT_PARAM_READABLE));
> +     /**
> +      * WebKitWebDatabase:path:
> +      *
> +      * The filesystem path of the Web Database database.
> +      *
> +      * Since: 1.1.13
> +      */
> +     g_object_class_install_property(gobject_class, PROP_PATH,
> +                                     g_param_spec_string("path",
> +                                                         _("Path"),
> +                                                         _("The filesystem path of the Web Stroage database"),
> +                                                         NULL,
> +                                                         WEBKIT_PARAM_READABLE));
> +
> +    g_type_class_add_private(klass, sizeof(WebKitWebDatabasePrivate));
> +}
> +
> +static void webkit_web_database_init(WebKitWebDatabase* web_database)
> +{
> +    web_database->priv = WEBKIT_WEB_DATABASE_GET_PRIVATE(web_database);
> +}
> +
> +// Internal use only
> +static void webkit_web_database_set_origin(WebKitWebDatabase *web_database, WebKitSecurityOrigin *security_origin)
> +{
> +    g_return_if_fail(WEBKIT_IS_WEB_DATABASE(web_database));
> +    g_return_if_fail(WEBKIT_IS_SECURITY_ORIGIN(security_origin));
> +
> +    WebKitWebDatabasePrivate* priv = web_database->priv;
> +
> +    if (priv->origin)
> +        g_object_unref(priv->origin);
> +
> +    g_object_ref(security_origin);
> +    priv->origin = security_origin;
> +}
> +
> +static void webkit_web_database_set_name(WebKitWebDatabase* web_database, const gchar* name)
> +{
> +    g_return_if_fail(WEBKIT_IS_WEB_DATABASE(web_database));
> +
> +    WebKitWebDatabasePrivate* priv = web_database->priv;
> +    g_free(priv->name);
> +    priv->name = g_strdup(name);
> +}
> +
> +/**
> + * webkit_web_database_get_origin:
> + * @web_database: a #WebKitWebDatabase
> + *
> + * Returns the security origin of the #WebKitWebDatabase.
> + *
> + * Returns: the security origin of the database
> + *
> + * Since: 1.1.13
> + **/
> +WebKitSecurityOrigin* webkit_web_database_get_origin(WebKitWebDatabase* web_database)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_DATABASE(web_database), NULL);
> +    WebKitWebDatabasePrivate* priv = web_database->priv;
> +
> +    return priv->origin;
> +}
> +
> +/**
> + * webkit_web_database_get_name:
> + * @web_database: a #WebKitWebDatabase
> + *
> + * Returns the canonical name of the #WebKitWebDatabase.
> + *
> + * Returns: the name of the database
> + *
> + * Since: 1.1.13
> + **/
> +G_CONST_RETURN gchar* webkit_web_database_get_name(WebKitWebDatabase* web_database)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_DATABASE(web_database), NULL);
> +    WebKitWebDatabasePrivate* priv = web_database->priv;
> +
> +    return priv->name;
> +}
> +
> +/**
> + * webkit_web_database_get_display_name:
> + * @web_database: a #WebKitWebDatabase
> + *
> + * Returns the name of the #WebKitWebDatabase as seen by the user.
> + *
> + * Returns: the name of the database as seen by the user.
> + *
> + * Since: 1.1.13
> + **/
> +G_CONST_RETURN gchar* webkit_web_database_get_display_name(WebKitWebDatabase* web_database)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_DATABASE(web_database), NULL);
> +
> +#if ENABLE(DATABASE)
> +    WebKitWebDatabasePrivate* priv = web_database->priv;
> +    WebCore::DatabaseDetails details = WebCore::DatabaseTracker::tracker().detailsForNameAndOrigin(priv->name, core(priv->origin));
> +    WebCore::String displayName =  details.displayName();
> +
> +    if (!displayName.isEmpty()) {
> +        g_free(priv->display_name);
> +        priv->display_name = g_strdup(displayName.utf8().data());
> +        return priv->display_name;
> +    } else {
> +        return "";
> +    }
> +#else
> +    return "";
> +#endif
> +}
> +
> +/**
> + * webkit_web_database_get_expected_size:
> + * @web_database: a #WebKitWebDatabase
> + *
> + * Returns the expected size of the #WebKitWebDatabase in bytes as defined by the
> + * web author. The Web Database standard allows web authors to specify an expected
> + * size of the database to optimize the user experience.
> + *
> + * Returns: the expected size of the database in bytes
> + *
> + * Since: 1.1.13
> + **/
> +guint64 webkit_web_database_get_expected_size(WebKitWebDatabase* web_database)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_DATABASE(web_database), 0);
> +
> +#if ENABLE(DATABASE)
> +    WebKitWebDatabasePrivate* priv = web_database->priv;
> +    WebCore::DatabaseDetails details = WebCore::DatabaseTracker::tracker().detailsForNameAndOrigin(priv->name, core(priv->origin));
> +    return details.expectedUsage();
> +#else
> +    return 0;
> +#endif
> +}
> +
> +/**
> + * webkit_web_database_get_size:
> + * @web_database: a #WebKitWebDatabase
> + *
> + * Returns the actual size of the #WebKitWebDatabase space on disk in bytes.
> + *
> + * Returns: the actual size of the database in bytes
> + *
> + * Since: 1.1.13
> + **/
> +guint64 webkit_web_database_get_size(WebKitWebDatabase* web_database)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_DATABASE(web_database), 0);
> +
> +#if ENABLE(DATABASE)
> +    WebKitWebDatabasePrivate* priv = web_database->priv;
> +    WebCore::DatabaseDetails details = WebCore::DatabaseTracker::tracker().detailsForNameAndOrigin(priv->name, core(priv->origin));
> +    return details.currentUsage();
> +#else
> +    return 0;
> +#endif
> +}
> +
> +/**
> + * webkit_web_database_get_path:
> + * @web_database: a #WebKitWebDatabase
> + *
> + * Returns the path to the #WebKitWebDatabase file on disk.
> + *
> + * Returns: the filesystem path of the database
> + *
> + * Since: 1.1.13
> + **/
> +G_CONST_RETURN gchar* webkit_web_database_get_path(WebKitWebDatabase* web_database)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_DATABASE(web_database), NULL);
> +
> +#if ENABLE(DATABASE)
> +    WebKitWebDatabasePrivate* priv = web_database->priv;
> +    WebCore::String coreName = WebCore::String::fromUTF8(priv->name);
> +    WebCore::String corePath = WebCore::DatabaseTracker::tracker().fullPathForDatabase(core(priv->origin), coreName);
> +
> +    if (!corePath.isEmpty()) {
> +        g_free(priv->path);
> +        priv->path = g_strdup(corePath.utf8().data());
> +        return priv->path;
> +    } else {
> +        return "";
> +    }
> +#else
> +    return "";
> +#endif
> +}
> +
> +/**
> + * webkit_web_database_remove:
> + * @web_database: a #WebKitWebDatabase
> + *
> + * Removes the #WebKitWebDatabase from its security origin and destroys all data
> + * stored in the database.
> + *
> + * Since: 1.1.13
> + **/
> +void webkit_web_database_remove(WebKitWebDatabase* web_database)
> +{
> +    g_return_if_fail(WEBKIT_IS_WEB_DATABASE(web_database));
> +
> +#if ENABLE(DATABASE)
> +    WebKitWebDatabasePrivate* priv = web_database->priv;
> +    WebCore::DatabaseTracker::tracker().deleteDatabase(core(priv->origin), priv->name);
> +#endif
> +}
> +
> +/**
> + * webkit_remove_all_web_databases:
> + *
> + * Removes all web databases from the current database directory path.
> + *
> + * Since: 1.1.13
> + **/
> +void webkit_remove_all_web_databases()
> +{
> +#if ENABLE(DATABASE)
> +    WebCore::DatabaseTracker::tracker().deleteAllDatabases();
> +#endif
> +}
> +
> +/**
> + * webkit_get_database_directory_path:
> + *
> + * Returns the current path to the directory WebKit will write Web 
> + * Database databases. By default this path will be in the user data
> + * directory.
> + *
> + * Returns: the current database directory path
> + *
> + * Since: 1.1.13
> + **/
> +G_CONST_RETURN gchar* webkit_get_database_directory_path()
> +{
> +#if ENABLE(DATABASE)
> +    WebCore::String path = WebCore::DatabaseTracker::tracker().databaseDirectoryPath();
> +    if (!path.isEmpty()) {
> +        g_free(webkit_database_directory_path);
> +        webkit_database_directory_path = g_strdup(path.utf8().data());
> +        return webkit_database_directory_path;
> +    } else {
> +        return "";
> +    }
> +#else
> +    return "";
> +#endif
> +}
> +
> +/**
> + * webkit_set_database_directory_path:
> + * @path: the new database directory path
> + *
> + * Sets the current path to the directory WebKit will write Web 
> + * Database databases. 
> + *
> + * Since: 1.1.13
> + **/
> +void webkit_set_database_directory_path(const gchar* path)
> +{
> +#if ENABLE(DATABASE)
> +    WebCore::String corePath = WebCore::String::fromUTF8(path);
> +    WebCore::DatabaseTracker::tracker().setDatabaseDirectoryPath(corePath);
> +
> +    g_free(webkit_database_directory_path);
> +    webkit_database_directory_path = g_strdup(corePath.utf8().data());
> +#endif
> +}
> +
> +/**
> + * webkit_get_default_database_quota:
> + *
> + * Returns the default quota for Web Database databases. By default
> + * this value is 5MB.
> + 
> + * Returns: the current default database quota in bytes
> + *
> + * Since: 1.1.13
> + **/
> +guint64 webkit_get_default_database_quota()
> +{
> +    return webkit_default_database_quota;
> +}
> +
> +/**
> + * webkit_set_default_database_quota:
> + * @default_quota: the new defaulta database quota

default is mispelled here.

> + *
> + * Sets the current path to the directory WebKit will write Web 
> + * Database databases. 
> + *
> + * Since: 1.1.13
> + **/
> +void webkit_set_default_database_quota(guint64 default_quota)
> +{
> +    webkit_default_database_quota = default_quota;
> +}



> +
> +WEBKIT_API GType
> +webkit_web_database_get_type (void);
> +
> +WEBKIT_API WebKitSecurityOrigin*
> +webkit_web_database_get_origin         (WebKitWebDatabase* web_database);

There should be a space between the type and the asterisk.

> +
> +WEBKIT_API void
> +webkit_web_database_remove             (WebKitWebDatabase* web_database);
> +

I think we need to distinguish between web_database and just database. Or maybe just rename database to databases (or something like that) to signify multiple databases?

> +WebKitSecurityOrigin* webkit_web_frame_get_origin(WebKitWebFrame* frame)
> +{
> +    WebKitWebFramePrivate* priv = frame->priv;
> +    if (!priv->coreFrame || !priv->coreFrame->document())
> +        return NULL;
> +
> +    WebKitSecurityOrigin* origin = kit(priv->coreFrame->document()->securityOrigin());
> +    return origin;

No need for the intermediate local variable here.

> +    /**
> +     * WebKitWebView::database-quota-exceeded
> +     * @web_view: the object which received the signal
> +     * @frame: the relevant frame
> +     * @database: the #WebKitWebDatabase which exceeded the quota of its #WebKitSecurityOrigin
> +     * @param: a #GHashTable with additional attributes (strings)
> +     *

What's the @param for? Also, I think it would be good nice if the signal handler can return a boolean stating that either it actioned the exceeded-quota event or not. This way, if the signal wasn't handled we can fallback to the default of setting a quota. If they handled the signal then maybe we don't need to reset the quota?

> +     * The #WebKitWebView::database-exceeded-quota signal will be emitted when
> +     * a Web Database exceeds the quota of its security origin. This signal
> +     * may be used to increase the size of the quota before the originating
> +     * operation fails.
> +     *
> +     * Since: 1.1.13
> +     */
> +    webkit_web_view_signals[PLUGIN_WIDGET] = g_signal_new("database-quota-exceeded",

Why PLUGIN_WIDGET?

Other parts look fine to me. Need some stakeholders to look into this as well.

Thanks for finishing the patch!

[Martin Robinson]

Created attachment 34906 [details]
Update patch rev2

[Martin Robinson]

Thanks for the great feedback, Jan!

> I think it's best if we move the database stuff out of SecurityOrigin and move
> them to WebDatabase. Is that more realistic or developers really should expect
> that SecurityOrigin should know about database quotas and such?

In this case, I followed the design of the Mac and QT ports which expose this functionality as methods on a security origin. I'm certainly not opposed to moving it elsewhere. :)

> Also I'm not sure if we should copy WebHistoryItem here. WebHistoryItem was
> meant to be shared between the WebBackForwardList and a future WebHistory. Does
> WebDatabase/WebSecurityOrigin have the same semantics? 

Really, the issue in this case is that I haven't figured out a good way to manage the lifetime of WebKitSecurityOrigin and WebKitWebDatabase. I'm using the hash in this case to prevent them from being allocated needlessly if they are required twice. I haven't figured out a way yet to keep them from leaking. This is probably the main reason I haven't marked this patch with r?.

> webkit_security_origin_get_databases -> webkit_database_from_security_origin?
> Also, we currently don't provide an API for the internal name so I'm not sure
> if it's worth mentioning here.

The other ports don't expose a method like this, so I've made it private and renamed it to something like what you suggested. This should also prevent the problem of trying to create a WebKitWebDatabase for a non-existent database. I didn't see a great way check manually in WebCore, but perhaps I missed something.

> I think we need to distinguish between web_database and just database. Or maybe
> just rename database to databases (or something like that) to signify multiple
> databases?

I'm not sure I understand this.

[Martin Robinson]

Created attachment 38794 [details]
Fixed memory issues

[Martin Robinson]

Created attachment 38795 [details]
This time with ChangeLog entries

[Gustavo Noronha (kov)]

Comment on attachment 38795 [details]
This time with ChangeLog entries

> +    WebKitSecurityOrigin* origin = webkit_web_frame_get_origin(webFrame);
> +    WebKitWebDatabase* webDatabase = webkit_web_database_from_security_origin(origin, databaseName.utf8().data());
> +    g_signal_emit_by_name(webView, "database-quota-exceeded", webFrame, webDatabase);

I am not sure we need this signal, though it may be good for convenience, one could just watch a usage property to see if it's past the quota, I guess.

> +        gchar* protocol;

Does the spec say protocol, or scheme?

> +void webkit_security_origin_add(WebKitSecurityOrigin* origin, WebCore::SecurityOrigin* coreOrigin)
> +{
> +    g_return_if_fail(WEBKIT_IS_SECURITY_ORIGIN(origin));
> +
> +    GHashTable* table = webkit_security_origins();
> +    g_hash_table_insert(table, coreOrigin, origin);
> +}

This should be static, though I don't see much point in this function existing. The g_return_if_fail check is only any good in APIs, and you could just do g_hash_table_inert(webkit_security_origins(), coreOrigin, origin); anywhere you need it.

> +    if (protocol.isEmpty()) {
> +        return "";
> +    } else {

No need for bracers

> +        g_free(priv->protocol);
> +        priv->protocol = g_strdup(protocol.utf8().data());
> +        return priv->protocol;

I don't see much point in using g_free/g_strdup here. Since it's const, just use return protocol.utf8().data(), perhaps? Is there a problem you noticed with doing that?

> +    WebCore::String host =  priv->coreOrigin->host();
> +
> +    if (host.isEmpty()) {
> +        return "";
> +    } else {
> +        g_free(priv->host);
> +        priv->host = g_strdup(host.utf8().data());
> +        return priv->host;

Also, we don't usually create variables for these kinds of uses. Just use the whole priv->coreOrigin->host().utf8().data() thing where you are using the 'host' variable.

> +WebKitSecurityOrigin* WebKit::kit(WebCore::SecurityOrigin* coreOrigin)
> +{
> +    g_return_val_if_fail(coreOrigin, NULL);

Better use an ASSERT here.

> +WebCore::SecurityOrigin* WebKit::core(WebKitSecurityOrigin* security_origin)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_SECURITY_ORIGIN(security_origin), NULL);

Same here.

> +WebKitWebDatabase* webkit_web_database_from_security_origin(WebKitSecurityOrigin* security_origin, const gchar* database_name)
> +{

I think a better API would be webkit_security_origin_get_web_database.

> + * To get access to all databases defined by a security origin, use
> + * #webkit_security_origin_get_databases. Each database has a canonical
> + * name, as well as a user-friendly display name.

That should probably be webkit_security_origin_get_all_web_databases.

> + * WebKit uses SQLite to create and access the local SQL databases. The location
> + * of a #WebKitWebDatabase can be accessed wth #webkit_web_database_get_path.
> + * You can configure the location of all databases with
> + * #webkit_set_database_directory_path.

That's pretty inconsistent. I do think these two should be webkit_get/set_web_database_path, since they are not acting on a specific web_database, or are they?

> +     /**
> +      * WebKitWebDatabase:path:
> +      *
> +      * The filesystem path of the Web Database database.

The path of the specific file? I am not sure what this property is supposed to represent. It looks like we would want a filename here, that would be joined with the global databases directory obtained by a webkit_get_web_database_path call.

> +     g_object_class_install_property(gobject_class, PROP_PATH,
> +                                     g_param_spec_string("path",
> +                                                         _("Path"),
> +                                                         _("The filesystem path of the Web Stroage database"),

Typo.

> +void webkit_remove_all_web_databases()
[...]
> +G_CONST_RETURN gchar* webkit_get_database_directory_path()
[...]
> +void webkit_set_database_directory_path(const gchar* path)
[...]
> +guint64 webkit_get_default_database_quota()
[...]
> +void webkit_set_default_database_quota(guint64 default_quota)

These are looking good, but I think we want 'web_database' in all of them for consistency.

> +WebKitSecurityOrigin* webkit_web_frame_get_origin(WebKitWebFrame* frame)

get_security_origin

All the DRT modifications look good, I only think you are missing setting a temporary directory as the place where webkit will store the databases? Running remove_all_web_databases() on my user's real database storage, when running the tests will make me unhappy =). A few things to be addressed, so r- for now. Thanks for working on this!

[Martin Robinson]

(In reply to comment #9)

Thanks for the feedback. I have a few comments below and should be able to submit another patch soon fixing the other issues.

> (From update of attachment 38795 [details])
> > +    WebKitSecurityOrigin* origin = webkit_web_frame_get_origin(webFrame);
> > +    WebKitWebDatabase* webDatabase = webkit_web_database_from_security_origin(origin, databaseName.utf8().data());
> > +    g_signal_emit_by_name(webView, "database-quota-exceeded", webFrame, webDatabase);
> 
> I am not sure we need this signal, though it may be good for convenience, one
> could just watch a usage property to see if it's past the quota, I guess.

This signal is for parity with other ports' delegates and it's needed by DRT. I'm not sure if there is a good way to track database usage as a GObject property with the current WebCore API, but I could be wrong.

> > +        gchar* protocol;
> Does the spec say protocol, or scheme?

The property accessor on the WebCore SecurityOrigin object calls it protocol, so I'll try to make sure that's used everywhere.

> > +        g_free(priv->protocol);
> > +        priv->protocol = g_strdup(protocol.utf8().data());
> > +        return priv->protocol;
> 
> I don't see much point in using g_free/g_strdup here. Since it's const, just
> use return protocol.utf8().data(), perhaps? Is there a problem you noticed with
> doing that?

Hmm. Seems that utf8() returned a fresh CString every time. If so, the destructor for that object will be called after that line. Perhaps the text data is stored in some buffer outside the CString as well, but can we really depend on that happening? I used this particular pattern, because it was used for webkit_web_frame_get_name.
 
> > + * WebKit uses SQLite to create and access the local SQL databases. The location
> > + * of a #WebKitWebDatabase can be accessed wth #webkit_web_database_get_path.
> > + * You can configure the location of all databases with
> > + * #webkit_set_database_directory_path.
> 
> That's pretty inconsistent. I do think these two should be
> webkit_get/set_web_database_path, since they are not acting on a specific
> web_database, or are they?

The first gets the full path to the SQLite file for the database and the latter gets the path to the parent directory for all SQLite web database files. Perhaps there is a way I can clarify this?

> > +     /**
> > +      * WebKitWebDatabase:path:
> > +      *
> > +      * The filesystem path of the Web Database database.
> 
> The path of the specific file? I am not sure what this property is supposed to
> represent. It looks like we would want a filename here, that would be joined
> with the global databases directory obtained by a webkit_get_web_database_path
> call.

We may have to return the full path. I'm not sure the API exposes the way to get from /path/to/all/databases to /path/to/all/databases/various/subdirectories/0000000000000002.db-journal. Subdirectories are used to separate database files by origin. I'll try to improve the doc here though.

[Xan Lopez]

(In reply to comment #10)
> > I don't see much point in using g_free/g_strdup here. Since it's const, just
> > use return protocol.utf8().data(), perhaps? Is there a problem you noticed with
> > doing that?
> 
> Hmm. Seems that utf8() returned a fresh CString every time. If so, the
> destructor for that object will be called after that line. Perhaps the text
> data is stored in some buffer outside the CString as well, but can we really
> depend on that happening? I used this particular pattern, because it was used
> for webkit_web_frame_get_name.

This is correct, the data returned by utf8() needs to be dupped before it's returned to the user. If the return type is 'char*' it can be returned as is, if it's 'const char*' the trick we use, like here, is to store it in some private structure.

[Gustavo Noronha (kov)]

(In reply to comment #10)
> (In reply to comment #9)
> 
> Thanks for the feedback. I have a few comments below and should be able to
> submit another patch soon fixing the other issues.

OK, sorry I didn't reply on IRC, I reviewed the patch and went to sleep, straight =)

> This signal is for parity with other ports' delegates and it's needed by DRT.
> I'm not sure if there is a good way to track database usage as a GObject
> property with the current WebCore API, but I could be wrong.

Yeah, that makes sense.
 
> > > + * WebKit uses SQLite to create and access the local SQL databases. The location
> > > + * of a #WebKitWebDatabase can be accessed wth #webkit_web_database_get_path.
> > > + * You can configure the location of all databases with
> > > + * #webkit_set_database_directory_path.
> > 
> > That's pretty inconsistent. I do think these two should be
> > webkit_get/set_web_database_path, since they are not acting on a specific
> > web_database, or are they?
> 
> The first gets the full path to the SQLite file for the database and the latter
> gets the path to the parent directory for all SQLite web database files.
> Perhaps there is a way I can clarify this?

I think you can say 'specific file' in the first one. 

> > The path of the specific file? I am not sure what this property is supposed to
> > represent. It looks like we would want a filename here, that would be joined
> > with the global databases directory obtained by a webkit_get_web_database_path
> > call.
> 
> We may have to return the full path. I'm not sure the API exposes the way to
> get from /path/to/all/databases to
> /path/to/all/databases/various/subdirectories/0000000000000002.db-journal.
> Subdirectories are used to separate database files by origin. I'll try to
> improve the doc here though.

I see. Yeah, I would suggest using 'filename' instead of path, and say in the documentation that it's the absolute filename.

[Martin Robinson]

Created attachment 38847 [details]
Update patch rev3

I've tried to fix all the issues that kov mentioned as well as changing the 'path' property of a WebDatabase to the 'filename' property.

[Xan Lopez]

Comment on attachment 38847 [details]
Update patch rev3

> +static void webkit_security_origin_finalize(GObject* object)
> +{
> +    WebKitSecurityOrigin* securityOrigin = WEBKIT_SECURITY_ORIGIN(object);
> +    WebKitSecurityOriginPrivate* priv = securityOrigin->priv;
> +
> +    g_free(priv->protocol);
> +    g_free(priv->host);
> +
> +    G_OBJECT_CLASS(securityOrigin)->finalize(object);

You have to chain to your parent class, not to yourself! I believe this should enter an infinite loop, does it actually work?

> +}
> +
> +static void webkit_security_origin_dispose(GObject* object)
> +{
> +    WebKitSecurityOrigin* securityOrigin = WEBKIT_SECURITY_ORIGIN(object);
> +    WebKitSecurityOriginPrivate* priv = securityOrigin->priv;
> +
> +    if (!priv->disposed) {
> +        priv->coreOrigin->deref();
> +        g_hash_table_destroy(priv->webDatabases);
> +        priv->disposed = true;
> +    }
> +
> +    G_OBJECT_CLASS(securityOrigin)->dispose(object);

Same thing here.

> +static void webkit_security_origin_class_init(WebKitSecurityOriginClass* klass)
> +{
> +    GObjectClass* gobject_class = G_OBJECT_CLASS(klass);

gobjectClass



> +
> +      /**
> +      * WebKitSecurityOrigin:web-database-usage:
> +      *
> +      * The cumulative size of all web databases in the security origin in bytes.
> +      *
> +      * Since: 1.1.13
> +      */
> +      g_object_class_install_property(gobject_class, PROP_DATABASE_USAGE,
> +                                      g_param_spec_uint64("web-database-usage",
> +                                          _("Web Database Usage"),
> +                                          _("The cumulative size of all web databases in the security origin"),
> +                                          0, G_MAXUINT64, 0,
> +                                          WEBKIT_PARAM_READABLE));

The indentation is off here.

> +      /**
> +      * WebKitSecurityOrigin:web-database-quota:
> +      *
> +      * The web database qouta of the security origin in bytes.
> +      *
> +      * Since: 1.1.13
> +      */
> +      g_object_class_install_property(gobject_class, PROP_DATABASE_QUOTA,
> +                                      g_param_spec_uint64("web-database-quota",
> +                                          _("Web Database Quota"),
> +                                          _("The web database quota of the security origin in bytes"),
> +                                          0, G_MAXUINT64, 0,
> +                                          WEBKIT_PARAM_READWRITE));

Same.

> +
> +/**
> + * webkit_security_origin_get_protocol:
> + * @security_origin: a #WebKitSecurityOrigin
> + *
> + * Returns the protocol for the security origin.
> + *
> + * Returns: the protocol for the security origin
> + *
> + * Since: 1.1.13
> + **/
> +G_CONST_RETURN gchar* webkit_security_origin_get_protocol(WebKitSecurityOrigin* security_origin)

securityOrigin for the parameter

> +{
> +    g_return_val_if_fail(WEBKIT_IS_SECURITY_ORIGIN(security_origin), NULL);
> +
> +    WebKitSecurityOriginPrivate* priv = security_origin->priv;
> +    WebCore::String protocol =  priv->coreOrigin->protocol();
> +
> +    if (protocol.isEmpty())
> +        return "";
> +
> +    g_free(priv->protocol);
> +    priv->protocol = g_strdup(protocol.utf8().data());
> +    return priv->protocol;
> +}
> +
> +/**
> + * webkit_security_origin_get_host:
> + * @security_origin: a #WebKitSecurityOrigin
> + *
> + * Returns the hostname for the security origin.
> + *
> + * Returns: the hostname for the security origin
> + *
> + * Since: 1.1.13
> + **/
> +G_CONST_RETURN gchar* webkit_security_origin_get_host(WebKitSecurityOrigin* security_origin)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_SECURITY_ORIGIN(security_origin), NULL);
> +
> +    WebKitSecurityOriginPrivate* priv = security_origin->priv;
> +    WebCore::String host =  priv->coreOrigin->host();
> +
> +    if (host.isEmpty()) {
> +        return "";
> +    } else {
> +        g_free(priv->host);
> +        priv->host = g_strdup(host.utf8().data());
> +        return priv->host;
> +    }
> +}

Mmm, can't the host and protocol properties be cached? Or can they actually between each call?

> +
> +WebKitWebDatabase* webkit_security_origin_get_web_database(WebKitSecurityOrigin* security_origin, const gchar* database_name)

and databaseName here.

> +{
> +    g_return_val_if_fail(WEBKIT_IS_SECURITY_ORIGIN(security_origin), NULL);
> +
> +    WebKitSecurityOriginPrivate* priv = security_origin->priv;
> +    GHashTable* databaseHash = priv->webDatabases;
> +    WebKitWebDatabase* database = (WebKitWebDatabase*) g_hash_table_lookup(databaseHash, database_name);
> +
> +    if (!database) {
> +        database =  WEBKIT_WEB_DATABASE(g_object_new(WEBKIT_TYPE_WEB_DATABASE,
> +                                       "security-origin", security_origin,
> +                                       "name", database_name,
> +                                        NULL));
> +        g_hash_table_insert(databaseHash, g_strdup(database_name), database);
> +    }
> +
> +    return database;
> +}
> +

> +
> +/**
> + * SECTION:webkitwebdatabase
> + * @short_description: A WebKit web application database
> + *
> + * #WebKitWebDatabase is a representation of a Web Database database. The
> + * proposed Web Database standard introduces support for SQL databases that web
> + * sites can create and access on a local computer through JavaScript.
> + *
> + * To get access to all databases defined by a security origin, use
> + * #webkit_security_origin_get_databases. Each database has a canonical
> + * name, as well as a user-friendly display name.
> + * 
> + * WebKit uses SQLite to create and access the local SQL databases. The location
> + * of a #WebKitWebDatabase can be accessed wth #webkit_web_database_get_filename.
> + * You can configure the location of all databases with
> + * #webkit_set_database_directory_path.
> + * 
> + * For each database the web site can define an estimated size which can be
> + * accessed with #webkit_web_database_get_expected_size. The current size of the
> + * database in bytes is returned by #webkit_web_database_get_size.
> + * 
> + * For more information refer to the Web Database specification proposal at
> + * http://dev.w3.org/html5/webdatabase
> + */

Awesome documentation!

> +static void webkit_web_database_finalize(GObject* object)
> +{
> +    WebKitWebDatabase* webDatabase = WEBKIT_WEB_DATABASE(object);
> +    WebKitWebDatabasePrivate* priv = webDatabase->priv;
> +
> +    if (priv->origin)
> +        g_object_unref(priv->origin);

This should be done in dispose, not in finalize.

> +
> +    g_free(priv->name);
> +    g_free(priv->display_name);
> +    g_free(priv->filename);
> +
> +    G_OBJECT_CLASS(webkit_web_database_parent_class)->finalize(object);
> +}
> +

Also you need to s/1.1.13/1.1.14/ everywhere :)

[Xan Lopez]

Comment on attachment 38847 [details]
Update patch rev3

Marking r- while we wait for the next patch.

[Martin Robinson]

Created attachment 38925 [details]
Update patch rev4

[Martin Robinson]

(In reply to comment #14)
> > +    G_OBJECT_CLASS(securityOrigin)->finalize(object);
> 
> You have to chain to your parent class, not to yourself! I believe this should
> enter an infinite loop, does it actually work?

Ouch. Thank you for catching that. I believe it was a search/replace gone bad.

> and databaseName here.

I've switched everything to camelCase and am caching the host and protocol portions of the security origin.

> Also you need to s/1.1.13/1.1.14/ everywhere :)

Done. :)

[Xan Lopez]

Comment on attachment 38925 [details]
Update patch rev4

> +GHashTable* webkit_security_origins()
> +{
> +    static GHashTable* securityOrigins = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, g_object_unref);
> +    return securityOrigins;
> +}

This function should be static too.


> +G_CONST_RETURN gchar* webkit_security_origin_get_protocol(WebKitSecurityOrigin* securityOrigin)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_SECURITY_ORIGIN(securityOrigin), NULL);
> +
> +    WebKitSecurityOriginPrivate* priv = securityOrigin->priv;
> +    WebCore::String protocol =  priv->coreOrigin->protocol();
> +
> +    if (protocol.isEmpty())
> +        return "";
> +
> +    if (!priv->protocol)
> +        priv->protocol = g_strdup(protocol.utf8().data());
> +
> +    return priv->protocol;
> +}

Idle thought, shouldn't you just always cache the value, even if it's "", and then return that without going to the core object? Same for host.

> +WebKitSecurityOrigin* WebKit::kit(WebCore::SecurityOrigin* coreOrigin)
> +{
> +    ASSERT(coreOrigin);
> +
> +    GHashTable* table = webkit_security_origins();
> +    WebKitSecurityOrigin* origin = (WebKitSecurityOrigin*) g_hash_table_lookup(table, coreOrigin);
> +
> +    if (!origin) {
> +        origin = WEBKIT_SECURITY_ORIGIN(g_object_new(WEBKIT_TYPE_SECURITY_ORIGIN, NULL));
> +        origin->priv->coreOrigin = coreOrigin;
> +        g_hash_table_insert(webkit_security_origins(), coreOrigin, origin);

You could use 'table' here instead of calling webkit_security_origins() again.

> +    }
> +
> +    return origin;
> +}
> +
> +


> +static void webkit_web_database_class_init(WebKitWebDatabaseClass* klass)
> +{
> +    GObjectClass* gobjectClass = G_OBJECT_CLASS(klass);
> +    gobjectClass->finalize = webkit_web_database_finalize;
> +    gobjectClass->set_property = webkit_web_database_set_property;
> +    gobjectClass->get_property = webkit_web_database_get_property;

You are not setting dispose here :)

Except for the last point it's all pretty minor/silly stuff, so you have my 1/2 r+ if you fix it. Now you need another 1/2 from other reviewer since this adds new API.

[Martin Robinson]

Created attachment 39037 [details]
Update patch rev5

Added a patch fixing the issues that you mentioned. Thanks again for taking the time to review this. :)

[Jan Alonzo]

Comment on attachment 39037 [details]
Update patch rev5

> diff --git a/GNUmakefile.am b/GNUmakefile.am
> index 53a04cd..41806e4 100644
> --- a/GNUmakefile.am
> +++ b/GNUmakefile.am
> @@ -331,6 +331,7 @@ webkitgtk_h_api += \
>  	$(srcdir)/WebKit/gtk/webkit/webkitwebsettings.h \
>  	$(srcdir)/WebKit/gtk/webkit/webkitwebwindowfeatures.h \
>  	$(srcdir)/WebKit/gtk/webkit/webkitwebview.h \
> +	$(srcdir)/WebKit/gtk/webkit/webkitwebdatabase.h \
>  	$(top_builddir)/WebKit/gtk/webkit/webkitversion.h
>  
>  webkitgtk_built_sources += \
> @@ -372,6 +373,10 @@ webkitgtk_sources += \
>  	WebKit/gtk/webkit/webkitwebnavigationaction.cpp \
>  	WebKit/gtk/webkit/webkitwebpolicydecision.cpp \
>  	WebKit/gtk/webkit/webkitwebresource.cpp \
> +	WebKit/gtk/webkit/webkitwebdatabase.cpp \
> +	WebKit/gtk/webkit/webkitwebdatabase.h \
> +	WebKit/gtk/webkit/webkitsecurityorigin.cpp \
> +	WebKit/gtk/webkit/webkitsecurityorigin.h \

Should the header be in the API section?

Other than that looks great! r=me.

[Xan Lopez]

(In reply to comment #20)
> (From update of attachment 39037 [details])
> > diff --git a/GNUmakefile.am b/GNUmakefile.am
> > index 53a04cd..41806e4 100644
> > --- a/GNUmakefile.am
> > +++ b/GNUmakefile.am
> > @@ -331,6 +331,7 @@ webkitgtk_h_api += \
> >  	$(srcdir)/WebKit/gtk/webkit/webkitwebsettings.h \
> >  	$(srcdir)/WebKit/gtk/webkit/webkitwebwindowfeatures.h \
> >  	$(srcdir)/WebKit/gtk/webkit/webkitwebview.h \
> > +	$(srcdir)/WebKit/gtk/webkit/webkitwebdatabase.h \
> >  	$(top_builddir)/WebKit/gtk/webkit/webkitversion.h
> >  
> >  webkitgtk_built_sources += \
> > @@ -372,6 +373,10 @@ webkitgtk_sources += \
> >  	WebKit/gtk/webkit/webkitwebnavigationaction.cpp \
> >  	WebKit/gtk/webkit/webkitwebpolicydecision.cpp \
> >  	WebKit/gtk/webkit/webkitwebresource.cpp \
> > +	WebKit/gtk/webkit/webkitwebdatabase.cpp \
> > +	WebKit/gtk/webkit/webkitwebdatabase.h \
> > +	WebKit/gtk/webkit/webkitsecurityorigin.cpp \
> > +	WebKit/gtk/webkit/webkitsecurityorigin.h \
> 
> Should the header be in the API section?
> 
> Other than that looks great! r=me.

I have pushed this with this fix and one "Since" tag that was missing. Landed in r48101, closing.