Bug 44069 - [GTK] Clean up WebCore/platform/graphics/gtk/ImageGtk.cpp
Summary: [GTK] Clean up WebCore/platform/graphics/gtk/ImageGtk.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2010-08-16 13:07 PDT by Martin Robinson
Modified: 2010-08-17 12:46 PDT (History)
5 users (show)

See Also:


Attachments
Cleanup patch (5.17 KB, patch)
2010-08-16 13:42 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch with build fixes (5.45 KB, patch)
2010-08-16 16:08 PDT, Martin Robinson
gustavo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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