Bug 69450 - [EFL] DRT: Create icon database path in LayoutTestController::setIconDatabaseEnabled.
Summary: [EFL] DRT: Create icon database path in LayoutTestController::setIconDatabase...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raphael Kubo da Costa (:rakuco)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-05 11:59 PDT by Raphael Kubo da Costa (:rakuco)
Modified: 2011-10-06 12:28 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.35 KB, patch)
2011-10-05 12:00 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Follow rniwa's suggestions (2.31 KB, patch)
2011-10-06 11:44 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Er, no need to ask for review again. List rniwa as the reviewer (2.31 KB, patch)
2011-10-06 11:52 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Follow rniwa's new suggestion (2.22 KB, patch)
2011-10-06 11:54 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raphael Kubo da Costa (:rakuco) 2011-10-05 11:59:37 PDT
[EFL] DRT: Create icon database path in LayoutTestController::setIconDatabaseEnabled.
Comment 1 Raphael Kubo da Costa (:rakuco) 2011-10-05 12:00:49 PDT
Created attachment 109833 [details]
Patch
Comment 2 Ryosuke Niwa 2011-10-06 11:37:51 PDT
Comment on attachment 109833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109833&action=review

It'll be nice if the logic to obtain the temp dir. was extracted as a function.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:403
> +    char* tempDir = getenv("TMPDIR");

You should use const char* instead.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:409
> +    else {
> +        tempDir = getenv("TEMP");
> +        if (tempDir)

You should define the variable inside if as in:
else if (tempDir = getenv("TEMP"))
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-10-06 11:44:46 PDT
Created attachment 109986 [details]
Follow rniwa's suggestions
Comment 4 Ryosuke Niwa 2011-10-06 11:47:17 PDT
Comment on attachment 109986 [details]
Follow rniwa's suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=109986&action=review

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:408
> +    else {
> +        if (tempDir = getenv("TEMP"))

Please use else if instead of nesting like this.
Comment 5 Raphael Kubo da Costa (:rakuco) 2011-10-06 11:52:02 PDT
Created attachment 109988 [details]
Er, no need to ask for review again. List rniwa as the reviewer
Comment 6 Ryosuke Niwa 2011-10-06 11:53:05 PDT
Comment on attachment 109988 [details]
Er, no need to ask for review again. List rniwa as the reviewer

View in context: https://bugs.webkit.org/attachment.cgi?id=109988&action=review

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:408
> +    else {
> +        if (tempDir = getenv("TEMP"))

Please fix this before landing it.
Comment 7 Raphael Kubo da Costa (:rakuco) 2011-10-06 11:54:57 PDT
Created attachment 109990 [details]
Follow rniwa's new suggestion
Comment 8 WebKit Review Bot 2011-10-06 12:27:58 PDT
Comment on attachment 109990 [details]
Follow rniwa's new suggestion

Clearing flags on attachment: 109990

Committed r96848: <http://trac.webkit.org/changeset/96848>
Comment 9 WebKit Review Bot 2011-10-06 12:28:03 PDT
All reviewed patches have been landed.  Closing bug.