Bug 90107

Summary: [EFL][WK2] Add APIs to support theme.
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebKit EFLAssignee: Ryuan Choi <ryuan.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, d-r, eric, gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61838, 89140, 90788    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Take Dominik's suggestion
none
Patch
none
Patch
none
Patch
none
fix mistake none

Description Ryuan Choi 2012-06-27 16:06:42 PDT
WebKit/Efl can set or change native theme using edj.
To support theme in WebKit2/Efl, ewk_view_theme_{get|set} should be added.
Comment 1 Ryuan Choi 2012-07-05 01:59:01 PDT
Created attachment 150895 [details]
Patch
Comment 2 Gyuyoung Kim 2012-07-05 03:21:44 PDT
Comment on attachment 150895 [details]
Patch

Attachment 150895 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13138909
Comment 3 Gyuyoung Kim 2012-07-05 03:26:50 PDT
Comment on attachment 150895 [details]
Patch

Attachment 150895 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13137957
Comment 4 Ryuan Choi 2012-07-05 08:09:12 PDT
Created attachment 150941 [details]
Patch
Comment 5 Ryuan Choi 2012-07-06 01:16:40 PDT
Created attachment 151033 [details]
Patch
Comment 6 Dominik Röttsches (drott) 2012-07-06 01:23:20 PDT
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.
Comment 7 Ryuan Choi 2012-07-06 01:32:43 PDT
(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.
Comment 8 Ryuan Choi 2012-07-06 01:58:17 PDT
Created attachment 151039 [details]
Take Dominik's suggestion
Comment 9 Dominik Röttsches (drott) 2012-07-06 06:58:53 PDT
LGTM & works as advertised.
Comment 10 Dominik Röttsches (drott) 2012-07-06 07:02:21 PDT
Could one of you kindly r(s)+ this one? Thanks.
Comment 11 Ryuan Choi 2012-07-06 17:58:48 PDT
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.
Comment 12 Dominik Röttsches (drott) 2012-07-09 07:36:31 PDT
(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.
Comment 13 Ryuan Choi 2012-07-09 15:38:54 PDT
(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.
Comment 14 Ryuan Choi 2012-07-09 15:50:23 PDT
Created attachment 151335 [details]
Patch
Comment 15 Dominik Röttsches (drott) 2012-07-16 07:56:40 PDT
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.
Comment 16 Ryuan Choi 2012-07-16 14:55:22 PDT
Created attachment 152619 [details]
Patch
Comment 17 Gyuyoung Kim 2012-07-16 15:03:56 PDT
Comment on attachment 152619 [details]
Patch

Attachment 152619 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13261108
Comment 18 Ryuan Choi 2012-07-16 15:14:34 PDT
Created attachment 152622 [details]
Patch
Comment 19 Ryuan Choi 2012-07-16 15:17:20 PDT
(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.
Comment 20 Ryuan Choi 2012-07-16 15:52:52 PDT
Created attachment 152629 [details]
fix mistake
Comment 21 Gyuyoung Kim 2012-07-16 17:46:17 PDT
Comment on attachment 152629 [details]
fix mistake

LGTM
Comment 22 Hajime Morrita 2012-07-16 19:02:25 PDT
Comment on attachment 152629 [details]
fix mistake

rs=me
Comment 23 WebKit Review Bot 2012-07-16 20:10:03 PDT
Comment on attachment 152629 [details]
fix mistake

Clearing flags on attachment: 152629

Committed r122799: <http://trac.webkit.org/changeset/122799>
Comment 24 WebKit Review Bot 2012-07-16 20:10:14 PDT
All reviewed patches have been landed.  Closing bug.