[GTK] WebKitIconDatabase doesn't keep icons cached

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Christian Dywan]

So you use WebKitIconDatabase to load icons, open a few sites and it works beautifully.

Then you quit.

Then you run the browser again and the icons are gone.

I thought this should either work by itself, until the hardcoded number of 25 icons is reached
or if I added WebCore::iconDatabase().retainIconForPageURL(pageURL) into webkit_icon_database_get_icon_pixbuf but this has no effect.

So I'm not sure what I'm missing, but clearly always removing all icons like this is nonsense.

[Sergio Villar Senin]

(In reply to comment #0)
> So you use WebKitIconDatabase to load icons, open a few sites and it works beautifully.
> 
> Then you quit.
> 
> Then you run the browser again and the icons are gone.
> 
> I thought this should either work by itself, until the hardcoded number of 25 icons is reached
> or if I added WebCore::iconDatabase().retainIconForPageURL(pageURL) into webkit_icon_database_get_icon_pixbuf but this has no effect.
> 
> So I'm not sure what I'm missing, but clearly always removing all icons like this is nonsense.

I don't know what browser you're using but if it's epiphany then I have to tell you that it has its own icon cache and actually does not make use of the webkit's icon cache. I'll work on that, because if it makes sense we could likely remove epy's cache.

[Christian Dywan]

(In reply to comment #1)
> (In reply to comment #0)
> > So you use WebKitIconDatabase to load icons, open a few sites and it works beautifully.
> > Then you quit.
> > Then you run the browser again and the icons are gone.
> I don't know what browser you're using but if it's epiphany then I have to tell you that it has
> its own icon cache and actually does not make use of the webkit's icon cache. I'll work on that,
> because if it makes sense we could likely remove epy's cache.

I know very well that nobody uses that cache because I committed it only last tuesday ;-)

I locally modified Midori to use it. I know about Epiphany's cache and I would indeed recommend it to switch once this issue and bug 56201 are resolved.

[Sergio Villar Senin]

Created attachment 86047 [details]
Patch

[Sergio Villar Senin]

Created attachment 86328 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/graphics/BitmapIma..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:402:  Extra space between guint and size  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:72:  Extra space between guint and size  [whitespace/declaration] [3]
Total errors found: 2 in 14 files


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

[Christian Dywan]

(In reply to comment #4)
> Created an attachment (id=86328) [details]
> Patch

An undesirable side effect of the patch is that the icon database is not shared by default, ie. the current behavior is to use the same database path unless explicitly overridden.

One idea: add a webkit_icon_database_get_default_path().

Another idea: don't cleanup by default, so the app can retain pages and allow cleanup when it makes sense. So it would be reduced from


_delay_database_cleanup
_set_path
_retain_icon_for_uri
_allow_database_cleanup

to

_retain_icon_for_uri
_allow_database_cleanup

Btw I think the "database" in the method name is redundant.

[Sergio Villar Senin]

Comment on attachment 86328 [details]
Patch

The patch is wrong as gdk_pixbuf_get_from_surface does not scale the pixbufs as I thought, it just return the part of the surface of the given size

[Christian Dywan]

Since I just saw it due to a commit: we may need to manually handle private mode, according to http://trac.webkit.org/changeset/81824/trunk/Source/WebKit/mac/Misc/WebIconDatabase.mm we need to call it from web settings. Not quite related to this bug, but I wouldn't want to forget it.

[Sergio Villar Senin]

Created attachment 86765 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #9)
> Created an attachment (id=86765) [details]
> Patch

Just a few comments about my last path:

   * the database is disabled by default, users must enable it
   * there is a default directory that will be used unless changed by caller
   * Not sure about some "Since:" comments, for example should I replace the ones whose API has changed?

[Sergio Villar Senin]

Created attachment 87701 [details]
New API for WebKitIconDatabase

New version after last beidson's changes in the IconDatabase

[Sergio Villar Senin]

Created attachment 87702 [details]
Remove "icon-loaded" signal from the WebKitIconDatabase

I think we no longer need this signal after the new "icon-changed" signal I added in the previous patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:402:  Extra space between guint and size  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:61:  Extra space between gboolean and enabled  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:80:  Extra space between guint and size  [whitespace/declaration] [3]
Total errors found: 3 in 8 files


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

[Sergio Villar Senin]

BTW another positive side effect of this patch is that WebCore will no longer issue a request for the favicon cache of every webpage the users visit. Right now, WebKitGtk+ always asks for favicon.ico for every web page because clients are not opening the IconDatabase, and yes, it does not matter that the client has a built-in favicon cache, WebCore will always ask for the favicon if the IconDatabase is not used.

[Christian Dywan]

Care to comment on my suggestions in comments #6 and #8? I don't necessarily expect you to like my ideas but it would be nice to discuss this :-)


+ // WebKit1 only has a single "icon did change" notification.

What exactly does this mean? I do see several functions there for different notifications. Or is that comment copied from another port?

For what I want, one signal is completely sufficient.

+ * your program will tipically do:
+ * webkit_icon_database_delay_database_cleanup(). Tipically this

Spanglish ;-) "typically"

+     * WebKitIconDatabase::icons-removed:

The method to achieve this is called "clear", I think this should be "cleared" accordingly.

+#if ENABLE(ICONDATABASE)

Most of the code is not wrapped in conditionals, and as far as I'm concerned it's not sensible to allow excluding this.

+ * webkit_icon_database_is_enabled:

I suggest to call this _get_enabled analogous to _set_enabled. Going by GTK+ as a role model _is_foo is used for read-only values that can't be changed.

+ * Requires an enabled database.

Personally I feel this is redundant/ obvious. But no strong opinion, if in doubt better to be more verbose than too curt.

+     * WebKitIconDatabase::icon-changed:

So effectively this is

icon-loaded (frame, uri) → icon-changed (uri)

Why are you dropping the frame argument?
Isn't this inconsitent with icon-loaded in WebKitWebView now?

I think this should be squashed in one change OR do the whole signal change in one patch.

+ * webkit_icon_database_retain_icon_for_uri:

Perhaps g_return_if_fail (!webkit_icon_database_get_enabled ()); here?

[Sergio Villar Senin]

(In reply to comment #15)
> Care to comment on my suggestions in comments #6 and #8? I don't necessarily expect you to like my ideas but it would be nice to discuss this :-)

Sure, I just somehow forget about them after rebasing the patch against the last changes. Regarding comment #6 I just somehow followed the current behaviour of WebCore when designing the API. Apart from that having an "allow" without a "disable" is like a "set" without a "get" :). And about comment #8 you mentioned that it was not related so I didn't take a closer look at it.

> + // WebKit1 only has a single "icon did change" notification.
> 
> What exactly does this mean? I do see several functions there for different notifications. Or is that comment copied from another port?

Sure I think I took it accidentally from the mac port, but anyway, I think that having a single signal is more than enough for the moment, we could think about adding more if needed in the future.
 
> For what I want, one signal is completely sufficient.
> 
> + * your program will tipically do:
> + * webkit_icon_database_delay_database_cleanup(). Tipically this
> 
> Spanglish ;-) "typically"

:)
 
> +     * WebKitIconDatabase::icons-removed:
> 
> The method to achieve this is called "clear", I think this should be "cleared" accordingly.

Yeah well, at the same time the IconDatabase method is called removeAllIcons, I guess that's why I chose that name but if you think it's clearer I can change it
 
> +#if ENABLE(ICONDATABASE)
> 
> Most of the code is not wrapped in conditionals, and as far as I'm concerned it's not sensible to allow excluding this.

Should be wrapped actually, but since none of our code is I guess I can remove it.

> + * webkit_icon_database_is_enabled:
> 
> I suggest to call this _get_enabled analogous to _set_enabled. Going by GTK+ as a role model _is_foo is used for read-only values that can't be changed.

Fair enough, looks sensible to me.
 
> + * Requires an enabled database.
> 
> Personally I feel this is redundant/ obvious. But no strong opinion, if in doubt better to be more verbose than too curt.

I think it's better to keep it just to state it very clearly that you have to follow the right steps to get it working.

> +     * WebKitIconDatabase::icon-changed:
> 
> So effectively this is
> 
> icon-loaded (frame, uri) → icon-changed (uri)
> 
> Why are you dropping the frame argument?
> Isn't this inconsitent with icon-loaded in WebKitWebView now?

Basically because I wanted to have just 1 signal that could be used both from the IconDatabaseClient and the FrameLoaderClient. I think it's better to keep the icon database away from any UI-like element like the frames. If you want to listen to a change in a favicon for a specific webview you can still use the icon-loaded signal.

> I think this should be squashed in one change OR do the whole signal change in one patch.

Sure, I just split them for reviewing purpouses to state clearly that replacing the other signal is not a requirement for the main patch

> + * webkit_icon_database_retain_icon_for_uri:
> 
> Perhaps g_return_if_fail (!webkit_icon_database_get_enabled ()); here?

Good catch!. Thx for the review Christian!!!

[Sergio Villar Senin]

(In reply to comment #16)
> > + * webkit_icon_database_retain_icon_for_uri:
> > 
> > Perhaps g_return_if_fail (!webkit_icon_database_get_enabled ()); here?
> 
> Good catch!. Thx for the review Christian!!!

:m actually I'm handling this in the body of the function as a valid case. Whether we add it to all the functions that require a enabled database or to none of them.

[Martin Robinson]

Comment on attachment 87702 [details]
Remove "icon-loaded" signal from the WebKitIconDatabase

This one is missing a ChangeLog and might be better to merge this with your first patch.

[Martin Robinson]

Comment on attachment 87701 [details]
New API for WebKitIconDatabase

As I said in person, this patch is awesome. Other than the nits that I mentioned I would switch to representing the IconDatabaseClient virtual methods with three separate signals though. Thanks!

[Sergio Villar Senin]

Created attachment 92245 [details]
Patch

New version with the changes suggested by Martin that I was able to remember :)

[Eric Seidel (no email)]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:402:  Extra space between guint and size  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:61:  Extra space between gboolean and enabled  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:80:  Extra space between guint and size  [whitespace/declaration] [3]
Total errors found: 3 in 8 files


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

[Martin Robinson]

Comment on attachment 92245 [details]
Patch

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

Seems like a very important change, though it's an API break. I'd really like to get Xan and Gustavo to sign off on this one as well. r- for now for the nits.

> Source/WebKit/gtk/WebCoreSupport/IconDatabaseClientGtk.cpp:57
> +}
> +void IconDatabaseClientGtk::didChangeIconForPageURL(const String& pageURL)

Need an extra space here for my sanity. ;)

> Source/WebKit/gtk/WebCoreSupport/IconDatabaseClientGtk.h:33
> +namespace WebCore {
> +class IconDatabase;
> +}; // namespace WebCore

I don't think you need this forward declaration.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:64
> + * Every icon in the database has a retain count.  If an icon has a
> + * retain count greater than 0, it will be written to disk for later
> + * use. If an icon's retain count equals zero it will be removed from
> + * disk.  The retain count is not persistent across launches. If the
> + * WebKit client wishes to retain an icon it should retain the icon
> + * once for every launch.  This is best done at initialization time
> + * before the database begins removing icons.  To make sure that the
> + * database does not remove unretained icons prematurely, call
> + * webkit_icon_database_delay_database_cleanup() until all desired
> + * icons are retained.  Once all are retained, call
> + * webkit_icon_database_allow_database_cleanup().

Some of these sentences have two spaces after them and some one. Please use one everywhere.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:192
> +            (GSignalFlags)G_SIGNAL_RUN_LAST,

Should use a static_cast here.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:196
> +            0,
> +            NULL,
> +            NULL,
> +            g_cclosure_marshal_VOID__STRING,

All NULLs should be zero unless that produces a warning.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:218
>              (GSignalFlags)G_SIGNAL_RUN_LAST,

static_cast

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:236
> +     * After receiving this signal
> +     * webkit_icon_database_get_icon_pixbuf() will successfully return
> +     * the icon data associatted with the provided page URI.

Maybe even this one out a little bit.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:242
> +            (GSignalFlags)G_SIGNAL_RUN_LAST,

static_cast

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:259
> +            (GSignalFlags)G_SIGNAL_RUN_LAST,

static_cast

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:275
> +    // Disabled by default

Might want to say here why it's disabled by default.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:301
> +        // Do not use set_path(), otherwise we could end up calling
> +        // startupIconDatabase twice.

You can make this one line.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:436
> + * be cached then it should listen to icon-changed signal in order to
> + * know when the icon data is actually available. Otherwise if the
> + * icon need to be retrieved from an external resource the client
> + * should connect to icon-changed signal.

Shouldn't a developer listen for the icon-data-imported-signal? I'm not sure I fully understand the last sentence. :/

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:499
> + * want to keep and once all are retained call allowDatabaseCleanup to

allowDatabaseCleanup -> webkit_icon_database_allow_cleanup

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:543
> +                                              const gchar*        page_uri)

Please use WebKit coding conventions here. So this declaration should be:
void void webkit_icon_database_retain_icon_for_uri(WebKitIconDatabase* database, const gchar* pageURI)
{

}

[Sergio Villar Senin]

(In reply to comment #22)
> (From update of attachment 92245 [details])
> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:436
> > + * be cached then it should listen to icon-changed signal in order to
> > + * know when the icon data is actually available. Otherwise if the
> > + * icon need to be retrieved from an external resource the client
> > + * should connect to icon-changed signal.
> 
> Shouldn't a developer listen for the icon-data-imported-signal? I'm not sure I fully understand the last sentence. :/

You're definitely right Martin, it is just that I forgot to change that paragraph in the last version of my patch.

[Sergio Villar Senin]

(In reply to comment #22)
> (From update of attachment 92245 [details])
> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:436
> > + * be cached then it should listen to icon-changed signal in order to
> > + * know when the icon data is actually available. Otherwise if the
> > + * icon need to be retrieved from an external resource the client
> > + * should connect to icon-changed signal.
> 
> Shouldn't a developer listen for the icon-data-imported-signal? I'm not sure I fully understand the last sentence. :/

You're definitely right Martin, it is just that I forgot to change that paragraph in the last version of my patch.

BTW I'm not sure about what I should use for the "Since:" section of the documentation.

[Martin Robinson]

(In reply to comment #24)

> You're definitely right Martin, it is just that I forgot to change that paragraph in the last version of my patch.
> 
> BTW I'm not sure about what I should use for the "Since:" section of the documentation.

That might depend a bit on how we handle the API break.

[Sergio Villar Senin]

(In reply to comment #25)
> (In reply to comment #24)
> 
> > You're definitely right Martin, it is just that I forgot to change that paragraph in the last version of my patch.
> > 
> > BTW I'm not sure about what I should use for the "Since:" section of the documentation.
> 
> That might depend a bit on how we handle the API break.

Xan?

[Sergio Villar Senin]

Adding Gustavo just in case.

[Gustavo Noronha (kov)]

Comment on attachment 92245 [details]
Patch

I think this kind of API break is something we should avoid. Unfortunately it doesn't seem to be trivial to do... if we want to remove that signal and change the signature of the get_icon_pixbuf API for 1.6.x we should deprecate them in 1.4.x and add the new stuff, but without breaking it would be great though.

So, would it be feasible to keep the signal and the methods, and initialize the icon database if it is not yet initialized when the a request is initiated? The new APIs look good to me, anyway.

[Sergio Villar Senin]

(In reply to comment #28)
> (From update of attachment 92245 [details])
> I think this kind of API break is something we should avoid. Unfortunately it doesn't seem to be trivial to do... if we want to remove that signal and change the signature of the get_icon_pixbuf API for 1.6.x we should deprecate them in 1.4.x and add the new stuff, but without breaking it would be great though.

The removal of the signal and the change of the get_icon_pixbuf API are not mandatory. We can make everything work without those changes. Just that IMO that signal becomes unnecessary (while adding confusion to users IMHO) and having a size attribute in the get_icon_pixbuf gives us a bit more of flexibility, but that's all.

> So, would it be feasible to keep the signal and the methods, and initialize the icon database if it is not yet initialized when the a request is initiated? The new APIs look good to me, anyway.

It is not possible because you have to let clients retain their icons. In order to do this you have to have an open database. But by opening the database, you are allowing the cleanup process to start removing icons, so you have to previously delay that cleanup.

[Xan Lopez]

Comment on attachment 92245 [details]
Patch

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

All the Since: should be 1.5.1, I agree we have to deprecate the old signal, we cannot just remove it. Also, as I say, it's not clear to me that we should not provide some easy way for apps to just say "I want persistent icons, store them here <path>"; the stuff that we require every app to do seems like the default behavior that most of them will want to enable anyway.

> Source/WebKit/gtk/WebCoreSupport/IconDatabaseClientGtk.cpp:23
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

We use LGPL2+ for the stuff we write unless we have a good reason not to, AFAIK.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:212
> +     * this signal.

Why would I ever want to know about only the URI? Can't the data signal just carry the URI too?

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:227
> +     * WebKitIconDatabase::icon-uri-imported:

Should be icon-data-imported

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:492
> + * webkit_icon_database_database_cleanup:

Missing 'delay'

[Xan Lopez]

Er, ignore the "As I said" bit in the previous comment. That was me talking with Sergio on jabber.

[Xan Lopez]

To expand a bit:

- A common behavior I see would be something along the lines of. I want favicons stored, use this directory as storage point. You can dispose of them after X days if they are unused, I don't care too much about anything else. I think this likely covers 99% of the apps.

- If someone else wants to do something more complicated I guess they should be able to override things, and then probably providing detailed APIs like this patch does makes sense. Epiphany might be one of those cases, since we might need custom storage of favicons for performace reasons at some point.

I guess my point is: I think what this patch provides seems like a barebones/detailed kind of API, which is OK, but I wonder if it makes sense to request every app to develop their own thing when we could cover most use cases with some code on the webkit side. Those are my 2 cents anyway.

[Martin Robinson]

(In reply to comment #32)
> To expand a bit:
> 
> - A common behavior I see would be something along the lines of. I want favicons stored, use this directory as storage point. You can dispose of them after X days if they are unused, I don't care too much about anything else. I think this likely covers 99% of the apps.
> 
> - If someone else wants to do something more complicated I guess they should be able to override things, and then probably providing detailed APIs like this patch does makes sense. Epiphany might be one of those cases, since we might need custom storage of favicons for performace reasons at some point.
> 
> I guess my point is: I think what this patch provides seems like a barebones/detailed kind of API, which is OK, but I wonder if it makes sense to request every app to develop their own thing when we could cover most use cases with some code on the webkit side. Those are my 2 cents anyway.

I agree that it may make sense to provide an easy icon cache API. Perhaps we should wait a release or two after the pushing the "bare bones" icon database API to guage interest. Providing convenience APIs over what the Mac API provides is somewhat of a risk due to situations where we design ourselves between a rock and a hard place -- like the previous icon database API. I think we should approach such convenience layers with a good deal of caution.

[Sergio Villar Senin]

Created attachment 95879 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 95879 [details]
Patch

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

I agree with Xan, 99% of the users (if not 100%) don't need this API, we could enable a persistent cache transparently by default, since I think it's the expected behaviour. It's what webkit2 does, indeed, see http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/WebIconDatabase.cpp#L63.
We could do the same in webkit_icon_database_set_path():


    WebCore::IconDatabase::delayDatabaseCleanup();
    WebCore::iconDatabase().setEnabled(true);
    if (!WebCore::iconDatabase().open(WebCore::filenameToString(path), WebCore::IconDatabase::defaultDatabaseFilename())) {
        WebCore::IconDatabase::allowDatabaseCleanup();
        return;
    }

    database->priv->path.set(g_strdup(path));

> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:933
> +    // TODO: remove this block. Deprecated signal.
>      const gchar* frameURI = webkit_web_frame_get_uri(m_frame);
>      WebKitIconDatabase* database = webkit_get_icon_database();
>      g_signal_emit_by_name(database, "icon-loaded", m_frame, frameURI);

I'm not sure I've understood how the icon database works, but I don't think icon-loaded is the same than any of the new signals added by this patch. dispatchDidReceiveIcon() will be called also when the icon is not loaded because it's already in the database, so probably the name is not correct, since the icon hasn't been loaded and synchronousIconForPageURL() might return NULL indeed. The difference is that this will be called always during loading, while icon-data-imported won't be emitted unless you explicitely ask for the icon.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:319
> +    WebCore::iconDatabase().open(webkit_icon_database_get_path(database),
> +                                 IconDatabase::defaultDatabaseFilename());

We should probably check the result of open(), since it might fail and return false.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:406
> + * As the WebKit's IconDatabase loads the icon data asynchronously and
> + * in a per-demand basis, this method could return NULL even if the
> + * icon is actually cached. If the caller knows that the icon is
> + * cached then it should listen to icon-data-imported signal in order
> + * to know when the icon data is actually available.

That's not enough I think, you should call synchronousIconForPageURL() at least once to trigger a new readFromDatabase() that will notify the client calling didImportIconDataForPageURL(). This is very confusing, the fact is that this method looks synchronous, but it's actually asynchronous. I think we could add an async version 

void webkit_icon_database_get_icon_pixbuf_async(WebKitIconDatabase* database, const gchar* pageURI,  GCancellable* cancellable,  GAsyncReadyCallback callback, gpointer userData);
GdkPixbuf* webkit_icon_database_get_icon_pixbuf_finish(WebKitIconDatabase* database,  GAsyncResult* result,  GError **error);

and add a real sync implementation to the existing method.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:415
> -GdkPixbuf* webkit_icon_database_get_icon_pixbuf(WebKitIconDatabase* database, const gchar* pageURI)
> +GdkPixbuf* webkit_icon_database_get_icon_pixbuf(WebKitIconDatabase* database, const gchar* pageURI, guint size)

Instead of breaking the API we could simply add a new version webkit_icon_database_get_icon_pixbuf_with_size() or add a comment to the docs suggesting to use gdk_pixbuf_scale_simple() to resize the icon if needed.

[Carlos Garcia Campos]

Comment on attachment 95879 [details]
Patch

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

> Source/WebKit/gtk/GNUmakefile.am:198
> +	Source/WebKit/gtk/WebCoreSupport/IconDatabaseClientGtk.cpp \
> +	Source/WebKit/gtk/WebCoreSupport/IconDatabaseClientGtk.h \

Thse files are missing in the patch.

[Carlos Garcia Campos]

Created attachment 99704 [details]
Another patch

I've noticed that there's FrameLoaderClient::registerForIconNotification() which is called when the icon is in the database, but its data is not yet available. In that case we can register a notification request in the database, and when notified call FrameLoaderClient::dispatchDidReceiveIcon(). This way we can make sure that webkit_icon_database_get_icon_pixbuf() will always return a valid pixbuf when called in the icon-loaded callback. The patch also adds support for favicons to GtkLauncher, although we should probably add a unit test for this too.

[Carlos Garcia Campos]

Created attachment 100340 [details]
Updated patch

This patch adds async api to get a pixbuf, since Xan told me it's required by ephy history and bookmarks that need an api to get a pixbuf without having to connect to icon-loaded signal. It also fixes some docs comments, and adds unit tests for WebIconDatabase.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testicondatabase.c"
Source/WebKit/gtk/webkit/webkiticondatabase.h:76:  Extra space between GCancellable* and cancellable  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:78:  Extra space between gpointer and user_data  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:82:  Extra space between GAsyncResult* and result  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:83:  Extra space between GError** and error  [whitespace/declaration] [3]
Total errors found: 4 in 9 files


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

[Carlos Garcia Campos]

Created attachment 100461 [details]
Updated patch

Fixed a possible use of m_cancelled_id unitnializaed in webkiticondatabaseprivate.h and renamed it to m_cancelledId to follow coding style.

[Carlos Garcia Campos]

Created attachment 100462 [details]
Updated patch

Sorry, I sumbitted the same patch again instead of the updated one.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testicondatabase.c"
Source/WebKit/gtk/webkit/webkiticondatabase.h:76:  Extra space between GCancellable* and cancellable  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:78:  Extra space between gpointer and user_data  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:82:  Extra space between GAsyncResult* and result  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:83:  Extra space between GError** and error  [whitespace/declaration] [3]
Total errors found: 4 in 9 files


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

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testicondatabase.c"
Source/WebKit/gtk/webkit/webkiticondatabase.h:76:  Extra space between GCancellable* and cancellable  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:78:  Extra space between gpointer and user_data  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:82:  Extra space between GAsyncResult* and result  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:83:  Extra space between GError** and error  [whitespace/declaration] [3]
Total errors found: 4 in 9 files


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

[Martin Robinson]

Comment on attachment 100462 [details]
Updated patch

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

I like this patch a great deal. I was initially a bit worried about the approach of hiding the behavior of the WebCore icon cache.  After seeing it done in WebKit2, I think it's a great idea.

> Source/WebKit/gtk/tests/testicondatabase.c:3
> +/*
> + * Copyright (C) 2011 Igalia S.L.
> + *

Please use WebKit style naming for this file.

> Source/WebKit/gtk/tests/testicondatabase.c:29
> +static void delete_db_file_if_exists(const gchar *db_path)

Instead of "db" this should be "database"

> Source/WebKit/gtk/tests/testicondatabase.c:34
> +    gchar* db_filename = g_build_filename(db_path, "WebpageIcons.db", NULL);

Webpage -> WebPage to match the typical naming convention in WebKit.

> Source/WebKit/gtk/tests/testicondatabase.c:59
> +    GError* error = NULL;

NULL -> 0

> Source/WebKit/gtk/tests/testicondatabase.c:108
> +    g_signal_connect(view, "icon-loaded", G_CALLBACK (idle_quit_loop_cb), NULL);

Extra space after G_CALLBACK

> Source/WebKit/gtk/tests/testicondatabase.c:118
> +    WebKitIconDatabase* icon_database = webkit_get_icon_database();
> +    webkit_icon_database_get_icon_pixbuf_async(icon_database, "http://www.google.com/", NULL,
> +                                               icon_database_get_icon_valid_callback, user_data);

No need to cache icon database.

> Source/WebKit/gtk/tests/testicondatabase.c:124
> +    WebKitIconDatabase* icon_database = webkit_get_icon_database();

Ditto.

> Source/WebKit/gtk/tests/testicondatabase.c:132
> +{
> +    WebKitIconDatabase* icon_database = webkit_get_icon_database();

Ditto

> Source/WebKit/gtk/tests/testicondatabase.c:148
> +    /* Load google to make sure favicon is added to database */

This sentence is missing a period.

> Source/WebKit/gtk/tests/testicondatabase.c:166
> +    webkit_icon_database_set_path(icon_database, NULL);

NULL -> 0

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:87
> +    GList* pendingIcons;
> +    gboolean importFinished;

Please use bool here instead of gboolean. It looks like these values are not initialized during instance initialization. I believe you should do that regardless of whether anything currently depends on it. Would prefer pendingIcons to be called pendingIconRequests and for it to be a WTF::Vector.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:277
> +static GdkPixbuf* get_icon_pixbuf(WebKitIconDatabase* database, const String& pageURL)

Should be called something like getIconPixbufSynchronously.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:332
> +            GSimpleAsyncResult* result = request->asyncResult();
> +            ASSERT(result);
> +            database->priv->pendingIcons = g_list_remove_link(database->priv->pendingIcons, icons);
> +            g_simple_async_result_set_error(result, G_IO_ERROR, G_IO_ERROR_CANCELLED, "%s", "Operation was cancelled");
> +            g_simple_async_result_complete(result);
> +            delete request;
> +            g_list_free(icons);
> +            break;

Can this be a helper in PendingIconRequest?

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:361
> + * This is an asynchronous method. When the operation is finished, callback will
> + * be invoked. You can then call webkit_icon_database_get_icon_pixbuf_finish()
> + * to get the result of the operation.
> + * See also webkit_icon_database_get_icon_pixbuf().

Do you think it's useful to mention why you want to load icons asynchronously here?

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:379
> +    if (cancellable) {
> +        request->m_cancellable = cancellable;
> +        request->m_cancelledId = g_cancellable_connect(cancellable, G_CALLBACK(webkit_icon_database_get_icon_pixbuf_cancelled), request, 0);
> +        g_object_set_data_full(G_OBJECT(result.get()), "cancellable", g_object_ref(cancellable), static_cast<GDestroyNotify>(g_object_unref));
> +    }
> +

I believe you should just pass the cancellable to the PendingIconRequest constructor and make all this the responsibility of the constructor.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:383
> +    String pageURL = String::fromUTF8(pageURI);
> +    GdkPixbuf* pixbuf = get_icon_pixbuf(database, pageURL);

No need to cache pageURL.


> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:394
> +    delete request;

Wish there was a better way to handle this than manual memory management. It's probably okay though. :)

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:433
> +        if (request->pageURI() == pageURI) {

Please use an early continue here instead of nesting.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:441
> +            database->priv->pendingIcons = g_list_remove_link(database->priv->pendingIcons, icons);
> +            if (request->frameLoaderClient())
> +                request->frameLoaderClient()->dispatchDidReceiveIcon();
> +            else if (request->asyncResult()) {
> +                GSimpleAsyncResult* result = request->asyncResult();
> +                g_simple_async_result_set_op_res_gpointer(result, get_icon_pixbuf(database, pageURI), 0);
> +                g_simple_async_result_complete(result);
> +            }

Can we move this to a helper in PendingIconRequest?

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:453
> +    PendingIconRequest* request = new PendingIconRequest(pageURI, client);
> +    database->priv->pendingIcons = g_list_prepend(database->priv->pendingIcons, request);

No need to cache request.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:493
> +            if (result) {
> +                g_simple_async_result_set_op_res_gpointer(result, 0, 0);
> +                g_simple_async_result_complete(result);
> +            }

I like the idea of pushing this kind of code into methods in PendingIconRequest.

> Source/WebKit/gtk/webkit/webkiticondatabaseprivate.h:30
> +#include "FrameLoaderClient.h"
> +#include "GRefPtr.h"
> +#include "IconDatabaseClient.h"
> +#include "webkitglobals.h"
> +
> +namespace WebCore {
> +class FrameLoaderClient;
> +class IconDatabaseClient;

Why both include headers and forward declare FrameLoaderClient and IconDatabaseClient?

> Source/WebKit/gtk/webkit/webkiticondatabaseprivate.h:38
> +void _webkit_icon_database_register_for_icon_notification(WebKitIconDatabase*, const gchar* pageURI, WebCore::FrameLoaderClient*);
> +void _webkit_icon_database_unregister_for_icon_notification(WebKitIconDatabase*, const gchar* pageURI, WebCore::FrameLoaderClient*);
> +void _webkit_icon_database_process_pending_icons_for_uri(WebKitIconDatabase*, const String& pageURI);
> +void _webkit_icon_database_import_finished(WebKitIconDatabase*);
> +}

We've been using WebKit-style naming for private methods.

> Source/WebKit/gtk/webkit/webkiticondatabaseprivate.h:54
> +class IconDatabaseClientGtk : public WebCore::IconDatabaseClient {
> +    public:
> +    // IconDatabaseClient interface
> +    virtual bool performImport() { return true; }
> +    virtual void didRemoveAllIcons() { };
> +    virtual void didImportIconURLForPageURL(const String&) { };
> +    virtual void didImportIconDataForPageURL(const String& URL)
> +    {
> +        _webkit_icon_database_process_pending_icons_for_uri(webkit_get_icon_database(), URL);
> +    }
> +    virtual void didChangeIconForPageURL(const String&) { };
> +    virtual void didFinishURLImport() { _webkit_icon_database_import_finished(webkit_get_icon_database()); }
> +};

This is only used in one file so it shouldn't be in the header. _webkit_icon_database_process_pending_icons_for_uri and _webkit_icon_database_import_finished can then become static.

> Source/WebKit/gtk/webkit/webkiticondatabaseprivate.h:62
> +struct PendingIconRequest {
> +public:
> +    PendingIconRequest(const char* pageURI, WebCore::FrameLoaderClient* frameLoaderClient)
> +        : m_pageURI(pageURI)
> +        , m_frameLoaderClient(frameLoaderClient)
> +        , m_asyncResult(0)
> +        , m_cancellable(0)

This class is only used in one file, so it should not be in a header.

[Martin Robinson]

Comment on attachment 100462 [details]
Updated patch

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

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:476
> +{
> +    database->priv->importFinished = TRUE;
> +

Let's assert that we are on the UI thread here and in other places where it matters.

[Carlos Garcia Campos]

(In reply to comment #44)
> (From update of attachment 100462 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100462&action=review
> 
> I like this patch a great deal. I was initially a bit worried about the approach of hiding the behavior of the WebCore icon cache.  After seeing it done in WebKit2, I think it's a great idea.
> 
> > Source/WebKit/gtk/tests/testicondatabase.c:3
> > +/*
> > + * Copyright (C) 2011 Igalia S.L.
> > + *
> 
> Please use WebKit style naming for this file.

I thought it wasn't necessary for unit tests, they are indeed ignored by the check-webkit-style script and other unit tests don't follow the style either. 

> > Source/WebKit/gtk/tests/testicondatabase.c:34
> > +    gchar* db_filename = g_build_filename(db_path, "WebpageIcons.db", NULL);
> 
> Webpage -> WebPage to match the typical naming convention in WebKit.

This is not possible, WebpageIcons.db is the name used by WebCore, webkit_icon_database_set_path() sets the directory, not the full path.

> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:87
> > +    GList* pendingIcons;
> > +    gboolean importFinished;
> 
> Please use bool here instead of gboolean. It looks like these values are not initialized during instance initialization.

Yes they are initialized, instances are allocated with g_slice_alloc0() by glib, the private struct is part of the instance.

> I believe you should do that regardless of whether anything currently depends on it. Would prefer pendingIcons to be called pendingIconRequests and for it to be a WTF::Vector.
> 
> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:277
> > +static GdkPixbuf* get_icon_pixbuf(WebKitIconDatabase* database, const String& pageURL)
> 
> Should be called something like getIconPixbufSynchronously.
> 
> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:332
> > +            GSimpleAsyncResult* result = request->asyncResult();
> > +            ASSERT(result);
> > +            database->priv->pendingIcons = g_list_remove_link(database->priv->pendingIcons, icons);
> > +            g_simple_async_result_set_error(result, G_IO_ERROR, G_IO_ERROR_CANCELLED, "%s", "Operation was cancelled");
> > +            g_simple_async_result_complete(result);
> > +            delete request;
> > +            g_list_free(icons);
> > +            break;
> 
> Can this be a helper in PendingIconRequest?

I'm not sure, we need to remove the request from the list and delete it, the request itself can't be removed from the list unless we pass the list somehow to the PendingIconRequest.

> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:361
> > + * This is an asynchronous method. When the operation is finished, callback will
> > + * be invoked. You can then call webkit_icon_database_get_icon_pixbuf_finish()
> > + * to get the result of the operation.
> > + * See also webkit_icon_database_get_icon_pixbuf().
> 
> Do you think it's useful to mention why you want to load icons asynchronously here?

Sure.

> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:379
> > +    if (cancellable) {
> > +        request->m_cancellable = cancellable;
> > +        request->m_cancelledId = g_cancellable_connect(cancellable, G_CALLBACK(webkit_icon_database_get_icon_pixbuf_cancelled), request, 0);
> > +        g_object_set_data_full(G_OBJECT(result.get()), "cancellable", g_object_ref(cancellable), static_cast<GDestroyNotify>(g_object_unref));
> > +    }
> > +
> 
> I believe you should just pass the cancellable to the PendingIconRequest constructor and make all this the responsibility of the constructor.

We need to connect the signal and the callback is part of WebKitIconDatabase, that's why it's here. 

> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:383
> > +    String pageURL = String::fromUTF8(pageURI);
> > +    GdkPixbuf* pixbuf = get_icon_pixbuf(database, pageURL);
> 
> No need to cache pageURL.
> 
> 
> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:394
> > +    delete request;
> 
> Wish there was a better way to handle this than manual memory management. It's probably okay though. :)
> 
> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:433
> > +        if (request->pageURI() == pageURI) {
> 
> Please use an early continue here instead of nesting.
> 
> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:441
> > +            database->priv->pendingIcons = g_list_remove_link(database->priv->pendingIcons, icons);
> > +            if (request->frameLoaderClient())
> > +                request->frameLoaderClient()->dispatchDidReceiveIcon();
> > +            else if (request->asyncResult()) {
> > +                GSimpleAsyncResult* result = request->asyncResult();
> > +                g_simple_async_result_set_op_res_gpointer(result, get_icon_pixbuf(database, pageURI), 0);
> > +                g_simple_async_result_complete(result);
> > +            }
> 
> Can we move this to a helper in PendingIconRequest?

We would need to check whether it's an async request here, to pass the pixbuf to the helper, since for a frame loader request we don't want to create a pixbuf here.

[Martin Robinson]

(In reply to comment #46)

> > > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:277
> > > +static GdkPixbuf* get_icon_pixbuf(WebKitIconDatabase* database, const String& pageURL)
> > 
> > Should be called something like getIconPixbufSynchronously.
> > 
> > > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:332
> > > +            GSimpleAsyncResult* result = request->asyncResult();
> > > +            ASSERT(result);
> > > +            database->priv->pendingIcons = g_list_remove_link(database->priv->pendingIcons, icons);
> > > +            g_simple_async_result_set_error(result, G_IO_ERROR, G_IO_ERROR_CANCELLED, "%s", "Operation was cancelled");
> > > +            g_simple_async_result_complete(result);
> > > +            delete request;
> > > +            g_list_free(icons);
> > > +            break;
> > 
> > Can this be a helper in PendingIconRequest?
> 
> I'm not sure, we need to remove the request from the list and delete it, the request itself can't be removed from the list unless we pass the list somehow to the PendingIconRequest.

Perhaps the list of PendingIconRequests should just be a static member on PendingIconRequest.


> > > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:379
> > > +    if (cancellable) {
> > > +        request->m_cancellable = cancellable;
> > > +        request->m_cancelledId = g_cancellable_connect(cancellable, G_CALLBACK(webkit_icon_database_get_icon_pixbuf_cancelled), request, 0);
> > > +        g_object_set_data_full(G_OBJECT(result.get()), "cancellable", g_object_ref(cancellable), static_cast<GDestroyNotify>(g_object_unref));
> > > +    }
> > > +
> > 
> > I believe you should just pass the cancellable to the PendingIconRequest constructor and make all this the responsibility of the constructor.
> 
> We need to connect the signal and the callback is part of WebKitIconDatabase, that's why it's here. 

Later, I suggested moving PendingIconRequest entirely into webkiticondatabase.cpp, so this shouldn't be an issue.


> > > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:441
> > > +            database->priv->pendingIcons = g_list_remove_link(database->priv->pendingIcons, icons);
> > > +            if (request->frameLoaderClient())
> > > +                request->frameLoaderClient()->dispatchDidReceiveIcon();
> > > +            else if (request->asyncResult()) {
> > > +                GSimpleAsyncResult* result = request->asyncResult();
> > > +                g_simple_async_result_set_op_res_gpointer(result, get_icon_pixbuf(database, pageURI), 0);
> > > +                g_simple_async_result_complete(result);
> > > +            }
> > 
> > Can we move this to a helper in PendingIconRequest?
> 
> We would need to check whether it's an async request here, to pass the pixbuf to the helper, since for a frame loader request we don't want to create a pixbuf here.

You could just pass the database and an argument to the helper.

[Xan Lopez]

Just ported (most of) Ephy to this. Seems to work OK, a couple of things:

- A getter with the icon_uri might be useful in some cases. Since we allow to get the icon uri for a given page uri, it seems to make sense to have it if possible. At least it would have allowed a more direct conversion in Epiphany, but I'm not sure that this is a representative case.

- Scaling. The EphyFaviconCache kept icons scaled to the size we use in ephy. WebKit just gives you the original icon. If you want some scaling (as ephy does), you'll need to keep a local cache with the scaled icon around. Maybe it makes sense to be able to tell to WebKit that you want icons in a given size and it will do it for you? Otherwise I'll do it in ephy.

Nothing else. Most of the ephy code really expected sync APIs, but since this is an implementation limitation in WebKit we'll have to live with it :)

[Xan Lopez]

Oh, an (unrelated) bug. When loading pages from the page cache ::icon-loaded is not fired, so we don't get a favicon. Should fix that.

[Carlos Garcia Campos]

(In reply to comment #48)
> Just ported (most of) Ephy to this. Seems to work OK, a couple of things:

great! :-)

> - A getter with the icon_uri might be useful in some cases. Since we allow to get the icon uri for a given page uri, it seems to make sense to have it if possible. At least it would have allowed a more direct conversion in Epiphany, but I'm not sure that this is a representative case.

There's webkit_icon_database_get_icon_uri(), or what do you mean? In that case you will always have an uri when the database have been imported, so you can call it in the get_icon_pixbuf_async() callback to make sure.

> - Scaling. The EphyFaviconCache kept icons scaled to the size we use in ephy. WebKit just gives you the original icon. If you want some scaling (as ephy does), you'll need to keep a local cache with the scaled icon around. Maybe it makes sense to be able to tell to WebKit that you want icons in a given size and it will do it for you? Otherwise I'll do it in ephy.

WebCore doesn't scale the icons, if we add api for that it would be the same but calling gdk_pixbuf_scale_simple() like sergio's patch did. So, I'm not sure it's worth adding more API for that, we can simply add a comment in the docs saying that the icon can be scaled using gdk pixbuf api. This way the user can scale it with any of the GdkInterpType, instead of forcing it, or even any other scale method by getting the pixels. 

> 
> Nothing else. Most of the ephy code really expected sync APIs, but since this is an implementation limitation in WebKit we'll have to live with it :)

If you are getting the icons from a thread and you really want to use a sync API because you are not going to block the UI we can add a sync implementation.

[Carlos Garcia Campos]

(In reply to comment #44)
> (From update of attachment 100462 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100462&action=review
> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:87
> > +    GList* pendingIcons;
> > +    gboolean importFinished;
> 
> Please use bool here instead of gboolean. It looks like these values are not initialized during instance initialization. I believe you should do that regardless of whether anything currently depends on it. Would prefer pendingIcons to be called pendingIconRequests and for it to be a WTF::Vector.

is it possible to remove items from a vector while iterating? that's why I used a GList. Maybe a Deque instead of a Vector would work, but I still don't see the problem of using a GList, where we can remove any item without having to move all others (like Deque does)

[Xan Lopez]

(In reply to comment #50)
> > - A getter with the icon_uri might be useful in some cases. Since we allow to get the icon uri for a given page uri, it seems to make sense to have it if possible. At least it would have allowed a more direct conversion in Epiphany, but I'm not sure that this is a representative case.
> 
> There's webkit_icon_database_get_icon_uri(), or what do you mean? In that case you will always have an uri when the database have been imported, so you can call it in the get_icon_pixbuf_async() callback to make sure.

I mean a getter with the icon URI instead of the page URI, in case you only have the former. This was the case sometimes in ephy, and it seems it could be useful in some situations. This depends on the WebCore db allowing to do this though.

> 
> > - Scaling. The EphyFaviconCache kept icons scaled to the size we use in ephy. WebKit just gives you the original icon. If you want some scaling (as ephy does), you'll need to keep a local cache with the scaled icon around. Maybe it makes sense to be able to tell to WebKit that you want icons in a given size and it will do it for you? Otherwise I'll do it in ephy.
> 
> WebCore doesn't scale the icons, if we add api for that it would be the same but calling gdk_pixbuf_scale_simple() like sergio's patch did. So, I'm not sure it's worth adding more API for that, we can simply add a comment in the docs saying that the icon can be scaled using gdk pixbuf api. This way the user can scale it with any of the GdkInterpType, instead of forcing it, or even any other scale method by getting the pixels.

The thing is that this will force a secondary cache in some (most?) applications, which seems to somewhat defeat the idea of giving an API that covers 99% of the use cases. I think telling WebKitGTK+ that you want your icons as 16x16 or whatever, and having WebKitGTK+ keep a cache of its own with the scaled versions might make sense. In either case either WebKit or ephy will have to do it.

[Carlos Garcia Campos]

Created attachment 100799 [details]
New patch according to review

[Gustavo Noronha (kov)]

Comment on attachment 100799 [details]
New patch according to review

Much better =)

[Martin Robinson]

(In reply to comment #51)

> is it possible to remove items from a vector while iterating? that's why I used a GList. Maybe a Deque instead of a Vector would work, but I still don't see the problem of using a GList, where we can remove any item without having to move all others (like Deque does)

You often need to access the pendingIconRequests for a particular pageURI, so it makes sense to store them it as a HashMap of Vectors on the pageURI. When you need to remove one particular request, you don't have to worry about iterating after removal. When you have to delete all requests, you can simply remove that Vector from the hash.

One benefit I see right away is that perhaps you can use OwnPtr to store the elements in the Vector. That way you can avoid manual memory management.

Here's the data structure I mentioned: HashMap<Vector<OwnPtr<PendingIconRquest> > >

[Martin Robinson]

(In reply to comment #55)

> Here's the data structure I mentioned: HashMap<Vector<OwnPtr<PendingIconRquest> > >

Sorry. This should be  HashMap<const char*, Vector<OwnPtr<PendingIconRquest> > >

[Carlos Garcia Campos]

(In reply to comment #56)
> (In reply to comment #55)
> 
> > Here's the data structure I mentioned: HashMap<Vector<OwnPtr<PendingIconRquest> > >
> 
> Sorry. This should be  HashMap<const char*, Vector<OwnPtr<PendingIconRquest> > >

I've tried with that but I can't make it build, wouldn't it be better to use this?

HashMap<String, Vector<OwnPtr<PendingIconRequest> >* >

The only problem I see is that we would still have to manually delete the vector.

[Martin Robinson]

(In reply to comment #57)

> I've tried with that but I can't make it build, wouldn't it be better to use this?

That's pretty odd. What build error did you see?

> HashMap<String, Vector<OwnPtr<PendingIconRequest> >* >

> The only problem I see is that we would still have to manually delete the vector.

Hrm. That doesn't seem like it should be necessary. :/

[Carlos Garcia Campos]

Created attachment 102130 [details]
New patch

It uses HashMap<String, Vector<OwnPtr<PendingIconRequest> >* > instead of GList* for the pending icon requests as suggested by Martin. It also completes the operation in an idle when the icon is already available in webkit_icon_database_get_icon_pixbuf_async() to make sure the function is always async, and it doesn't leak the icon when the operation is cancelled.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testicondatabase.c"
Source/WebKit/gtk/webkit/webkiticondatabase.h:76:  Extra space between GCancellable* and cancellable  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:78:  Extra space between gpointer and user_data  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:82:  Extra space between GAsyncResult* and result  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:83:  Extra space between GError** and error  [whitespace/declaration] [3]
Total errors found: 4 in 9 files


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

[Martin Robinson]

Comment on attachment 102130 [details]
New patch

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

This seems good to me. I have a few suggestions below, but I r+ from me otherwise. We should wait until Xan can test this and approve it before the final r+.

> Source/WebKit/gtk/tests/testicondatabase.c:21
> +

No newline needed here.

> Source/WebKit/gtk/tests/testicondatabase.c:27
> +GMainLoop *loop;

I think it's possible just to use the default main loop instead of this interior main loop.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:78
> +    public:

Typically the public/private/protected labels aren't indented in WebKit code.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:183
> +    HashMap<String, Vector<OwnPtr<PendingIconRequest> >* > pendingIconRequests;

Do you mind adding typedefs for Vector<OwnPtr<PendingIconRequest> >* and  HashMap<String, Vector<OwnPtr<PendingIconRequest> >* > to decrease the verbosity of the code?

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:438
> +    String pageURL = request->pageURI();

I think you should make pageURL be a String& instead to avoid a String copy. You could also define it above along with "database" and use it in when assigning "icons."

[Gustavo Noronha (kov)]

Comment on attachment 102130 [details]
New patch

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

A few comments on the test

> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:664
> +    const gchar* frameURI = webkit_web_frame_get_uri(m_frame);

Xan doesn't like gchar, s/gchar/char/

> Source/WebKit/gtk/tests/testicondatabase.c:145
> +    loadURI("http://www.google.com/");

That is fine, but ideally we should have a SoupServer providing the page and the favicon to avoid having the test depend on externalities like latency and broken network.

> Source/WebKit/gtk/tests/testicondatabase.c:166
> +    gchar *iconURI = webkit_icon_database_get_icon_uri(webkit_get_icon_database(), "http://www.google.com/");
> +    g_assert_cmpstr(iconURI, ==, "http://www.google.es/favicon.ico");

Hrm... this is not going to work. The bots are not necessarily going to be redirected to google.es, nor am I =) So I guess check for the presence of google and having favicon.ico as the suffix, or just use the same approach as testmimehandling.c, and get a SoupServer up!

[Carlos Garcia Campos]

(In reply to comment #62)
> (From update of attachment 102130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102130&action=review
> 
> A few comments on the test
> 
> > Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:664
> > +    const gchar* frameURI = webkit_web_frame_get_uri(m_frame);
> 
> Xan doesn't like gchar, s/gchar/char/

Ok.

> > Source/WebKit/gtk/tests/testicondatabase.c:145
> > +    loadURI("http://www.google.com/");
> 
> That is fine, but ideally we should have a SoupServer providing the page and the favicon to avoid having the test depend on externalities like latency and broken network.

I'll try that way. 

> > Source/WebKit/gtk/tests/testicondatabase.c:166
> > +    gchar *iconURI = webkit_icon_database_get_icon_uri(webkit_get_icon_database(), "http://www.google.com/");
> > +    g_assert_cmpstr(iconURI, ==, "http://www.google.es/favicon.ico");
> 
> Hrm... this is not going to work. The bots are not necessarily going to be redirected to google.es, nor am I =) So I guess check for the presence of google and having favicon.ico as the suffix, or just use the same approach as testmimehandling.c, and get a SoupServer up!

oops :-P

[Carlos Garcia Campos]

Comment on attachment 102130 [details]
New patch

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

>> Source/WebKit/gtk/tests/testicondatabase.c:27
>> +GMainLoop *loop;
> 
> I think it's possible just to use the default main loop instead of this interior main loop.

What's the default main loop?

[Carlos Garcia Campos]

Created attachment 106123 [details]
Updated patch

It uses a soup server for the tests and fixes other issues commented by Gustavo and Martin.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testicondatabase.c"
Source/WebKit/gtk/webkit/webkiticondatabase.h:76:  Extra space between GCancellable* and cancellable  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:78:  Extra space between gpointer and user_data  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:82:  Extra space between GAsyncResult* and result  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:83:  Extra space between GError** and error  [whitespace/declaration] [3]
Total errors found: 4 in 9 files


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

[Martin Robinson]

Comment on attachment 102130 [details]
New patch

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

>>> Source/WebKit/gtk/tests/testicondatabase.c:27
>>> +GMainLoop *loop;
>> 
>> I think it's possible just to use the default main loop instead of this interior main loop.
> 
> What's the default main loop?

There should already be a main loop running. For instance, in testatk.c it manually spins the default GMainContext. I'm not sure if this approach is feasible here. What do you think?

[Carlos Garcia Campos]

(In reply to comment #67)
> (From update of attachment 102130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102130&action=review
> 
> >>> Source/WebKit/gtk/tests/testicondatabase.c:27
> >>> +GMainLoop *loop;
> >> 
> >> I think it's possible just to use the default main loop instead of this interior main loop.
> > 
> > What's the default main loop?
> 
> There should already be a main loop running. For instance, in testatk.c it manually spins the default GMainContext. I'm not sure if this approach is feasible here. What do you think?

No, there isn't any main loop running, that's why testak needs to manually iterate the default GMainContext. I don't know why testatk needs this, but I think it's easier to use a GMainLoop like most of the other tests do.

[Martin Robinson]

Comment on attachment 106123 [details]
Updated patch

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

Sorry for the long wait for the review. r-, because of some nits and modifying a hash while iterating through it. If you fix all this little stuff though, it's r+. We just need another review to approve the API.

> Source/WebKit/gtk/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +

Please put a short overview of your change here.

> Source/WebKit/gtk/ChangeLog:47
> +        (server_callback):
> +        (deleteDatabaseFileIfExists):
> +        (testWebkitIconDatabaseSetPath):
> +        (iconDatabaseGetIconValidCallback):
> +        (iconDatabaseGetIconInvalidCallback):
> +        (iconDatabaseGetIconCancelledCallback):
> +        (idleQuitLoopCallback):
> +        (loadURI):
> +        (iconDatabaseGetIconValidIdle):
> +        (iconDatabaseGetIconInvalidIdle):
> +        (iconDatabaseGetIconCancelledIdle):
> +        (testWebkitIconDatabaseGetIconAsync):
> +        (testWebkitIconDatabaseGetIconURI):
> +        (testWebkitIconDatabaseRemoveAll):
> +        (testWebkitIconDatabaseCloseDatabase):
> +        (main):
> +        * webkit/webkiticondatabase.cpp:
> +        (IconDatabaseClientGtk::performImport):
> +        (IconDatabaseClientGtk::didRemoveAllIcons):
> +        (IconDatabaseClientGtk::didImportIconURLForPageURL):
> +        (IconDatabaseClientGtk::didImportIconDataForPageURL):
> +        (IconDatabaseClientGtk::didChangeIconForPageURL):
> +        (IconDatabaseClientGtk::didFinishURLImport):
> +        (PendingIconRequest::PendingIconRequest):
> +        (PendingIconRequest::~PendingIconRequest):
> +        (PendingIconRequest::pageURI):
> +        (PendingIconRequest::frameLoaderClient):
> +        (PendingIconRequest::asyncResult):
> +        (PendingIconRequest::asyncResultComplete):
> +        (PendingIconRequest::asyncResultCompleteInIdle):
> +        (PendingIconRequest::asyncResultCancel):
> +        (PendingIconRequest::frameLoaderDispatchDidReceiveIcon):
> +        (webkitIconDatabaseIconLoaded): Retain the icon when loaded to

Sicne the method descriptions are all empty, they are just noise. In these situations I think it's fine to just leave the filename.

> Source/WebKit/gtk/tests/testicondatabase.c:33
> +static void
> +server_callback(SoupServer *server, SoupMessage *msg,
> +                const char *path, GHashTable *query,
> +                SoupClientContext *context, gpointer data)

Extra newlines here. No need to split the signature across multiple lines.

> Source/WebKit/gtk/tests/testicondatabase.c:89
> +    GError *error = 0;

0 should be NULL here.

> Source/WebKit/gtk/tests/testicondatabase.c:142
> +    webkit_web_view_load_uri(view, uri);
> +    //    g_signal_connect(view, "icon-loaded", G_CALLBACK(idleQuitLoopCallback), NULL);
> +    g_signal_connect(view, "notify::load-status", G_CALLBACK(idleQuitLoopCallback), NULL);

If this line isn't needed, just remove it instead of commenting it out.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:71
> +struct PendingIconRequest;

You should say:

class PendingIconRequest.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:146
> +        g_simple_async_result_set_error(m_asyncResult.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED, "%s", "Operation was cancelled");

The string "Operation was cancelled" is missing the l10n underscore.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:181
> +typedef HashMap<String, PendingIconRequestVector* > PendingIconRequestMap;

Extra space after PendingIconRequestVector*

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:392
> +    // We must pass something greater than 0, 0 to get a pixbuf.

0, 0 -> 0x0 or 0 by 0

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:440
> +    database->priv->pendingIconRequests.remove(pageURL);
> +    delete requests;

I think I'd rather this take an iterator or just a pageURI, but not both the value and the index. Maybe just a pageURI. You could do:

iterator toDelete = pendingIconRequests.remove(pageURL);
delete toDelete->second;
database->priv->pendingIconRequests.remove(toDelete);

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:467
> +static void webkitIconDatabaseGetIconPixbufCancelled(GCancellable* cancellable, PendingIconRequest* request)
> +{
> +    // Handle cancelled in a in idle since it might be called from any thread.
> +    g_idle_add(getIconPixbufCancelledIdle, request);
> +}

I think it would be better to use the wtf::callOnMainThread here.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:483
> + * This is an asynchronous method. When the operation is finished, callback will
> + * be invoked. You can then call webkit_icon_database_get_icon_pixbuf_finish()
> + * to get the result of the operation.
> + * See also webkit_icon_database_get_icon_pixbuf().

I'm not sure I like the use of GIO style APIs here. At this point though, we should get this patch in and then reconsider the APIs for WebKit2.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:485
> + * Since: 1.5.3

This will need to change now. :(

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:495
> +    String pageURL(String::fromUTF8(pageURI));

I think the preferred style is:  String pageURL = String::fromUTF8(pageURI);

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:505
> +        // Import is ongoing, there might be an icon.

I think a comment like: " Import is ongoing, the icon we want might load later." would be less confusing.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:527
> + * Since: 1.5.3

Should change again.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:539
> +        g_set_error_literal(error, G_IO_ERROR, G_IO_ERROR_CANCELLED, "Operation was cancelled");

I think "Operation was  cancelled" needs to be prefixed by an underscore for l10n.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:544
> +    return icon ? static_cast<GdkPixbuf*>(g_object_ref(icon)) : 0;

I think it's clearer here to use an early return:
if (!icon)
    return 0;
return g_object_ref(icon);

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:547
> +void webkitIconDatabaseRegisterForIconNotification(WebKitIconDatabase* database, const gchar* pageURI, WebCore::FrameLoaderClient* client)

I'd rather this took a const String& pageURI instead of converting it here and in webkitIconDatabaseUnregisterForIconNotification.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:557
> +void webkitIconDatabaseUnregisterForIconNotification(WebKitIconDatabase* database, const gchar* pageURI, WebCore::FrameLoaderClient* client)
> +{
> +    String pageURL(String::fromUTF8(pageURI));

Ditto.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:601
> +    // requests we only need to delete the request so that icon-loaded won't be emitted.
> +    PendingIconRequestMap::const_iterator end = database->priv->pendingIconRequests.end();
> +    for (PendingIconRequestMap::const_iterator iter = database->priv->pendingIconRequests.begin(); iter != end; ++iter) {
> +        String iconURL = WebCore::iconDatabase().synchronousIconURLForPageURL(iter->first);
> +        if (!iconURL.isEmpty())
> +            continue;
> +
> +        PendingIconRequestVector* icons = iter->second;
> +        for (size_t i = 0; i < icons->size(); ++i) {
> +            PendingIconRequest* request = icons->at(i).get();
> +            if (request->asyncResult())
> +                request->asyncResultComplete(0);
> +        }
> +
> +        webkitIconDatabaseDeleteRequests(database, icons, iter->first);

I'm not sure it's safe to modify the HashSet while you are using an iterator to it. Better to keep a list of defunct pageURIs and then delete them afterward.

> Source/WebKit/gtk/webkit/webkiticondatabaseprivate.h:30
> +extern "C" {
> +void webkitIconDatabaseRegisterForIconNotification(WebKitIconDatabase*, const gchar* pageURI, WebCore::FrameLoaderClient*);
> +void webkitIconDatabaseUnregisterForIconNotification(WebKitIconDatabase*, const gchar* pageURI, WebCore::FrameLoaderClient*);
> +}

Please omit the extern "C". It's fine that these are mangled.

[Eric Seidel (no email)]

This bug has gone epic.  I recommend creating a new bug with an updated patch if you wish to continue work on this.  It requires too much mental effort to figure out what's going on here, drastically reducing the chance of a successful review.

[Eric Seidel (no email)]

Comment on attachment 95879 [details]
Patch

Cleared review? from attachment 95879 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Sergio Villar Senin]

reopening as I resumed the work on this issue.

[Sergio Villar Senin]

Created attachment 127433 [details]
Patch

[Philippe Normand]

Comment on attachment 127433 [details]
Patch

Attachment 127433 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11541154

[Sergio Villar Senin]

Created attachment 127545 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #75)
> Created an attachment (id=127545) [details]
> Patch

The previous patch had an error in the gtk-doc and the two new API functions were missing from webkitgtk-sections.txt

[Philippe Normand]

Comment on attachment 127545 [details]
Patch

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

This looks quite good in general but I think a more experienced reviewer should look at it, this is not exactly my field of expertise. You might also want to use 2012 in the copyright headers :)

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:465
> +    // Handle cancelled in a in idle since it might be called from any thread.

you mean "in an idle" I guess?

[Sergio Villar Senin]

Created attachment 130396 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #78)
> Created an attachment (id=130396) [details]
> Patch

I've just realized that I forgot to change the libtool versions. What was the policy anyway?

BTW this patch does not purge retained icons yet. I'll try to come up with something now but I uploaded this because I'm not sure that is a blocker (unless we'd like to add some API for that).

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testicondatabase.c"
Source/WebKit/gtk/webkit/webkitwebview.h:435:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:436:  Extra space between guint and height  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:72:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:73:  Extra space between guint and height  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:78:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:79:  Extra space between guint and height  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:80:  Extra space between GCancellable* and cancellable  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:82:  Extra space between gpointer and user_data  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:86:  Extra space between GAsyncResult* and result  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:87:  Extra space between GError** and error  [whitespace/declaration] [3]
Total errors found: 10 in 13 files


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

[Martin Robinson]

I think it's important that we don't let the database grow indefinitely. We also need to figure out how Epiphany can reasonably use such an API.

[Sergio Villar Senin]

(In reply to comment #81)
> I think it's important that we don't let the database grow indefinitely. We also need to figure out how Epiphany can reasonably use such an API.

No problem, I'm working on that. It'd be quite easy in fact. Do you think we should any any API for that or just set a fixed amount of time?

[Martin Robinson]

(In reply to comment #82)

> No problem, I'm working on that. It'd be quite easy in fact. Do you think we should any any API for that or just set a fixed amount of time?

I think it makes sense for the application to have control over when icons are released.

[Martin Robinson]

Comment on attachment 130396 [details]
Patch

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

Not a thorough, review, but here are a couple things I noticed...

> Source/WebKit/gtk/tests/testicondatabase.c:2
> + * Copyright (C) 2011 Igalia S.L.

All these copyrights should be 2012 now.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:268
> +      * main frame of a page.See #WebKitWebView::icon-loaded if you

Missing a space after the period.

[Xan Lopez]

Comment on attachment 130396 [details]
Patch

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

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:413
> +        pixbuf = gdk_pixbuf_scale_simple(pixbuf.get(), width, height, GDK_INTERP_BILINEAR);

OK, so the icon is cached at the original size, and we rescale it every single time. This does not seem to be a great solution. Probably the UI is going to show it again and again at a given size, and resizing it every single time makes no sense. So we should either cache it scaled, or make it very explicit that the client should cache it on their side.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:433
> + * maximum available size for the icon.

This is missing from the _async method.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:510
> + * See also webkit_icon_database_get_icon_pixbuf_sync().

Should mention at what size the icon is cached, if at all.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:521
> +                                                                             reinterpret_cast<gpointer>(webkit_icon_database_get_icon_pixbuf)));

I'm just reading the documentation here, but it seems you should be passing an async. function here, not the sync one. If it's only used as a 'tag' I guess it's not important, but shrug.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:540
> +        // Import is ongoing, there icon we want might load later.

s/there/the/ ? Not totally sure what you want to say here though, tbh.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:549
> +        webkitIconDatabaseDeleteRequests(database, icons, pageURL);

Hrm, OK, I guess I'm not getting how this works, but why do you remove the request immediately after asking it to be completed in an idle? Because it's no longer pending?

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:567
> +    g_warn_if_fail(g_simple_async_result_get_source_tag(simpleResult) == webkit_icon_database_get_icon_pixbuf);

Shouldn't this be an error (g_return_if_fail)? And the same comment about the sync vs. async I guess.

> Source/WebKit/gtk/webkit/webkiticondatabaseprivate.h:25
> +}

This is not needed?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5048
> + * and call this method in the callback.

Hrm, so isn't this a bit duplicated with the sync. method in the icon database? Is it just meant to be a helper method? I'm not sure it's worth breaking the API for this. Also, here or elsewhere it should be explained what happens with the icon of the size you have requested: does webkit cache it at the original size, the new one, both?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5051
>   * be resized before it is displayed.

It seems this comment is obsolete now.

> Tools/GtkLauncher/main.c:219
> +    g_free(iconDatabasePath);

If everybody is going to have to do this perhaps we should have a way of enabling/disabling the db different than setting the path, and a default path that is exactly this. If you then want to set another path, you can do that.

[Sergio Villar Senin]

(In reply to comment #85)
> (From update of attachment 130396 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130396&action=review
> 
> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:413
> > +        pixbuf = gdk_pixbuf_scale_simple(pixbuf.get(), width, height, GDK_INTERP_BILINEAR);
> 
> OK, so the icon is cached at the original size, and we rescale it every single time. This does not seem to be a great solution. Probably the UI is going to show it again and again at a given size, and resizing it every single time makes no sense. So we should either cache it scaled, or make it very explicit that the client should cache it on their side.

I'm fine with any solution.

> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:433
> > + * maximum available size for the icon.
> 
> This is missing from the _async method.

Indeed.

> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:510
> > + * See also webkit_icon_database_get_icon_pixbuf_sync().
> 
> Should mention at what size the icon is cached, if at all.

OK.
 
> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:521
> > +                                                                             reinterpret_cast<gpointer>(webkit_icon_database_get_icon_pixbuf)));
> 
> I'm just reading the documentation here, but it seems you should be passing an async. function here, not the sync one. If it's only used as a 'tag' I guess it's not important, but shrug.

True.

> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:540
> > +        // Import is ongoing, there icon we want might load later.
> 
> s/there/the/ ? Not totally sure what you want to say here though, tbh.

It's "the".

> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:549
> > +        webkitIconDatabaseDeleteRequests(database, icons, pageURL);
> 
> Hrm, OK, I guess I'm not getting how this works, but why do you remove the request immediately after asking it to be completed in an idle? Because it's no longer pending?

Exactly, is no longer pending because it's already resolved. Just waiting for the idle to be fired.

> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:567
> > +    g_warn_if_fail(g_simple_async_result_get_source_tag(simpleResult) == webkit_icon_database_get_icon_pixbuf);
> 
> Shouldn't this be an error (g_return_if_fail)? And the same comment about the sync vs. async I guess.

It should.

> > Source/WebKit/gtk/webkit/webkiticondatabaseprivate.h:25
> > +}
> 
> This is not needed?

Oops I left it there by mistake. There were some other calls before.

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:5048
> > + * and call this method in the callback.
> 
> Hrm, so isn't this a bit duplicated with the sync. method in the icon database? Is it just meant to be a helper method? I'm not sure it's worth breaking the API for this. Also, here or elsewhere it should be explained what happens with the icon of the size you have requested: does webkit cache it at the original size, the new one, both?

Well that method was there before. The difference with the one in the icon database is that the webview returns the url of the icon instead of the page's. Regarding the size WK only caches the icon at its original size.

> > Tools/GtkLauncher/main.c:219
> > +    g_free(iconDatabasePath);
> 
> If everybody is going to have to do this perhaps we should have a way of enabling/disabling the db different than setting the path, and a default path that is exactly this. If you then want to set another path, you can do that.

I kind of disagree with that. Take into account that apps that use WK as a mere document viewer wouldn't be most likely interested in favicons.

[Sergio Villar Senin]

Created attachment 130639 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testicondatabase.c"
Source/WebKit/gtk/webkit/webkitwebview.h:435:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:436:  Extra space between guint and height  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:73:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:74:  Extra space between guint and height  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:79:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:80:  Extra space between guint and height  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:81:  Extra space between GCancellable* and cancellable  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:83:  Extra space between gpointer and user_data  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:87:  Extra space between GAsyncResult* and result  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkiticondatabase.h:88:  Extra space between GError** and error  [whitespace/declaration] [3]
Total errors found: 10 in 14 files


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

[Sergio Villar Senin]

Created attachment 130683 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:73:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:74:  Extra space between guint and height  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:79:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:80:  Extra space between guint and height  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:81:  Extra space between GCancellable* and cancellable  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:82:  Extra space between GAsyncReadyCallback and callback  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:83:  Extra space between gpointer and user_data  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:87:  Extra space between GAsyncResult* and result  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:88:  Extra space between GError** and error  [whitespace/declaration] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testfavicondatabase.c"
Source/WebKit/gtk/webkit/webkitwebview.h:438:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:439:  Extra space between guint and height  [whitespace/declaration] [3]
Total errors found: 11 in 18 files


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

[Gustavo Noronha (kov)]

Comment on attachment 130683 [details]
Patch

Attachment 130683 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11849389

[Xan Lopez]

Comment on attachment 130683 [details]
Patch

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

Looks good to me. I think maybe you could split the expiration stuff in a second patch, since it's an improvement over the first iteration and would make it easier to review it. I'm marking r- to not be the  "scumbag reviewer" that makes comments and does not mark the patch.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:46
> + * SECTION:webkitwebdatabase

webkitfavicondatabase

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:47
> + * @short_description: A WebKit web application database

Hrm, a favicon database right? Copy&pasta error I guess.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:51
> + * the same icon database.

I don't think it makes much sense to use browser UI terms to explain what a favicon is here. Just explain in technically, or not at all.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:54
> + * loaded, you will need to set an icon database path using

s/loaded/stored/ ?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:60
> + * images found into the memory cache if possible.

And I'd put the paragraph about enabling storage after this.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:112
> +    }

Hrm, I suppose you need to define this because C++ complains otherwise?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:136
> +            return;

What does it mean exactly to 'return;' from a ctor? Shouldn't you ASSERT or something?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:139
> +        g_object_set_data_full(G_OBJECT(result), "cancellable", g_object_ref(cancellable), static_cast<GDestroyNotify>(g_object_unref));

Hrm, why do you need to do this if the class has both objects as members?

Added later: OK, you use it in _finish. Where is it unrefed exactly?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:221
> +}

This is doing nothing ,no need to define it.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:283
> +      * only need the favicon of a particular #WebKitWebView.

This is confusing. Is it only for the main frame or any frame? @frame_uri: says "the frame" and later you say main frame.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:307
> +    new (database->priv) WebKitFaviconDatabasePrivate();

Hrm, isn't this smashing the stuff you get from database->priv ? If you are using the new() thing do you need the GObject system?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:314
> +    if (database->priv->expirationTimesDatabase) {

if (!database->...) and early return?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:356
> +{

Perhaps check with argc before accessing argv[0], just in case.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:370
> +    int lastValidTime = (int) currentTime() - ICON_EXPIRATION_TIME;

No space for the casting, right?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:496
> + * to WebKitFaviconDatabase::icon-loaded and call this in the callback.

s/call this/use this function/?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:500
> + * different size the icon will be scaled on every single call to this

I'd say better "will be scaled each time you call this function".

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:618
> +

Can we add a comment here about removing the request we just created? It's confusing otherwise.

[Sergio Villar Senin]

(In reply to comment #92)
> (From update of attachment 130683 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130683&action=review
> 
> Looks good to me. I think maybe you could split the expiration stuff in a second patch, since it's an improvement over the first iteration and would make it easier to review it. I'm marking r- to not be the  "scumbag reviewer" that makes comments and does not mark the patch.

OK.

> > Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:54
> > + * loaded, you will need to set an icon database path using
> 
> s/loaded/stored/ ?

Actually both. Will change that.

> > Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:112
> > +    }
> 
> Hrm, I suppose you need to define this because C++ complains otherwise?

Yes because they're pure virtual functions in the interface.
 
> > Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:136
> > +            return;
> 
> What does it mean exactly to 'return;' from a ctor? Shouldn't you ASSERT or something?

Absolutely, will add an ASSERT

> > Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:139
> > +        g_object_set_data_full(G_OBJECT(result), "cancellable", g_object_ref(cancellable), static_cast<GDestroyNotify>(g_object_unref));
> 
> Hrm, why do you need to do this if the class has both objects as members?
> 
> Added later: OK, you use it in _finish. Where is it unrefed exactly?

The destroy notify function will do that once the async result is finalized.

> > Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:283
> > +      * only need the favicon of a particular #WebKitWebView.
> 
> This is confusing. Is it only for the main frame or any frame? @frame_uri: says "the frame" and later you say main frame.

it's only the main frame. I'll try to rephrase it.
 
> > Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:307
> > +    new (database->priv) WebKitFaviconDatabasePrivate();
> 
> Hrm, isn't this smashing the stuff you get from database->priv ? If you are using the new() thing do you need the GObject system?

AFAIK the placement new syntax just uses the memory you have previously allocated.

> > Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:356
> > +{
> 
> Perhaps check with argc before accessing argv[0], just in case.

Well the callback won't be called if there is no matching row, and a select needs at least one row. Anyway I guess we can put an ASSERT there.

[Xan Lopez]

(In reply to comment #93)
> > Hrm, isn't this smashing the stuff you get from database->priv ? If you are using the new() thing do you need the GObject system?
> 
> AFAIK the placement new syntax just uses the memory you have previously allocated.

Ah, but it calls the ctor so that you can use C++ magic, I guess. I think I was already confused by this once back in the day!

[Sergio Villar Senin]

Created attachment 131361 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:73:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:74:  Extra space between guint and height  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:79:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:80:  Extra space between guint and height  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:81:  Extra space between GCancellable* and cancellable  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:82:  Extra space between GAsyncReadyCallback and callback  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:83:  Extra space between gpointer and user_data  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:87:  Extra space between GAsyncResult* and result  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:88:  Extra space between GError** and error  [whitespace/declaration] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testfavicondatabase.c"
Source/WebKit/gtk/webkit/webkitwebview.h:438:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:439:  Extra space between guint and height  [whitespace/declaration] [3]
Total errors found: 11 in 18 files


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

[Gustavo Noronha (kov)]

Comment on attachment 131361 [details]
Patch

Attachment 131361 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11947044

[Sergio Villar Senin]

Created attachment 131879 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 131879 [details]
Patch

Attachment 131879 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11951312

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:73:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:74:  Extra space between guint and height  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:79:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:80:  Extra space between guint and height  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:81:  Extra space between GCancellable* and cancellable  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:82:  Extra space between GAsyncReadyCallback and callback  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:83:  Extra space between gpointer and user_data  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:87:  Extra space between GAsyncResult* and result  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitfavicondatabase.h:88:  Extra space between GError** and error  [whitespace/declaration] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testfavicondatabase.c"
Source/WebKit/gtk/webkit/webkitwebview.h:438:  Extra space between guint and width  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:439:  Extra space between guint and height  [whitespace/declaration] [3]
Total errors found: 11 in 18 files


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

[Xan Lopez]

Comment on attachment 131879 [details]
Patch

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

I think this patch is basically OK as it is, I just have a few minor nitpicks. So in principle r1/2=me, waiting for Martin or other reviewer to jump in.

> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:806
> +    // IconController load icons only for the main frame.

s/load/loads/

> Source/WebKit/gtk/tests/testfavicondatabase.c:68
> +    }

You are leaking databaseFilename if the unlink works.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:269
> +      * only need the favicon of a particular #WebKitWebView.

Isn't this just the same? What's the difference between the favicon for the mainframe and the view?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:273
> +      * of the favicon.

This seems more like the actual difference between the signals.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:493
> + * @cancellable: A #GCancellable or %NULL.

Also (allow-none), right?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:586
> +    return static_cast<GdkPixbuf*>(g_object_ref(icon));

I checked briefly the life cycle of this and it seems you could just return the icon as-is here and remove the extra g_object_unref destroy notifies when setting up things?

[Sergio Villar Senin]

(In reply to comment #101)
> (From update of attachment 131879 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131879&action=review
> 
> I think this patch is basically OK as it is, I just have a few minor nitpicks. So in principle r1/2=me, waiting for Martin or other reviewer to jump in.

Thx Xan for the review. I fixed locally the issues you mentioned (plus a couple of missing "allow-none"). Let's see if some other reviewer approves it.

[Martin Robinson]

Comment on attachment 131879 [details]
Patch

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

Okay. Looks good. I see lots of style errors so be sure to run check-webkit-style directly on the new files to find the rest of them. Please fix them all before landing and considering addressing the lingering questions Xan and I have.

> Source/WebKit/gtk/tests/testfavicondatabase.c:26
> +#define ICON_SIZE 16

You shoulduse const int gIconSize here instead of a #define.

> Source/WebKit/gtk/tests/testfavicondatabase.c:30
> +GMainLoop *loop;
> +char* base_uri;

Asterisk craziness!

base_uri -> baseURI

> Source/WebKit/gtk/tests/testfavicondatabase.c:33
> +static void
> +server_callback(SoupServer *server, SoupMessage *msg, const char *path, GHashTable *query, SoupClientContext *context, gpointer data)

Even though this is C code, you should still use WebKit coding style so this declaration should be on one line and you should use camelCase and non-abbreviated names so:

server_callback -> serverCallback
msg -> message

> Source/WebKit/gtk/tests/testfavicondatabase.c:45
> +
> +    if (g_str_equal(path, "/favicon.ico")) {

You should define length and contents here, right before you use them.

> Source/WebKit/gtk/tests/testfavicondatabase.c:73
> +static void testWebKitFaviconDatabaseSetPath(void)

Omit the void argument here.

> Source/WebKit/gtk/tests/testfavicondatabase.c:89
> +static void faviconDatabaseGetFaviconValidCallback(GObject *sourceObject, GAsyncResult *result, gpointer userData)

faviconDatabaseGetValidFaviconCallback?

> Source/WebKit/gtk/tests/testfavicondatabase.c:102
> +static void faviconDatabaseGetFaviconInvalidCallback(GObject *sourceObject, GAsyncResult *result, gpointer userData)

faviconDatabaseGetInvalidFaviconCallback?

> Source/WebKit/gtk/tests/testfavicondatabase.c:127
> +static inline void mainLoopQuitIfLoadCompleted(gboolean* iconOrPageLoaded)

quitMainLoopIfLoadCompleted

> Source/WebKit/gtk/tests/testfavicondatabase.c:135
> +static void idleQuitLoopCallback(WebKitWebView *webView, GParamSpec *pspec, gboolean* iconOrPageLoaded)

pspec -> paramSpec

> Source/WebKit/gtk/tests/testfavicondatabase.c:143
> +static void webkitWebViewIconLoaded(WebKitFaviconDatabase* database, const char* frameURI, gboolean* iconOrPageLoaded)

The asterisks are in the wrong place here.

> Source/WebKit/gtk/tests/testfavicondatabase.c:163
> +static gboolean faviconDatabaseGetFaviconValidIdle(gpointer userData)

faviconDatabaseGetValidFaviconIdle

> Source/WebKit/gtk/tests/testfavicondatabase.c:167
> +    webkit_favicon_database_get_favicon_pixbuf(webkit_get_favicon_database(), base_uri,
> +					       ICON_SIZE, ICON_SIZE, NULL,
> +					       faviconDatabaseGetFaviconValidCallback, userData);

The indentation seems off here.

> Source/WebKit/gtk/tests/testfavicondatabase.c:171
> +static gboolean faviconDatabaseGetFaviconInvalidIdle(gpointer userData)

faviconDatabaseGetInvalidFaviconIdle

> Source/WebKit/gtk/tests/testfavicondatabase.c:176
> +					       ICON_SIZE, ICON_SIZE, NULL,
> +					       faviconDatabaseGetFaviconInvalidCallback, userData);
> +    return FALSE;

The indetation looks off here as well.

> Source/WebKit/gtk/tests/testfavicondatabase.c:184
> +    webkit_favicon_database_get_favicon_pixbuf(webkit_get_favicon_database(), base_uri,
> +					       ICON_SIZE, ICON_SIZE, cancellable,
> +					       faviconDatabaseGetFaviconCancelledCallback, userData);

Ditto.

> Source/WebKit/gtk/tests/testfavicondatabase.c:190
> +static void testWebKitFaviconDatabaseGetFavicon(void)

Omit the second "void"

> Source/WebKit/gtk/tests/testfavicondatabase.c:196
> +    /* Load uri to make sure favicon is added to database */

Missing a period on this comment.

> Source/WebKit/gtk/tests/testfavicondatabase.c:246
> +    SoupServer* server;

Define this where you first use it:

SoupServer *server = soup_server...

> Source/WebKit/gtk/tests/testfavicondatabase.c:247
> +    SoupURI* soup_uri;

soup_uri -> soupURI

Your asterisk is in the wrong place. Define this where you first use it.

> Source/WebKit/gtk/tests/testfavicondatabase.c:249
> +    gtk_test_init(&argc, &argv, 0);

You should use NULL here instead of 0. This could very well crash a 64-bit machine.

> Source/WebKit/gtk/tests/testfavicondatabase.c:252
> +    /* Hopefully make test independent of the path it's called from. */
> +    testutils_relative_chdir("Source/WebKit/gtk/tests/resources/test.html", argv[0]);

Hopefully make test -> This hopefully makes the test

You might double-check that this works during make distcheck

> Source/WebKit/gtk/tests/testfavicondatabase.c:254
> +    server = soup_server_new(SOUP_SERVER_PORT, 0, NULL);

What does it meant to pass zero for the port here? That might deserve a comment.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:48
> + * a particular Web page or Web site.

In this circumstance, web is not typically capitalized. You can probably just say:

#WebKitFaviconDatabase provides access to the icons associated with web sites.

The way it is now suggests that each page or site has multiple icons.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:52
> + * images found into the memory cache if possible.

into the memory cache -> into a memory cache

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:57
> + * The database is disabled by default. In order for icons to be
> + * stored and accessed, you will need to set an icon database path
> + * using webkit_favicon_database_set_path(). Disable the database
> + * again passing %NULL to the previous call.

I think it's good to be explicit that when the database is enabled, the in memory cache is also frozen to an on-disk database. It's a bit unclear. You should probably put that clarification at the end of the previous paragraph.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:61
> + * If WebKitWebSettings::enable-private-browsing is %TRUE new icons
> + * won't be added to the database on disk and no existing icons will
> + * be deleted from it.

What about the in-memory cache?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:64
> + * Icons will be automatically purged after 4 days without being
> + * used.

Is this still true?

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:81
> +

Extra newline here.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:87
> +    // Called when the an icon is requested while the initial
> +    // import is going on.

Called when the an icon -> Called when an icon

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:165
> +    String m_pageURI;

This is not API, so this should probably be call m_pageURL and the accessor updated to match.

>> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:269
>> +      * only need the favicon of a particular #WebKitWebView.
> 
> Isn't this just the same? What's the difference between the favicon for the mainframe and the view?

I guess he's making the distinction between something that can happen to any WebView and a particular instance of a WebView. Agree that it's a bit unclear here. Perhaps change  

See #WebKitWebView::icon-loaded if you only need the favicon of a particular #WebKitWebView.

to

This signal is fired if an icon is loaded on any #WebKitWebView. If you are only interested in a particular #WebKitWebView see #WebKitWebView::icon-loaded.

>> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:273
>> +      * of the favicon.
> 
> This seems more like the actual difference between the signals.

Out of curiosity, what's the reason for this difference? It seems very arbitrary.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:283
> +                                                              0,
> +                                                              NULL,
> +                                                              NULL,
> +                                                              webkit_marshal_VOID__STRING,

ELIMINATE NULL. ;)

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:314
> + **/

I think you can omit the first asterisk.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:341
> + **/

Ditto.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:350
> +    if (!(path && path[0])) {

I think that popping open a nice cold can of DeMorgan's Law on this makes it a little clearer:

if (!path || !path[0]) {

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:378
> + **/

Can remove one asterisk.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:407
> +    if (width && height && (icon->width() != (int) width || icon->height() != (int) height))

Please use static_cast here instead of C-style casts.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:427
> + * If (@width, @height) is (0, 0) then this method will return the

I think this reads better as "If @width and @height are both zero." (0, 0) can be confused as vector notation.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:500
> + * webkit_favicon_database_try_get_favicon_pixbuf() is that it always return the

always return -> always returns

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:509
> + * If (@width, @height) is (0, 0) then this method will return the

Same comment as above.

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:540
> +    GdkPixbuf* pixbuf = getIconPixbufSynchronously(database, pageURL, 16, 16);

Why do you pass 16 pixels here for the width and height? Won't this resize the icon to something other than @width and @height?

>> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:586
>> +    return static_cast<GdkPixbuf*>(g_object_ref(icon));
> 
> I checked briefly the life cycle of this and it seems you could just return the icon as-is here and remove the extra g_object_unref destroy notifies when setting up things?

Good point. If you remove the new reference be sure to change the annotation above.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5176
> +    webkitWebViewRegisterForIconNotification(webView, FALSE);

You should use false here.

[Sergio Villar Senin]

Comment on attachment 131879 [details]
Patch

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

Thanks for the review, and sorry for all the style errors. The check-webkit-style script ignore the unit tests, that's why I haven't paid much attention to that file.

>> Source/WebKit/gtk/tests/testfavicondatabase.c:254
>> +    server = soup_server_new(SOUP_SERVER_PORT, 0, NULL);
> 
> What does it meant to pass zero for the port here? That might deserve a comment.

Nothing special. We do it for every unit test.

>> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:61
>> + * be deleted from it.
> 
> What about the in-memory cache?

Added a comment clarifying this. The in-memory cache keeps storing the icons during the execution.

>> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:64
>> + * used.
> 
> Is this still true?

It's not. I forgot to remove it when I split the patch.

>>> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:273
>>> +      * of the favicon.
>> 
>> This seems more like the actual difference between the signals.
> 
> Out of curiosity, what's the reason for this difference? It seems very arbitrary.

No reason AFAIK. That difference was there in the old IconDatabase.

>> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:540
>> +    GdkPixbuf* pixbuf = getIconPixbufSynchronously(database, pageURL, 16, 16);
> 
> Why do you pass 16 pixels here for the width and height? Won't this resize the icon to something other than @width and @height?

Although it indeed looks like it will resize the icon, the truth is that WebCore never does that and blindly ignores the width and height passed as arguments. We just need to pass values >0 and we'll get the maximum available size for the icon. Anyway I'll replace them with width and height and add a comment explaining this (I thought I had written that somewhere)

>>> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:586
>>> +    return static_cast<GdkPixbuf*>(g_object_ref(icon));
>> 
>> I checked briefly the life cycle of this and it seems you could just return the icon as-is here and remove the extra g_object_unref destroy notifies when setting up things?
> 
> Good point. If you remove the new reference be sure to change the annotation above.

Why? It's still a new reference. We are just saving a ref/unref cycle.

[Sergio Villar Senin]

(In reply to comment #104)
> (From update of attachment 131879 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131879&action=review
> >> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:540
> >> +    GdkPixbuf* pixbuf = getIconPixbufSynchronously(database, pageURL, 16, 16);
> > 
> > Why do you pass 16 pixels here for the width and height? Won't this resize the icon to something other than @width and @height?
> 
> Although it indeed looks like it will resize the icon, the truth is that WebCore never does that and blindly ignores the width and height passed as arguments. We just need to pass values >0 and we'll get the maximum available size for the icon. Anyway I'll replace them with width and height and add a comment explaining this (I thought I had written that somewhere)

Actually I found that we were not using the size provided by the caller in one case (when asking for icons before the database import). Fixed that.

[Sergio Villar Senin]

Committed r110999: <http://trac.webkit.org/changeset/110999>