Bug 44069

Summary: [GTK] Clean up WebCore/platform/graphics/gtk/ImageGtk.cpp
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, gns, webkit.review.bot, xan.lopez
Priority: P3 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Cleanup patch
none
Patch with build fixes gns: review+

Description Martin Robinson 2010-08-16 13:07:53 PDT
There are a good number of style violations in ImageGtk.cpp as well as some general badness.
Comment 1 Martin Robinson 2010-08-16 13:42:04 PDT
Created attachment 64516 [details]
Cleanup patch
Comment 2 WebKit Review Bot 2010-08-16 15:14:54 PDT
Attachment 64516 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3756311
Comment 3 Martin Robinson 2010-08-16 16:08:15 PDT
Created attachment 64530 [details]
Patch with build fixes
Comment 4 Gustavo Noronha (kov) 2010-08-16 16:31:56 PDT
Comment on attachment 64530 [details]
Patch with build fixes

WebCore/platform/graphics/gtk/ImageGtk.cpp:58
 +      if (!GetModuleFileName(hmodule, (CHAR *) dataDirectory, sizeof(retval) - 10))
Should this (CHAR *) be turned into a static_cast<CHAR*>(dataDirectory) for greater style guide followage?

WebCore/platform/graphics/gtk/ImageGtk.cpp:141
 +          GOwnPtr<gchar> glibFileName(g_build_filename(getWebKitDataDirectory(), "webkit-1.0", "images", imageName.get(), NULL));
Data directories should follow the "soname" of the library. We have done it in InspectoClientGtk:

http://trac.webkit.org/browser/trunk/WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp#L90

Can you then also please change WebCore/GNUmakefile.am to add 'gtk' after 'webkit' here? You'll notice the inspector one is already correct, but we missed images =(:

http://trac.webkit.org/browser/trunk/WebCore/GNUmakefile.am#L4575

As we discussed on IRC, it would be great to move the getWebKitDataDirectory function to, say, FileSystemGtk, and then use it in InspectorClient as well, so it starts actually working on Windows heh. Let's follow up on that later!
Comment 5 Martin Robinson 2010-08-17 12:00:35 PDT
Committed r65529: <http://trac.webkit.org/changeset/65529>
Comment 6 WebKit Review Bot 2010-08-17 12:46:01 PDT
http://trac.webkit.org/changeset/65529 might have broken GTK Linux 64-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/65529
http://trac.webkit.org/changeset/65530