[Webkit-unassigned] [Bug 56200] [GTK] WebKitIconDatabase doesn't keep icons cached

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Apr 6 12:59:46 PDT 2011


--- Comment #15 from Christian Dywan <christian at twotoasts.de>  2011-04-06 12:59:47 PST ---
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.


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?

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

More information about the webkit-unassigned mailing list