Bug 105846 – [WK2][EFL] Add guard around NativeWebTouchEvent

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit2

[Seokju Kwon]

Build breaks when TOUCH_EVENTS is disabled.
Source/WebKit2/Shared/NativeWebTouchEvent.h:41:50: error: expected class-name before ‘{’ token

[Seokju Kwon]

Created attachment 180913 [details]
Patch

[Benjamin Poulain]

Comment on attachment 180913 [details]
Patch

With a few exceptions, the feature #ifdef should be on the #include side, not in the header.

Why is this case different?

[Seokju Kwon]

(In reply to comment #2)
> (From update of attachment 180913 [details])
> With a few exceptions, the feature #ifdef should be on the #include side, not in the header.
> 
> Why is this case different?

NativeWebTouchEvent was derived from WebTouchEvent. But it is around ENABLE(TOUCH_EVENTS) in header file (WebEvent.h).
It seems that build breaks when TOUCH_EVENTS is disabled.

[Benjamin Poulain]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 180913 [details] [details])
> > With a few exceptions, the feature #ifdef should be on the #include side, not in the header.
> > 
> > Why is this case different?
> 
> NativeWebTouchEvent was derived from WebTouchEvent. But it is around ENABLE(TOUCH_EVENTS) in header file (WebEvent.h).
> It seems that build breaks when TOUCH_EVENTS is disabled.

I understand that. But why #ifdef the header instead of in the code where the header is included?

[Seokju Kwon]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 180913 [details] [details] [details])
> > > With a few exceptions, the feature #ifdef should be on the #include side, not in the header.
> > > 
> > > Why is this case different?
> > 
> > NativeWebTouchEvent was derived from WebTouchEvent. But it is around ENABLE(TOUCH_EVENTS) in header file (WebEvent.h).
> > It seems that build breaks when TOUCH_EVENTS is disabled.
> 
> I understand that. But why #ifdef the header instead of in the code where the header is included?

Sorry, I was confused. I will upload a new patch.

[Seokju Kwon]

Created attachment 181787 [details]
Patch

[Seokju Kwon]

Created attachment 181790 [details]
Patch

[Benjamin Poulain]

Comment on attachment 181790 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        Add TOUCH_EVENTS guard.

Please add a sentence to say this fixes EFL build without TOUCH_EVENTS.

> Source/WebKit2/Shared/qt/NativeWebTouchEventQt.cpp:29
> +#if ENABLE(TOUCH_EVENTS)
>  #include "NativeWebTouchEvent.h"
> +#endif

This is not correct for Qt.

For performance reason, Qt exclude such files through the project file. In this case, NativeWebTouchEventQt.cpp is excluded from Target.pri if TOUCH_EVENTS is not defined.

[Gyuyoung Kim]

Comment on attachment 181790 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        Add guard around NativeWebTouchEvent

It would be good if you add [WK2][EFL][QT] prefix.

[Seokju Kwon]

Created attachment 181876 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 181876 [details]
Patch

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

> Source/WebKit2/Shared/efl/NativeWebTouchEventEfl.cpp:27
> +#if ENABLE(TOUCH_EVENTS)

NativeWebTouchEventEfl.cpp already uses this guard. I think this build break occurs because NativeWebTouchEvent.h doesn't use the guard. I think it would be good to add this guard to NativeWebTouchEvent.h.

[Benjamin Poulain]

Comment on attachment 181876 [details]
Patch

(In reply to comment #11)
> (From update of attachment 181876 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181876&action=review
> 
> > Source/WebKit2/Shared/efl/NativeWebTouchEventEfl.cpp:27
> > +#if ENABLE(TOUCH_EVENTS)
> 
> NativeWebTouchEventEfl.cpp already uses this guard. 

You can just move that guard up under the #include of config.h.

> I think this build break occurs because NativeWebTouchEvent.h doesn't use the guard. I think it would be good to add this guard to NativeWebTouchEvent.h.

See my previous comments.

[Gyuyoung Kim]

(In reply to comment #12)
> (From update of attachment 181876 [details])
> (In reply to comment #11)
> > (From update of attachment 181876 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=181876&action=review
> > 
> > > Source/WebKit2/Shared/efl/NativeWebTouchEventEfl.cpp:27
> > > +#if ENABLE(TOUCH_EVENTS)
> > 
> > NativeWebTouchEventEfl.cpp already uses this guard. 
> 
> You can just move that guard up under the #include of config.h.

Seokju, please do that. Then, I will set cq+.

> > I think this build break occurs because NativeWebTouchEvent.h doesn't use the guard. I think it would be good to add this guard to NativeWebTouchEvent.h.
> 
> See my previous comments.

>> With a few exceptions, the feature #ifdef should be on the #include side, not in the header.

I have no strong opinion on this, but it seems to me #ifdef has been using in header in many places. For example, Modules sub-directories. But, I don't wanna object your opinion.

[Seokju Kwon]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 181876 [details] [details])
> > (In reply to comment #11)
> > > (From update of attachment 181876 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=181876&action=review
> > > 
> > > > Source/WebKit2/Shared/efl/NativeWebTouchEventEfl.cpp:27
> > > > +#if ENABLE(TOUCH_EVENTS)
> > > 
> > > NativeWebTouchEventEfl.cpp already uses this guard. 
> > 
> > You can just move that guard up under the #include of config.h.
> 
> Seokju, please do that. Then, I will set cq+.
> 
> > > I think this build break occurs because NativeWebTouchEvent.h doesn't use the guard. I think it would be good to add this guard to NativeWebTouchEvent.h.
> > 
> > See my previous comments.
> 
> >> With a few exceptions, the feature #ifdef should be on the #include side, not in the header.
> 
> I have no strong opinion on this, but it seems to me #ifdef has been using in header in many places. For example, Modules sub-directories. But, I don't wanna object your opinion.

Ok, Thanks.

[Seokju Kwon]

Created attachment 182066 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 182066 [details]
Patch

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

> Source/WebKit2/Shared/efl/NativeWebTouchEventEfl.cpp:26
>  #include "config.h"

I prefer to add a new line below config.h, if possible.

[Seokju Kwon]

Created attachment 182068 [details]
Patch

[Benjamin Poulain]

> >> With a few exceptions, the feature #ifdef should be on the #include side, not in the header.
> 
> I have no strong opinion on this, but it seems to me #ifdef has been using in header in many places. For example, Modules sub-directories. But, I don't wanna object your opinion.

You are right, I am thinking of touch events header in particular. If I am not mistaken, those use the style I mentioned above (right?).

For me the difference ties into a feature in development, and optional features like touch events.

(In reply to comment #16)
> > Source/WebKit2/Shared/efl/NativeWebTouchEventEfl.cpp:26
> >  #include "config.h"
> 
> I prefer to add a new line below config.h, if possible.

+1 :)

[WebKit Review Bot]

Comment on attachment 182068 [details]
Patch

Clearing flags on attachment: 182068

Committed r139287: <http://trac.webkit.org/changeset/139287>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Gyuyoung Kim]

(In reply to comment #18)
> > >> With a few exceptions, the feature #ifdef should be on the #include side, not in the header.
> > 
> > I have no strong opinion on this, but it seems to me #ifdef has been using in header in many places. For example, Modules sub-directories. But, I don't wanna object your opinion.
> 
> You are right, I am thinking of touch events header in particular. If I am not mistaken, those use the style I mentioned above (right?).
> 
> For me the difference ties into a feature in development, and optional features like touch events.

It looks there is no rule for this. However, I prefer to tie .h and .cpp together with #ifdef guard personally if there is #ifdef guard for those files. I think it will prevent unexpected build break on other ports. But, I'm not sure if this should become one of coding style.

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.