[Webkit-unassigned] [Bug 27899] [Gtk] Expose a database API

[bugzilla-daemon at webkit.org]

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


Gustavo Noronha (kov) <gns at gnome.org> changed:

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




--- Comment #9 from Gustavo Noronha (kov) <gns at gnome.org>  2009-08-30 21:08:18 PDT ---
(From update of attachment 38795)
> +    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!

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