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

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #92245|review?                     |review-
               Flag|                            |




--- Comment #22 from Martin Robinson <mrobinson at webkit.org>  2011-05-04 09:33:20 PST ---
(From update of attachment 92245)
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)
{

}

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