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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Apr 7 07:00:00 PDT 2011


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





--- Comment #16 from Sergio Villar Senin <svillar at igalia.com>  2011-04-07 07:00:00 PST ---
(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!!!

-- 
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