Bug 39494 - Change filenameFromString to return CString
Summary: Change filenameFromString to return CString
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-21 09:49 PDT by Kwang Yul Seo
Modified: 2010-06-04 02:43 PDT (History)
3 users (show)

See Also:


Attachments
Patch (12.44 KB, patch)
2010-05-21 10:05 PDT, Kwang Yul Seo
eric: review-
Details | Formatted Diff | Diff
Revised patch (12.43 KB, patch)
2010-05-23 20:10 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2010-05-21 09:49:35 PDT
filenameFromString returns a newly allocated string and the caller must free the string. GTK and EFL ports use g_free while all others ports use fastFree. This is confusing because the same function behaves differently with respect to ports. Change filenameFromString to return CString.
Comment 1 Kwang Yul Seo 2010-05-21 10:05:57 PDT
Created attachment 56720 [details]
Patch
Comment 2 WebKit Review Bot 2010-05-21 10:07:52 PDT
Attachment 56720 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/gtk/FileSystemGtk.cpp:102:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/gtk/FileSystemGtk.cpp:113:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/gtk/FileSystemGtk.cpp:208:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2010-05-21 11:16:46 PDT
Comment on attachment 56720 [details]
Patch

Good change!

> -char* filenameFromString(const String&);
> +WTF::CString filenameFromString(const String&);

Just CString should be fine, no need for the "WTF::" prefix here.

I can't tell whether filenameFromString is allowed to return the null string or not. If it is allowed to return null, it seems that many call sites are missing code to handle this case and will crash. If it is not allowed to return null, then some call sites have extra unneeded code.
Comment 4 Eric Seidel (no email) 2010-05-21 12:02:56 PDT
Comment on attachment 56720 [details]
Patch

r- per darin's comment above.
Comment 5 Kwang Yul Seo 2010-05-23 20:10:34 PDT
Created attachment 56844 [details]
Revised patch

Remove "WTF:: prefix".

I will file a separate bug for null checks.
Comment 6 WebKit Review Bot 2010-05-23 20:14:22 PDT
Attachment 56844 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/gtk/FileSystemGtk.cpp:102:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/gtk/FileSystemGtk.cpp:113:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/gtk/FileSystemGtk.cpp:208:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Kwang Yul Seo 2010-05-23 20:15:49 PDT
(In reply to comment #3)
> Just CString should be fine, no need for the "WTF::" prefix here.

I tried to be consistent as two other functions use "WTF::" prefix here:

WTF::CString fileSystemRepresentation(const String&);
WTF::CString openTemporaryFile(const char* prefix, PlatformFileHandle&);
Comment 8 Kent Tamura 2010-06-04 02:01:30 PDT
Comment on attachment 56844 [details]
Revised patch

> I tried to be consistent as two other functions use "WTF::" prefix here:

That's reasonable.
Style errors came from the existing code. So, this patch looks OK.
Comment 9 WebKit Commit Bot 2010-06-04 02:42:54 PDT
Comment on attachment 56844 [details]
Revised patch

Clearing flags on attachment: 56844

Committed r60668: <http://trac.webkit.org/changeset/60668>
Comment 10 WebKit Commit Bot 2010-06-04 02:43:00 PDT
All reviewed patches have been landed.  Closing bug.