Bug 94670 - [EFL][WK2] Add API unit tests for ewk_view_theme_set.
Summary: [EFL][WK2] Add API unit tests for ewk_view_theme_set.
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: Ryuan Choi
URL:
Keywords:
Depends on: 94778 95832
Blocks: 90451
  Show dependency treegraph
 
Reported: 2012-08-21 22:59 PDT by Ryuan Choi
Modified: 2012-09-06 21:59 PDT (History)
7 users (show)

See Also:


Attachments
Patch (132.59 KB, patch)
2012-08-21 23:36 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (125.88 KB, patch)
2012-08-22 00:47 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (123.98 KB, patch)
2012-09-04 06:55 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (124.03 KB, patch)
2012-09-05 00:53 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (124.42 KB, patch)
2012-09-06 16:20 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (124.43 KB, patch)
2012-09-06 16:21 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2012-08-21 22:59:28 PDT
ewk_view_theme_set was added without unit tests.

And while making test cases, I found two bugs.

1. ewk_view_theme_set changes theme only one time.
2. document mentioned that theme path can be null to reset to the default theme, but it's not working.
Comment 1 Ryuan Choi 2012-08-21 23:36:38 PDT
Created attachment 159870 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-08-21 23:47:20 PDT
I'd rather have only the minimum required in this patch. For example, the OwnPtr changes should really be done separately, and so should any change not tested by the tests being added.
Comment 3 Chris Dumez 2012-08-21 23:50:45 PDT
Comment on attachment 159870 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:40
> +    void waitUntilLoadFinished();

I don't think we really need this. We usually use waitUntilTitleChangedTo().

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:200
> +    waitUntilLoadFinished();

You don't need waitUntilLoadFinished(), you can use the existing waitUntilTitleChangedTo().

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:204
> +    WKEinaSharedString defaultThemPath = ewk_view_theme_get(webView());

"defaultThemePath"

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:205
> +    ASSERT_EQ(defaultThemPath.isNull(), false);

ASSERT_FALSE(defaultThemePath.isNull());

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:209
> +    waitUntilLoadFinished();

You don't need waitUntilLoadFinished(), you can use the existing waitUntilTitleChangedTo().
Comment 4 Thiago Marcos P. Santos 2012-08-21 23:55:45 PDT
Comment on attachment 159870 [details]
Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.h:-38
> +#include "RenderTheme.h"
> +#include <cairo.h>
> +#include <wtf/OwnPtr.h>
> +
>  #if ENABLE(VIDEO)
>  #include "MediaControlElements.h"
>  #endif
> -#include "RenderTheme.h"
> -
> -#include <cairo.h>

Nit: should have a space after #include "RenderTheme.h" and the rest should be packed together.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:66
> +static const char* defaultThemePath = DEFAULT_THEME_PATH"/default.edj";

For globals you should use array[]. It increases the chances of the compiler to do funny optimizations (although inside functions IMO is fine to use const char*).

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:94
> +void EWK2UnitTestBase::loadUrlSync(const char* url)
> +{
> +    ewk_view_uri_set(m_webView, url);
> +    waitUntilLoadFinished();
> +}

I like this change. Gives more flexibility, thanks.
Comment 5 Gyuyoung Kim 2012-08-21 23:57:08 PDT
Comment on attachment 159870 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:66
> +static const char* defaultThemePath = DEFAULT_THEME_PATH"/default.edj";

Though I'm not sure this is our rule, WK2 EFL port has used "static const char defaultThemePath[]" instead.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:51
> +    return makeString(TEST_RESOURCES_DIR"/", resource).utf8();

IIRC, makeString() function was deprecated.
Comment 6 Thiago Marcos P. Santos 2012-08-22 00:08:27 PDT
Comment on attachment 159870 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:51
>> +    return makeString(TEST_RESOURCES_DIR"/", resource).utf8();
> 
> IIRC, makeString() function was deprecated.

It was deprecated for appending two strings (since + operator fast now). In this case I think it is fine.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:209
>> +    waitUntilLoadFinished();
> 
> You don't need waitUntilLoadFinished(), you can use the existing waitUntilTitleChangedTo().

That's true. I didn't see that he was changing the title. titleChanged might happen after the loadFinished, please change to waitUntilTitleChangedTo().
Comment 7 Ryuan Choi 2012-08-22 00:09:36 PDT
(In reply to comment #2)
> I'd rather have only the minimum required in this patch. For example, the OwnPtr changes should really be done separately, and so should any change not tested by the tests being added.

OK, I will try.

(In reply to comment #3)
> (From update of attachment 159870 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159870&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:40
> > +    void waitUntilLoadFinished();
> 
> I don't think we really need this. We usually use waitUntilTitleChangedTo().
> 

Well, I think that waitUnitilTitleChangedTo() is not good because it will not be finished when tests are failed.


> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:204
> > +    WKEinaSharedString defaultThemPath = ewk_view_theme_get(webView());
> 
> "defaultThemePath"
> 
typo, I will fix it.

> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:205
> > +    ASSERT_EQ(defaultThemPath.isNull(), false);
> 
> ASSERT_FALSE(defaultThemePath.isNull());
> 
Right, I will fix it.


(In reply to comment #4)
> (From update of attachment 159870 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159870&action=review
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.h:-38
> > +#include "RenderTheme.h"
> > +#include <cairo.h>
> > +#include <wtf/OwnPtr.h>
> > +
> >  #if ENABLE(VIDEO)
> >  #include "MediaControlElements.h"
> >  #endif
> > -#include "RenderTheme.h"
> > -
> > -#include <cairo.h>
> 
> Nit: should have a space after #include "RenderTheme.h" and the rest should be packed together.
> 
I am not sure, but I will separate this as kubos' comment.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:66
> > +static const char* defaultThemePath = DEFAULT_THEME_PATH"/default.edj";
> 
> For globals you should use array[]. It increases the chances of the compiler to do funny optimizations (although inside functions IMO is fine to use const char*).
> 
right. I will

(In reply to comment #5)
> (From update of attachment 159870 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159870&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:66
> > +static const char* defaultThemePath = DEFAULT_THEME_PATH"/default.edj";
> 
> Though I'm not sure this is our rule, WK2 EFL port has used "static const char defaultThemePath[]" instead.
> 
right. I will.

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:51
> > +    return makeString(TEST_RESOURCES_DIR"/", resource).utf8();
> 
> IIRC, makeString() function was deprecated.

Ah right, I fogot it.
I will fix.

Thank you for all interests. :)
Comment 8 Ryuan Choi 2012-08-22 00:16:46 PDT
(In reply to comment #6)
> (From update of attachment 159870 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159870&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:51
> >> +    return makeString(TEST_RESOURCES_DIR"/", resource).utf8();
> > 
> > IIRC, makeString() function was deprecated.
> 
> It was deprecated for appending two strings (since + operator fast now). In this case I think it is fine.
> 
> >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:209
> >> +    waitUntilLoadFinished();
> > 
> > You don't need waitUntilLoadFinished(), you can use the existing waitUntilTitleChangedTo().
> 
> That's true. I didn't see that he was changing the title. titleChanged might happen after the loadFinished, please change to waitUntilTitleChangedTo().

In my test case, I think that titleChanged will happen before loadFinished is called because script is executed while contents parsing.
Comment 9 Chris Dumez 2012-08-22 00:22:45 PDT
(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 159870 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=159870&action=review
> > 
> > >> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:51
> > >> +    return makeString(TEST_RESOURCES_DIR"/", resource).utf8();
> > > 
> > > IIRC, makeString() function was deprecated.
> > 
> > It was deprecated for appending two strings (since + operator fast now). In this case I think it is fine.
> > 
> > >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:209
> > >> +    waitUntilLoadFinished();
> > > 
> > > You don't need waitUntilLoadFinished(), you can use the existing waitUntilTitleChangedTo().
> > 
> > That's true. I didn't see that he was changing the title. titleChanged might happen after the loadFinished, please change to waitUntilTitleChangedTo().
> 
> In my test case, I think that titleChanged will happen before loadFinished is called because script is executed while contents parsing.

Either way, it does not matter. What you're really interested in is the title change (because you check it), not the loadFinished. If an existing convenience method fits your needs, I don't see the point of adding a new one.
Comment 10 Thiago Marcos P. Santos 2012-08-22 00:27:36 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > (From update of attachment 159870 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=159870&action=review
> > > 
> > > >> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:51
> > > >> +    return makeString(TEST_RESOURCES_DIR"/", resource).utf8();
> > > > 
> > > > IIRC, makeString() function was deprecated.
> > > 
> > > It was deprecated for appending two strings (since + operator fast now). In this case I think it is fine.
> > > 
> > > >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:209
> > > >> +    waitUntilLoadFinished();
> > > > 
> > > > You don't need waitUntilLoadFinished(), you can use the existing waitUntilTitleChangedTo().
> > > 
> > > That's true. I didn't see that he was changing the title. titleChanged might happen after the loadFinished, please change to waitUntilTitleChangedTo().
> > 
> > In my test case, I think that titleChanged will happen before loadFinished is called because script is executed while contents parsing.
> 
> Either way, it does not matter. What you're really interested in is the title change (because you check it), not the loadFinished. If an existing convenience method fits your needs, I don't see the point of adding a new one.

Agree with Chris. It is always nice to add a new functionality if this new functionality has an use case.

loadFinished is fired when resources are loaded. Is not bind to script execution, the way how this test is written right now, is a candidate to be a flaky test.
Comment 11 Ryuan Choi 2012-08-22 00:40:23 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > (From update of attachment 159870 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=159870&action=review
> > > 
> > > >> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:51
> > > >> +    return makeString(TEST_RESOURCES_DIR"/", resource).utf8();
> > > > 
> > > > IIRC, makeString() function was deprecated.
> > > 
> > > It was deprecated for appending two strings (since + operator fast now). In this case I think it is fine.
> > > 
> > > >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:209
> > > >> +    waitUntilLoadFinished();
> > > > 
> > > > You don't need waitUntilLoadFinished(), you can use the existing waitUntilTitleChangedTo().
> > > 
> > > That's true. I didn't see that he was changing the title. titleChanged might happen after the loadFinished, please change to waitUntilTitleChangedTo().
> > 
> > In my test case, I think that titleChanged will happen before loadFinished is called because script is executed while contents parsing.
> 
> Either way, it does not matter. What you're really interested in is the title change (because you check it), not the loadFinished. If an existing convenience method fits your needs, I don't see the point of adding a new one.

I tried to use it but test_ewk2_view was in infinite loop when test case is failed.

Anyway because ./Tools/Scripts/run-efl-tests is fine and it is not related to this bug, I will change to use it.

I hope that someone fix infinite loop in other bugs.

Thank you.
Comment 12 Ryuan Choi 2012-08-22 00:47:44 PDT
Created attachment 159874 [details]
Patch
Comment 13 Chris Dumez 2012-08-22 00:52:59 PDT
Comment on attachment 159874 [details]
Patch

LGTM.
Comment 14 Raphael Kubo da Costa (:rakuco) 2012-08-22 08:58:17 PDT
Comment on attachment 159874 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Fixed bug that ewk_view_theme_set() changes theme information only one time.

Interesting, so multiple calls to ewk_view_theme_set() had no effect?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1031
> +    if (!path)
> +        path = defaultThemePath;

I wonder if it doesn't make more sense to adjust the documentation (for both WK1 and WK2) instead; for one, the WK1 behavior is still broken (I don't remember if it has ever been different), and I don't see why the default theme should be different from any other theme. One needs to call ewk_view_theme_set() when constructing the view anyway, and always has access to the default theme's path.
Comment 15 Ryuan Choi 2012-08-22 16:31:14 PDT
(In reply to comment #14)
> (From update of attachment 159874 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159874&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Fixed bug that ewk_view_theme_set() changes theme information only one time.
> 
> Interesting, so multiple calls to ewk_view_theme_set() had no effect?
> 

Yes. Once m_edje was initialized, multiple calls had no effect.

Although this bugs lives for a long time, we did not catch this because we did not change theme path before.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1031
> > +    if (!path)
> > +        path = defaultThemePath;
> 
> I wonder if it doesn't make more sense to adjust the documentation (for both WK1 and WK2) instead; for one, the WK1 behavior is still broken (I don't remember if it has ever been different), and I don't see why the default theme should be different from any other theme. One needs to call ewk_view_theme_set() when constructing the view anyway, and always has access to the default theme's path.

IMO, I want to fix the webkit1/efl behavior in new bug.
In webkit1/efl, applications must call ewk_view_theme_set() when creating webview, because webkit1/efl does not have default theme concept(although document mentioned!!),
However, they also don't have a way to know where defaut.edj is.

And if default theme was provided, I think that we need a way to reset to default after user changed.
Comment 16 Raphael Kubo da Costa (:rakuco) 2012-08-22 17:14:22 PDT
(In reply to comment #15)
> In webkit1/efl, applications must call ewk_view_theme_set() when creating webview, because webkit1/efl does not have default theme concept(although document mentioned!!),
> However, they also don't have a way to know where defaut.edj is.
> 
> And if default theme was provided, I think that we need a way to reset to default after user changed.

My point is that EWK itself shouldn't need to differentiate between the default theme and the others: it only needs to know it is changing the theme. The user can then pass whatever path she wants; that's what we usually do in other functions that take paths and what's usually expected.
Comment 17 Gyuyoung Kim 2012-08-23 05:29:47 PDT
Comment on attachment 159874 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:51
> +    return makeString(TEST_RESOURCES_DIR"/", resource).utf8();

As discussed in IRC, makeString is deprecated. So, please just use + operator to make new string.
Comment 18 Ryuan Choi 2012-09-04 06:55:22 PDT
Created attachment 162033 [details]
Patch
Comment 19 Gyuyoung Kim 2012-09-04 22:16:56 PDT
Comment on attachment 162033 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:254
> +    waitUntilTitleChangedTo("34");

I think it would good to say why we need to wait until title 34.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:258
> +    waitUntilTitleChangedTo("303");

Don't you need to check if this test is passed ? For example,  ASSERT_XXX ?
Comment 20 Ryuan Choi 2012-09-05 00:53:37 PDT
Created attachment 162178 [details]
Patch
Comment 21 Ryuan Choi 2012-09-05 00:58:03 PDT
(In reply to comment #19)
> (From update of attachment 162033 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162033&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:254
> > +    waitUntilTitleChangedTo("34");
> 
> I think it would good to say why we need to wait until title 34.
> 
OK, I updated comment and changed html to use clientWidth which does not have border(default border is 4 px)

> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:258
> > +    waitUntilTitleChangedTo("303");
> 
> Don't you need to check if this test is passed ? For example,  ASSERT_XXX ?

If I remember correctly, tmpsantos recommend not to check after finished waitUntilTitleChangedTo.

If this test is failed, timeout would occur.
Comment 22 Gyuyoung Kim 2012-09-05 01:24:36 PDT
Comment on attachment 162178 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:54
> +    builder.appendLiteral(TEST_RESOURCES_DIR"/");

IMHO, this would be nice,

builder.appendLiteral(TEST_RESOURCES_DIR);
builder.append('/');

See also : http://trac.webkit.org/wiki/EfficientStrings
Comment 23 Gyuyoung Kim 2012-09-05 01:29:56 PDT
(In reply to comment #22)
> (From update of attachment 162178 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162178&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:54
> > +    builder.appendLiteral(TEST_RESOURCES_DIR"/");
> 
> IMHO, this would be nice,
> 
> builder.appendLiteral(TEST_RESOURCES_DIR);
> builder.append('/');
> 
> See also : http://trac.webkit.org/wiki/EfficientStrings

This one is better for me.

builder.append(TEST_RESOURCES_DIR);
builder.append('/');
Comment 24 Ryuan Choi 2012-09-05 02:55:33 PDT
(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 162178 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=162178&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:54
> > > +    builder.appendLiteral(TEST_RESOURCES_DIR"/");
> > 
> > IMHO, this would be nice,
> > 
> > builder.appendLiteral(TEST_RESOURCES_DIR);
> > builder.append('/');
> > 
> > See also : http://trac.webkit.org/wiki/EfficientStrings
> 

For the efficiency, I think that current looks better.
In compile time, TEST_RESOURCES_DIR and "/" will be concatenated.
Do you want better readability?

> This one is better for me.
> 
> builder.append(TEST_RESOURCES_DIR);
> builder.append('/');

I am not sure, but if we need to split them, I understood that below is better.
builder.appendLiteral(TEST_RESOURCES_DIR);
builder.append('/');

In my understanding, document mentioned that append(`char`) is better than appendLiteral("X")
Comment 25 Gyuyoung Kim 2012-09-05 03:21:19 PDT
(In reply to comment #24)
> builder.appendLiteral(TEST_RESOURCES_DIR);
> builder.append('/');
> 
> In my understanding, document mentioned that append(`char`) is better than appendLiteral("X")

It looks appendLiteral is used by literal(" ") itself. But, I'm not sure which one is best as well. 

CC'ing benjamin, could you take a look this ?
Comment 26 Kenneth Rohde Christiansen 2012-09-05 03:29:03 PDT
Comment on attachment 162178 [details]
Patch

Oh I fixed this as well as well as made it load fuzzily. Please check bug 95832. Maybe we can land my patch instead and then your test? :-)
Comment 27 Benjamin Poulain 2012-09-05 10:26:30 PDT
(In reply to comment #25)
> (In reply to comment #24)
> > builder.appendLiteral(TEST_RESOURCES_DIR);
> > builder.append('/');
> > 
> > In my understanding, document mentioned that append(`char`) is better than appendLiteral("X")
> 
> It looks appendLiteral is used by literal(" ") itself. But, I'm not sure which one is best as well. 

As long as TEST_RESOURCES_DIR is a const char[] or a #define for a constant literal, you can use appendLiteral.

Given the same conditions, TEST_RESOURCES_DIR"/" would also be correct. But I think you'd better add a space between the two string constants otherwise it is a little hard to read:
    appendLiteral(TEST_RESOURCES_DIR "/")
Comment 28 Ryuan Choi 2012-09-05 15:02:50 PDT
(In reply to comment #27)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > builder.appendLiteral(TEST_RESOURCES_DIR);
> > > builder.append('/');
> > > 
> > > In my understanding, document mentioned that append(`char`) is better than appendLiteral("X")
> > 
> > It looks appendLiteral is used by literal(" ") itself. But, I'm not sure which one is best as well. 
> 
> As long as TEST_RESOURCES_DIR is a const char[] or a #define for a constant literal, you can use appendLiteral.
> 
> Given the same conditions, TEST_RESOURCES_DIR"/" would also be correct. But I think you'd better add a space between the two string constants otherwise it is a little hard to read:
>     appendLiteral(TEST_RESOURCES_DIR "/")

Right, I will add space for readability.

Thank you.
Comment 29 Ryuan Choi 2012-09-06 16:20:41 PDT
Created attachment 162614 [details]
Patch
Comment 30 Ryuan Choi 2012-09-06 16:21:48 PDT
Created attachment 162615 [details]
Patch
Comment 31 Ryuan Choi 2012-09-06 16:23:46 PDT
(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #25)
> > > (In reply to comment #24)
> > > > builder.appendLiteral(TEST_RESOURCES_DIR);
> > > > builder.append('/');
> > > > 
> > > > In my understanding, document mentioned that append(`char`) is better than appendLiteral("X")
> > > 
> > > It looks appendLiteral is used by literal(" ") itself. But, I'm not sure which one is best as well. 
> > 
> > As long as TEST_RESOURCES_DIR is a const char[] or a #define for a constant literal, you can use appendLiteral.
> > 
> > Given the same conditions, TEST_RESOURCES_DIR"/" would also be correct. But I think you'd better add a space between the two string constants otherwise it is a little hard to read:
> >     appendLiteral(TEST_RESOURCES_DIR "/")
> 
> Right, I will add space for readability.
> 
> Thank you.

Fixed.

In addition, I added more test cases to take kenneth's suggestion.

Thank you, kenneth.
Comment 32 WebKit Review Bot 2012-09-06 21:59:49 PDT
Comment on attachment 162615 [details]
Patch

Clearing flags on attachment: 162615

Committed r127823: <http://trac.webkit.org/changeset/127823>
Comment 33 WebKit Review Bot 2012-09-06 21:59:56 PDT
All reviewed patches have been landed.  Closing bug.