[webkit-reviews] review requested: [Bug 32510] [GTK] provide an API to control the IconDatabase : [Attachment 83469] Implement WebKitIconDatabase API #4

[bugzilla-daemon at webkit.org]

Christian Dywan <christian at twotoasts.de> has asked  for review:
Bug 32510: [GTK] provide an API to control the IconDatabase
https://bugs.webkit.org/show_bug.cgi?id=32510

Attachment 83469: Implement WebKitIconDatabase API #4
https://bugs.webkit.org/attachment.cgi?id=83469&action=review

------- Additional Comments from Christian Dywan <christian at twotoasts.de>
(In reply to comment #13)
> (From update of attachment 83333 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=83333&action=review
> 
> Great stuff. Watch the style issues. I'd really like to see some GIR
annotations in all the new documentation.

Oddly enough, I did run check-webkit-style on the patch locally but it didn't
show any issues.

I don't know what kind of annotations you have in mind. The pixbufs are newly
referenced and strings don't need annotations as far as I'm aware, and
webkit_get_icon_database() does have an annotation already.

> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:111
> >  void DumpRenderTreeSupportGtk::setIconDatabaseEnabled(bool enabled)
> >  {
> > -	 WebKit::setIconDatabaseEnabled(enabled);
> > +	 WebKitIconDatabase* database = webkit_get_icon_database();
> > +	 if (enabled) {
> > +	     GOwnPtr<gchar>
iconDatabasePath(g_build_filename(g_get_user_data_dir(), "webkit",
"icondatabase", NULL));
> > +	     webkit_icon_database_set_path(database, iconDatabasePath.get());
> > +	 } else {
> > +	     webkit_icon_database_set_path(database, 0);
> > +	 }
> >  }
> 
> I would remove this method entirely and just do this in DumpRenderTree
itself. DumpRenderTreeSupport really exists just to expose stuff that isn't in
the API yet. When we remove things from here its a big win. : )
> 
> Perhaps DRT should have it's own icon directory too?

Moved the code inside DRT and made it use /tmp/webkit now. This actually seems
like a good idea to avoid unexplicable test failures.

> > Source/WebKit/gtk/webkit/webkitglobals.cpp:253
> > +	 if (!database) {
> > +	     database =
WEBKIT_ICON_DATABASE(g_object_new(WEBKIT_TYPE_ICON_DATABASE, NULL));
> > +	     atexit(closeIconDatabaseOnExit);
> > +	 }
> > +
> 
> It makes sense to move this to where you first open the WebCore icon database
actually. I'd move both the atexit call and closeIconDatabaseOnExit to
webkitwebicondatabase.cpp.

Moved.

> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:213
> > +	 if (database->priv->path.get()) {
> > +	     WebCore::iconDatabase()->setEnabled(false);
> > +	     WebCore::iconDatabase()->close();
> > +	 }
> 
> Why do you do you disable the database here?

Trying to keep the code flow simple. I changed it now.

> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:236
> > + * Returns: the URI for the favicon, or %NULL
> 
> You need to mention whether it's newly allocated or not. It's probably
possible to avoid it using a static CString to just cache it locally in the
method.

I think in this case it is fully expected to get a newly allocated string. In
particular because there is no single string that is always returned, unlike
the icon of a page loaded at a point in time. And this could actually lead to
bugs when loading more than one icon at the same time.

I also added a comment that icons are cleaned up after 4 days and mentioned
that private enable-private-browsing prevents changes of the database on disk.