Bug 94670 – [EFL][WK2] Add API unit tests for ewk_view_theme_set.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

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.

[Ryuan Choi]

Created attachment 159870 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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.

[Chris Dumez]

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().

[Thiago Marcos P. Santos]

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.

[Gyuyoung Kim]

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.

[Thiago Marcos P. Santos]

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().

[Ryuan Choi]

(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. :)

[Ryuan Choi]

(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.

[Chris Dumez]

(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.

[Thiago Marcos P. Santos]

(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.

[Ryuan Choi]

(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.

[Ryuan Choi]

Created attachment 159874 [details]
Patch

[Chris Dumez]

Comment on attachment 159874 [details]
Patch

LGTM.

[Raphael Kubo da Costa (:rakuco)]

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.

[Ryuan Choi]

(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.

[Raphael Kubo da Costa (:rakuco)]

(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.

[Gyuyoung Kim]

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.

[Ryuan Choi]

Created attachment 162033 [details]
Patch

[Gyuyoung Kim]

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 ?

[Ryuan Choi]

Created attachment 162178 [details]
Patch

[Ryuan Choi]

(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.

[Gyuyoung Kim]

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

[Gyuyoung Kim]

(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('/');

[Ryuan Choi]

(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")

[Gyuyoung Kim]

(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 ?

[Kenneth Rohde Christiansen]

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? :-)

[Benjamin Poulain]

(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 "/")

[Ryuan Choi]

(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.

[Ryuan Choi]

Created attachment 162614 [details]
Patch

[Ryuan Choi]

Created attachment 162615 [details]
Patch

[Ryuan Choi]

(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.

[WebKit Review Bot]

Comment on attachment 162615 [details]
Patch

Clearing flags on attachment: 162615

Committed r127823: <http://trac.webkit.org/changeset/127823>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.