Bug 97753 – [EFL] Default.edj should be generated although ENABLE_WEBKIT disabled.

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

[Ryuan Choi]

default.edj is used for only webkit/efl but also webkit2/efl.

However, related files and codes are in WebKit/efl so that we can not use it when disabled webkit1.

First suggestion is to extract scripts and add dependency (simple way)

Patch will be added

[Ryuan Choi]

Created attachment 165924 [details]
Patch

[Ryuan Choi]

Created attachment 165929 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 165929 [details]
Patch

Looks make sense. Kubo, how do you think ?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 165929 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165929&action=review

The bug itself seems valid, however I have a few remarks:

 o Please mention somewhere in the ChangeLog the reason this change is needed, just like you did in the bug description.

 o Keeping DefaultTheme in WebKit/ sounds weird since it is used by WebKit2, but I guess there currently isn't a place for common stuff like this in the tree?

> Source/CMakeLists.txt:5
> +WEBKIT_INCLUDE_CONFIG_FILES_IF_EXISTS()
> +

Would it be possible to move this after the common subdirectories so that it is clear that this can be processed after WTF, JSC and friends are built?

[Ryuan Choi]

(In reply to comment #4)
> (From update of attachment 165929 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165929&action=review
> 
> The bug itself seems valid, however I have a few remarks:
> 
Thank you for review.

>  o Please mention somewhere in the ChangeLog the reason this change is needed, just like you did in the bug description.

OK, I will

> 
>  o Keeping DefaultTheme in WebKit/ sounds weird since it is used by WebKit2, but I guess there currently isn't a place for common stuff like this in the tree?
> 

Yes, so I could not move. 
I think that keeping current is not worst soltion to me until we have good location to move.


> > Source/CMakeLists.txt:5
> > +WEBKIT_INCLUDE_CONFIG_FILES_IF_EXISTS()
> > +
> 
> Would it be possible to move this after the common subdirectories so that it is clear that this can be processed after WTF, JSC and friends are built?

Do you mean that we'd better to move below ADD_SUBDIRECTORY(WEBKIT2) ?

It's not important to me.
I will move where you want.

[Ryuan Choi]

Created attachment 165989 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 165989 [details]
Patch

Looks fine, thanks! Note this may conflict with the upcoming commit that updates the theme dependencies, so you might need to rebase before landing.

[Ryuan Choi]

(In reply to comment #7)
> (From update of attachment 165989 [details])
> Looks fine, thanks! Note this may conflict with the upcoming commit that updates the theme dependencies, so you might need to rebase before landing.

Sure, I already rebased the patch not to conflict with r129752.

[Laszlo Gombos]

Comment on attachment 165989 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165989&action=review

> Source/WebKit/ChangeLog:9
> +        however, it has been generated only when enable_webkit is enabled.

Nit: However

Capitalization in the ChangeLog does not seem to be consistent. Is it ENABLE_WEBKIT or enable_webkit ? Can you correct it before landing ? 

Otherwise looks good to me.

[Ryuan Choi]

Created attachment 167030 [details]
Patch

[Ryuan Choi]

Comment on attachment 167030 [details]
Patch

Thank you.

Fixed Capitalization.

[WebKit Review Bot]

Comment on attachment 167030 [details]
Patch

Clearing flags on attachment: 167030

Committed r130364: <http://trac.webkit.org/changeset/130364>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.