Bug 39494

Summary: Change filenameFromString to return CString
Product: WebKit Reporter: Kwang Yul Seo <skyul>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, joybro201, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
eric: review-
Revised patch none

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.