[Webkit-unassigned] [Bug 96476] [WK2][GTK] Implement new Favicons API
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Sep 26 01:49:24 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=96476
Carlos Garcia Campos <cgarcia at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #165573|review? |review-
Flag| |
--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> 2012-09-26 01:49:49 PST ---
(From update of attachment 165573)
View in context: https://bugs.webkit.org/attachment.cgi?id=165573&action=review
Looks great in general, there are some minor details, and I think we should try to test as much as possible in the unit tests, when possible because this is not easy to test.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:88
> + * WebKitFaviconDatabase::icon-ready:
Since we are using favicon everywhere, maybe this should be called favicon-ready
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:92
> + * This signal gets emitted when the icon's data is ready to be
We could be a bit more explicit about what icon we are referring. Someting like
This signal gets emitted when the favicon of @page_uri is ready. This means that the favicon's data ....
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:114
> + gulong cancelledId;
Use unsigned long instead.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:168
> + toImpl(wkIconDatabase)->retainIconForPageURL(pageURLString);
You can avoid the toImpl cast by using database->priv->iconDatabase->retainIconForPageURL(pageURLString); directly.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:185
> + iconDatabase->initializeIconDatabaseClient(&wkIconDatabaseClient);
We are using the C API in this particular case in other files, because we are passing a C API struct client after all.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:255
> + * @page_uri: URI of the page containing the icon
We don't know if the page contains an icon or not. Maybe we should say this is the page for which we want to get the favicon
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:269
> +void webkit_favicon_database_get_favicon(WebKitFaviconDatabase* database, const gchar* pageURL, GCancellable* cancellable, GAsyncReadyCallback callback, gpointer userData)
Use pageURI instead of pageURL, to know this is what we get from the api user.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:279
> + data->pageURL = pageURL;
This should be String::fromUTF8(pageURI);
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:284
> + if (cancellable) {
> + data->cancelledId =
> + g_cancellable_connect(cancellable, G_CALLBACK(getIconSurfaceCancelledCallback), result.get(), 0);
> + }
You also need to call g_simple_async_result_set_check_cancellable(), so that when the operation is cancelled and completed in getIconSurfaceCancelled(), g_simple_async_result_propagate_error() fills the error with Operation was cancelled error.
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:294
> + String pageURLString = String::fromUTF8(pageURL);
You already have the pageURLString in data->pageURL
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:372
> + // If the icon URL is known it obviously means that the icon data
> + // hasn't been retrieved yet, so report that it's 'unknown'.
> + g_set_error_literal(error, WEBKIT_FAVICON_DATABASE_ERROR,
> + WEBKIT_FAVICON_DATABASE_ERROR_ICON_UNKNOWN,
> + _("Unknown favicon for page"));
Here we an include the page uri in the error message. Favicon for page %s is unknown or something like that
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:383
> + * Obtains the URI for the favicon for the given page URI.
Maybe the URI of the favicon for the given page URI or even for @page_uri
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.h:68
> +/**
> + * WebKitFaviconDatabaseError:
> + * @WEBKIT_FAVICON_DATABASE_ERROR_NOT_INITIALIZED: The #WebKitFaviconDatabase has not been initialized yet
> + * @WEBKIT_FAVICON_DATABASE_ERROR_ICON_NOT_FOUND: There is not an icon available for the requested URL
> + * @WEBKIT_FAVICON_DATABASE_ERROR_ICON_UNKNOWN: There might be an icon for the requested URL, but its data is unknown at the moment
> + *
> + * Enum values used to denote the various errors related to the #WebKitFaviconDatabase.
> + **/
> +typedef enum {
> + WEBKIT_FAVICON_DATABASE_ERROR_NOT_INITIALIZED,
> + WEBKIT_FAVICON_DATABASE_ERROR_ICON_NOT_FOUND,
> + WEBKIT_FAVICON_DATABASE_ERROR_ICON_UNKNOWN
> +} WebKitFaviconDatabaseError;
I think we should probably use FAVICON everywhere
> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabasePrivate.h:26
> +WebKitFaviconDatabase* webkitFaviconDatabaseCreate(WebKit::WebIconDatabase*);
You can add using namespace WebKit; here and remove it from the .cpp as we are doing in other files.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:24
> +#include "WebIconDatabase.h"
This is already included by WebKitFaviconDatabasePrivate.h
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:102
> + GRefPtr<WebKitFaviconDatabase> iconDatabase;
faviconDatabase?
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:339
> + * default directory for the platform.
You should mention what this means, just by pointing to g_get_user_data_dir() docs.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:358
> + // Do nothing if the database is already initialized, since in WK2
> + // it's not allowed to change this path once the database is open.
> + if (context->priv->iconDatabase || !path)
> + return;
Maybe we could turn this into a g_return_if_fail() since it's a programmer error to call this method twice.
> Source/WebKit2/UIProcess/API/gtk/tests/GNUmakefile.am:74
> + Source/WebKit2/UIProcess/API/gtk/tests/resources/blank.ico
No need to include this, you can use the icon directly from wk1 dir, like we currently do in other tests.
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:39
> + , m_mainLoop(g_main_loop_new(0, TRUE))
> + , m_webView(WEBKIT_WEB_VIEW(webkit_web_view_new()))
If you need a web view and a main loop you probably want to inherit from WebViewTest instead of Test
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:40
> + , m_baseURI(soup_uri_to_string(kServer->baseURI(), FALSE))
Do you really need to cache this? you can use WebKitTestServer::getURIForPath() when you need the base uri
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:56
> + const char* baseURI()
> + {
> + return m_baseURI.get();
> + }
Do you really need a method for this? :-)
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:166
> + CString resourcesDir = Test::getResourcesDir();
> + GOwnPtr<char> pathToFavicon(g_build_filename(resourcesDir.data(), "blank.ico", NULL));
You can use Test::getWebKit1TestResoucesDir() instead and use the icon from the wk1 dir
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:170
> + GError* error = 0;
> + g_file_get_contents(pathToFavicon.get(), &contents, &length, &error);
> + g_assert(!error);
g_file_get_contents returns a boolean, if you are no interested in the error, use the result. It's very unlikely that this fail, so I don't think it's worth it checking anyway.
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:206
> + test->m_iconReadySignalReceived = false;
> + test->askForIconAndWaitForIconReadySignal();
> + g_assert(test->m_iconReadySignalReceived);
> +
> + test->m_iconIsValid = false;
> + test->askAndWaitForValidFavicon();
> + g_assert(test->m_iconIsValid);
> +
> + test->askAndWaitForInvalidFavicon();
> + g_assert(!test->m_iconIsValid);
Maybe we should move some of the logic from the fixture here, I'm not sure, but I think we want to test all possible errors (if possible).
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:215
> + GOwnPtr<char> expectedURI(g_strdup_printf("%sfavicon.ico", baseURI));
You can use kServer->getURIForPath(/favicon.ico)
--
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