Bug 90107 – [EFL][WK2] Add APIs to support theme.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Ryuan Choi]

WebKit/Efl can set or change native theme using edj.
To support theme in WebKit2/Efl, ewk_view_theme_{get|set} should be added.

[Ryuan Choi]

Created attachment 150895 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 150895 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 150895 [details]
Patch

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

[Ryuan Choi]

Created attachment 150941 [details]
Patch

[Ryuan Choi]

Created attachment 151033 [details]
Patch

[Dominik Röttsches (drott)]

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.

[Ryuan Choi]

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

[Ryuan Choi]

Created attachment 151039 [details]
Take Dominik's suggestion

[Dominik Röttsches (drott)]

LGTM & works as advertised.

[Dominik Röttsches (drott)]

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

[Ryuan Choi]

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.

[Dominik Röttsches (drott)]

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

[Ryuan Choi]

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

[Ryuan Choi]

Created attachment 151335 [details]
Patch

[Dominik Röttsches (drott)]

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.

[Ryuan Choi]

Created attachment 152619 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 152619 [details]
Patch

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

[Ryuan Choi]

Created attachment 152622 [details]
Patch

[Ryuan Choi]

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

[Ryuan Choi]

Created attachment 152629 [details]
fix mistake

[Gyuyoung Kim]

Comment on attachment 152629 [details]
fix mistake

LGTM

[Hajime Morrita]

Comment on attachment 152629 [details]
fix mistake

rs=me

[WebKit Review Bot]

Comment on attachment 152629 [details]
fix mistake

Clearing flags on attachment: 152629

Committed r122799: <http://trac.webkit.org/changeset/122799>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.