Bug 90107

Created attachment 150895 [details] Patch

Comment on attachment 150895 [details] Patch Attachment 150895 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13138909

Comment on attachment 150895 [details] Patch Attachment 150895 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13137957

Created attachment 150941 [details] Patch

Created attachment 151033 [details] Patch

Comment on attachment 151033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151033&action=review > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:51 > + ewk_view_theme_set(m_view, DATA_DIR"/default.edj"); Shouldn't this be done in ewk_view_base_add in ewk_view.cpp? - A view is not fully functional without a default theme - that's why I don't think it should be required to do this explicitly after WkViewCreate.

(In reply to comment #6) > (From update of attachment 151033 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151033&action=review > > > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:51 > > + ewk_view_theme_set(m_view, DATA_DIR"/default.edj"); > > Shouldn't this be done in ewk_view_base_add in ewk_view.cpp? - A view is not fully functional without a default theme - that's why I don't think it should be required to do this explicitly after WkViewCreate. Make sense, we can make default.edc as a default.

Created attachment 151039 [details] Take Dominik's suggestion

LGTM & works as advertised.

Could one of you kindly r(s)+ this one? Thanks.

Comment on attachment 151039 [details] Take Dominik's suggestion While thinking little bit more, I realize that it looks wrong. For MiniBrowser, WebKitTestRunner, it will be fine. However applications using installed webkit2 is not useful. If we don't provide a way to find installed theme path, previous one looks more reasonable.

(In reply to comment #11) > (From update of attachment 151039 [details]) > While thinking little bit more, I realize that it looks wrong. > > For MiniBrowser, WebKitTestRunner, it will be fine. > However applications using installed webkit2 is not useful. > > If we don't provide a way to find installed theme path, previous one looks more reasonable. Can you explain what you think is wrong? I think the patch is alright since we just set a reasonable default for the default theme (which works for MiniBrowser and WTR) - still, an embedding application can override the theme setting using the ewk_view_theme_set again if the embedder finds a different theme path more suitable. I'd say, let's land this - since it's an important part to improve WTR pass rate.

(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 151039 [details] [details]) > > While thinking little bit more, I realize that it looks wrong. > > > > For MiniBrowser, WebKitTestRunner, it will be fine. > > However applications using installed webkit2 is not useful. > > > > If we don't provide a way to find installed theme path, previous one looks more reasonable. > > Can you explain what you think is wrong? I think the patch is alright since we just set a reasonable default for the default theme (which works for MiniBrowser and WTR) - still, an embedding application can override the theme setting using the ewk_view_theme_set again if the embedder finds a different theme path more suitable. I'd say, let's land this - since it's an important part to improve WTR pass rate. I agree with you about default theme concept, but my patch is wrong. This patch set the compiled path as default theme path. For example, applications can get package publisher's directory such as {webkit path}/WebKitBuild/Release/WebKit/efl/DefaultTheme/default.edj when application calls ewk_view_theme_get without overriding default theme. Of course, this default theme will not work for applications. So, the theme path should not be a compiled path. It should be a path which will be installed. And it means that test programs such as MiniBrowser and WebKitTestRunner should override theme path to compiled path because they can run without installation.

Created attachment 151335 [details] Patch

Comment on attachment 151335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151335&action=review Looks good to me overall with the variable naming nit + I'd like to clarify whether the default would work on a regular EFL installation. > Source/WebKit2/PlatformEfl.cmake:150 > +ADD_DEFINITIONS(-DDEFAULT_THEME_PATH=\"${CMAKE_INSTALL_PREFIX}/${DATA_INSTALL_DIR}/themes\") Looks like a good default. So this would map to /usr/share/themes on a regular system? Is this where desktop EFL would store themes? Can "default.edj" be found there on a regular installation? > Tools/MiniBrowser/efl/CMakeLists.txt:58 > +ADD_DEFINITIONS(-DDATA_DIR=\"${THEME_BINARY_DIR}\") I see that DATA_DIR is used in the other cases, but I don't think it's a good name, can we change it here, and for EWebLauncher and DRT (DumpRenderTree/efl/CMakeLists.txt & EWebLauncher/CMakeLists.txt) to THEME_DIR? > Tools/MiniBrowser/efl/main.c:166 > + ewk_view_theme_set(app->browser, DATA_DIR"/default.edj"); Ditto -> THEME_DIR? > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:51 > + ewk_view_theme_set(m_view, DATA_DIR"/default.edj"); Ditto, THEME_DIR.

Created attachment 152619 [details] Patch

Comment on attachment 152619 [details] Patch Attachment 152619 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13261108

Created attachment 152622 [details] Patch

(In reply to comment #15) > (From update of attachment 151335 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151335&action=review > > Looks good to me overall with the variable naming nit + I'd like to clarify whether the default would work on a regular EFL installation. > > > Source/WebKit2/PlatformEfl.cmake:150 > > +ADD_DEFINITIONS(-DDEFAULT_THEME_PATH=\"${CMAKE_INSTALL_PREFIX}/${DATA_INSTALL_DIR}/themes\") > > Looks like a good default. So this would map to /usr/share/themes on a regular system? Is this where desktop EFL would store themes? Can "default.edj" be found there on a regular installation? Thank you for your comment. CMAKE_INSTALL_PREFIX can be changed by user, but it may be /usr/local in linux system as a default. And DATA_INSTALL_DIR is defined share/${WebKit_LIBRARY_NAME}-${PROJECT_VERSION_MAJOR} in cmake/OptionsEfl.cmake. So, DEFAULT_THEME_PATH will be /usr/local/share/ewebkit-0 if we didn't change. > > > Tools/MiniBrowser/efl/CMakeLists.txt:58 > > +ADD_DEFINITIONS(-DDATA_DIR=\"${THEME_BINARY_DIR}\") > > I see that DATA_DIR is used in the other cases, but I don't think it's a good name, can we change it here, and for EWebLauncher and DRT (DumpRenderTree/efl/CMakeLists.txt & EWebLauncher/CMakeLists.txt) to THEME_DIR? > I fixed like you mentioned. But, I think that DRT and EWebLuancher are not related to this bug. I will create new bug for it. > > Tools/MiniBrowser/efl/main.c:166 > > + ewk_view_theme_set(app->browser, DATA_DIR"/default.edj"); > > Ditto -> THEME_DIR? Done. > > > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:51 > > + ewk_view_theme_set(m_view, DATA_DIR"/default.edj"); > > Ditto, THEME_DIR. Done.

Created attachment 152629 [details] fix mistake

Comment on attachment 152629 [details] fix mistake LGTM

Comment on attachment 152629 [details] fix mistake rs=me

Comment on attachment 152629 [details] fix mistake Clearing flags on attachment: 152629 Committed r122799: <http://trac.webkit.org/changeset/122799>

All reviewed patches have been landed. Closing bug.