Bug 64923 – [EFL] Defines header files will be publicly installed explicitly.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kangil Han]

Removes 'ewk_util.h' from public header list because it has been determined to internal purpose.

[Kangil Han]

A method of defining open header list has been changed to an explicit manner. In addition, 'ewk/ewk_util.h' was deleted because API in this file has been determined to internal purposes.

[Kangil Han]

Created attachment 101553 [details]
Patch file

I am attaching a patch file. Please review my patch.

[Gyuyoung Kim]

Comment on attachment 101553 [details]
Patch file

LGTM.

[Raphael Kubo da Costa (:rakuco)]

I don't see much need for changing how we specify which headers are installed -- either way, we still need to specify quite a few headers manually, but the code gets bigger if we explicitly list which files should be installed in opposition to listing which files should not be installed.

> Source/WebKit/efl/ChangeLog:3
> +        [EFL] A method of defining open header list has been changed

This is *very* vague for a one-line description of the change.

> Source/WebKit/efl/ChangeLog:6
> +        A method of defining open header list has been changed to an explicit manner.

You should explain why this has been done.

[Kangil Han]

Created attachment 101715 [details]
Patch file

I think that addressing header list clearly one by one would be better to understand even though it needs little more lines in source code.
Please review my patch.

[Raphael Kubo da Costa (:rakuco)]

I'm still not convinced by the arguments you have presented. What if you move the private headers to a private/ directory inside ewk? I'd be OK with this and I think it should be explicitly enough for you too.

> Source/WebKit/efl/ChangeLog:3
> +        [EFL] A method of defining open header list has been changed to an explicit manner.

That's not much in line with what I had in mind when I asked you to be less vague here. If one starts reading the ChangeLog file, this will probably make very little sense. What's an "open header", for a start? You should say something like "[EFL] Explicitly define which headers will be publicly installed".

> Source/WebKit/efl/ChangeLog:7
> +        different packages to clearly understand which header files contain publid API.

publid -> public

This would only help if people actually resorted to looking at the build system to check that (which means people working on webkit-efl itself, not people using it in other applications/packages), so this does not look very convincing.

> Source/WebKit/efl/ChangeLog:9
> +        modify cmake file by this change.

OK, this is a more compelling argument, as people will probably add internal headers more frequently than public ones. Still, neither of kind of addition should be frequent enough to justify such change.

> Source/WebKit/efl/ChangeLog:12
> +        because API in this file has been determined to internal purposes.

You could just say "Do not install ewk_util.h anymore, as it only has internal API". Plus, you need to adjust EWebKit.h accordingly.

[Kangil Han]

Created attachment 101721 [details]
Patch file

Please review my patch file.

[Raphael Kubo da Costa (:rakuco)]

I'd like to hear your answers to some suggestions and questions I raised in my previous comment, namely moving private headers to private/ and why I don't think the current patch will "improve the readability which files will be publicly installed". And EWebKit.h is still wrong.

[Kangil Han]

Dear Raphael,

Here are my answers.

1. Creating 'private' directory and moving files contain internal API seem good to me. However, I think this suggestion is improving my patch not a substitution. One my small concern about your idea is that there are no ports using 'private' directory. For example, GTK port locates '*private*' header files together with others.

2. I believe 'white-list' way is better than 'black-list'.
At initial time, I only concentrated on removing 'ewk_util.h' file from installation.
But, some of my colleagues suggested me to change a way to define open header list from 'black-list' to 'white-list' while internal code-review. I agreed with them because I thought 'white-list' way would reduce mistakes from developers especially newly joined people. This is because newly created header file without addressing in cmake won't affect system. As you mentioned, internal header files would be created more frequently and as a result, developers would not be irritated from inserting one line in cmake file. This is also an additional benefit from this patch.

3. You are right. 'EWebKit.h' is the file that developers, who want to implement stuff by using EWebkit, should know about. However, Including header correctly to 'EWebKit.h' should be precondition of my patch. Rather, my patch will remove internally used header files from installation. As you said, developers would read 'EWebKit.h' in most cases but I think installing internal header files to the system is not nice.

I hope this helps your understanding.

[Ryuan Choi]

Comment on attachment 101721 [details]
Patch file

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

> Source/WebKit/efl/CMakeListsEfl.txt:218
> +LIST(APPEND EWebKit_HEADERS "${CMAKE_CURRENT_SOURCE_DIR}/efl/ewk/EWebKit.h")

Anyway, It's not good for me.

you can use SET for these headers.
SET(EWebKit_Headers
    EWebKit.h
    ...
    ewk_window_features.h
)

[Kangil Han]

Okay Ryuan, I will fix it after I finish discuss with Raphael.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #9)
> 1. Creating 'private' directory and moving files contain internal API seem good to me. However, I think this suggestion is improving my patch not a substitution. One my small concern about your idea is that there are no ports using 'private' directory. For example, GTK port locates '*private*' header files together with others.

The layout is very port-dependent and should be up to each port's maintainers, so I don't think there should be any problem with doing so.

> 2. I believe 'white-list' way is better than 'black-list'.
> At initial time, I only concentrated on removing 'ewk_util.h' file from installation.
> But, some of my colleagues suggested me to change a way to define open header list from 'black-list' to 'white-list' while internal code-review. I agreed with them because I thought 'white-list' way would reduce mistakes from developers especially newly joined people. This is because newly created header file without addressing in cmake won't affect system. As you mentioned, internal header files would be created more frequently and as a result, developers would not be irritated from inserting one line in cmake file. This is also an additional benefit from this patch.

This is a valid point, I think we can go this way instead of arguing forever.
 
> 3. You are right. 'EWebKit.h' is the file that developers, who want to implement stuff by using EWebkit, should know about. However, Including header correctly to 'EWebKit.h' should be precondition of my patch. Rather, my patch will remove internally used header files from installation. As you said, developers would read 'EWebKit.h' in most cases but I think installing internal header files to the system is not nice.

That's not what I meant. What I'm saying is that EWebKit.h is still #including ewk_utils.h, which is not going to be installed anymore.

[Kangil Han]

(In reply to comment #12)
> The layout is very port-dependent and should be up to each port's maintainers, so I don't think there should be any problem with doing so.

If so, I don't have any reason to disagree with you. Let's hear other developers opinion.
 
> That's not what I meant. What I'm saying is that EWebKit.h is still #including ewk_utils.h, which is not going to be installed anymore.

Sorry but I couldn't find 'ewk_util.h' in 'EWebKit.h'.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #13)
> > That's not what I meant. What I'm saying is that EWebKit.h is still #including ewk_utils.h, which is not going to be installed anymore.
> 
> Sorry but I couldn't find 'ewk_util.h' in 'EWebKit.h'.

Oops, I was not looking at my upstream checkout, ewk_util.h is not included by EWebKit.h indeed.

So r+ from my side after you follow Ryuan's suggestion.

[Kangil Han]

Created attachment 102106 [details]
Patch file

Please review attached patch file.
Please also leave opinion about creating 'private' directory for private source files in EFL port.

[Ryuan Choi]

(In reply to comment #15)
> Created an attachment (id=102106) [details]
> Patch file
> 
> Please review attached patch file.
> Please also leave opinion about creating 'private' directory for private source files in EFL port.

LGTM.

[Raphael Kubo da Costa (:rakuco)]

> Please review attached patch file.

LGTM.

> Please also leave opinion about creating 'private' directory for private source files in EFL port.

I'm all for it.

[Kangil Han]

For creating 'private' directory for internally used source code, let's discuss in another bug because we can separate this idea from current patch. Dear Raphael, please create new bug. I am going to review this patch at this state.

[WebKit Review Bot]

Comment on attachment 102106 [details]
Patch file

Clearing flags on attachment: 102106

Committed r91973: <http://trac.webkit.org/changeset/91973>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.