[Webkit-unassigned] [Bug 96476] [WK2][GTK] Implement new Favicons API
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Sep 12 06:34:20 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=96476
--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com> 2012-09-12 06:34:45 PST ---
(From update of attachment 163600)
View in context: https://bugs.webkit.org/attachment.cgi?id=163600&action=review
The new public header WebKitFaviconDatabase.h should be added to the main header file webkit2.h. And all new symbols should be added to the webkit2gtk-section.txt file.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:39
> +static void webkitFaviconDatabaseProcessPendingIconsForURI(WebKitFaviconDatabase*, const String&);
> +static void webkitFaviconDatabaseGetIconSurfaceCancelled(GCancellable*, GSimpleAsyncResult*);
I think these were needed in wk1 because they were used by the PendingRequest class. Now we can just move the functions before they are used and avoid the prototypes.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:85
> +static void webkit_favicon_database_class_init(WebKitFaviconDatabaseClass* findClass)
findClass?
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:93
> + * @page_url: the URI of the Web page containing the icon.
Use page_uri instead of page_url
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:97
> + * Note that this signal carries the URI of the frame that loads
> + * the icon, while #WebKitWebView::icon-ready provides the URI
> + * of the favicon.
I guess this is copied from wk1, there's not icon-ready signal in WebKitWebView for wk2. Here you should explain that this is emitted when the icon has been loaded from the network or imported from the database.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:102
> + (GSignalFlags)G_SIGNAL_RUN_LAST,
The cast is not needed, I think.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:178
> +struct GetFaviconSurfaceAsyncData {
> + GRefPtr<WebKitFaviconDatabase> faviconDatabase;
> + String pageURL;
> + RefPtr<cairo_surface_t> icon;
> + GRefPtr<GCancellable> cancellable;
> + gulong cancelledId;
> +};
You need to provide a destructor to disconnect the cancellable signal, something like:
struct GetFaviconSurfaceAsyncData {
~GetFaviconSurfaceAsyncData()
{
if (cancelledID)
g_cancellable_disconnect(cancellable.get(), cancelledId);
}
.....
};
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:186
> + if (data && data->cancelledId > 0)
data can't be NULL, add an ASSERT if you want to be extra sure.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:187
> + g_cancellable_disconnect(data->cancellable.get(), data->cancelledId);
This should be done by the struct destructor.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:200
> + completePendingIconRequest(result);
You can call g_simple_async_result_complete(result); directly here.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:245
> + * @page_url: URL of the page containing the icon
Use page_uri
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:273
> + data->cancelledId = cancellable
> + ? g_cancellable_connect(cancellable, G_CALLBACK(webkitFaviconDatabaseGetIconSurfaceCancelled), result.get(), 0)
> + : 0;
The struct is initialize to 0 when allocated, so in thsi case maybe it's clearer to use an if:
if (cancellable)
data->cancelledId = g_cancellable_connect(cancellable, G_CALLBACK(webkitFaviconDatabaseGetIconSurfaceCancelled), result.get(), 0);
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:326
> + * Returns: (transfer full): a new reference to a #cairo_surface_t, or %NULL.
or %NULL in case of error
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:338
> + // The following statement should always return valid data.
> + GetFaviconSurfaceAsyncData* data = static_cast<GetFaviconSurfaceAsyncData*>(g_simple_async_result_get_op_res_gpointer(simpleResult));
> + ASSERT(data);
I think the ASSERT already gives the idea that data shouldn't be NULL, so the comment looks redundant.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:342
> + // Ensure we disconnect any cancellable that might have been set.
> + if (data && data->cancelledId > 0)
> + g_cancellable_disconnect(data->cancellable.get(), data->cancelledId);
This will be done by the destructor, and the async result will be released when the operation si completed.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:346
> + // Return the icon is we can get it already.
> + if (data->icon)
> + return static_cast<cairo_surface_t*>(data->icon.release().leakRef());
Instead of leaking the ref, simple return a new ref, the one in AsyncData struct will be released by the destructor.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:363
> + // If the icon URL is known it obviously means that the icon data
> + // hasn't been retrieved yet, so report that it's 'unavailable'.
'unknown' instead of 'unavailable' no?
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:383
> + completePendingIconRequest(result);
g_simple_async_result_complete()
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:394
> + * Obtains the URI for the favicon for the given page URI.
> + * See also webkit_web_view_get_icon_uri().
I guess this has been copied from wk1 too.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:396
> + * Returns: a newly allocated URI for the favicon, or %NULL
or %NULL if the database doesn't have a favicon for @page_uri
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h:77
> +webkit_favicon_database_get_favicon (WebKitFaviconDatabase* database,
WebKitFaviconDatabase* database -> WebKitFaviconDatabase *database
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h:78
> + const gchar* page_url,
page_uri, or uri
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h:82
> +WEBKIT_API cairo_surface_t*
cairo_surface_t* -> cairo_surface_t *
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h:90
> +webkit_favicon_database_get_favicon_finish (WebKitFaviconDatabase* database,
> + GAsyncResult* result,
> + GError** error);
> +WEBKIT_API gchar*
> +webkit_favicon_database_get_favicon_uri (WebKitFaviconDatabase* database,
> + const gchar* pageURL);
> +WEBKIT_API void
> +webkit_favicon_database_clear (WebKitFaviconDatabase* database);
Ok, all * are incorrectly placed.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabasePrivate.h:26
> +#include <wtf/text/CString.h>
Why do you need this here?
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:46
> +#include <wtf/text/StringBuilder.h>
I don't think you need this.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:331
> + * webkit_web_context_set_favicon_database_path:
I've never known why this method expects a directory and doesn't allow to set a full path, but in case we really want it and use our own filename for the database, I would call this set_favicon_database_directory() to make clearer that the path must be a directory.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:337
> + * for @context on disk. Passing NULL as @path means using the default
%NULL
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:342
> + * Use this before webkit_web_context_get_favicon_database() only if
> + * you want WebKitGTK+ to use a directory different than the default
> + * one for the platform.
If you want to use a different path for the favicon database, this method must be called before the database is created by webkit_web_context_get_favicon_database(). Note that a #WebKitWebView could use the favicon database, so this should also be called before loading any web page.
Or something similar, but these cases should be clear.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:357
> + context->priv->iconDatabasePath = WebCore::filenameToString(path).utf8();
Are you sure that if path is NULL iconDatabasePath will be null too? I would check the case in the early return:
if (context->priv->iconDatabase || !path)
return;
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:360
> +const gchar* webkit_web_context_get_favicon_database_path(WebKitWebContext *context)
This method is undocumented.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:395
> + StringBuilder builder;
This is unused, no?
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:398
>> + GOwnPtr<gchar> iconDatabasePath(g_build_filename(webkit_web_context_get_favicon_database_path(context), "/",
>> + WebCore::IconDatabase::defaultDatabaseFilename().utf8().data(),
>> + NULL));
>
> Use 0 instead of NULL. [readability/null] [5]
Don't add "/", g_build_filename already adds dir separators
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:123
> +WEBKIT_API const gchar*
const gchar* -> const gchar *
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list