Bug 94670

Created attachment 159870 [details] Patch

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

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

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

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

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

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

Created attachment 159874 [details] Patch

Comment on attachment 159874 [details] Patch LGTM.

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.

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

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

Created attachment 162033 [details] Patch

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 ?

Created attachment 162178 [details] Patch

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

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

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

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

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

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

Created attachment 162614 [details] Patch

Created attachment 162615 [details] Patch

(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 on attachment 162615 [details] Patch Clearing flags on attachment: 162615 Committed r127823: <http://trac.webkit.org/changeset/127823>

All reviewed patches have been landed. Closing bug.