Build breaks when TOUCH_EVENTS is disabled. Source/WebKit2/Shared/NativeWebTouchEvent.h:41:50: error: expected class-name before ‘{’ token
Created attachment 180913 [details] Patch
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?
(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.
(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?
(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.
Created attachment 181787 [details] Patch
Created attachment 181790 [details] Patch
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.
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.
Created attachment 181876 [details] Patch
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.
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.
(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.
(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.
Created attachment 182066 [details] Patch
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.
Created attachment 182068 [details] Patch
> >> 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 :)
Comment on attachment 182068 [details] Patch Clearing flags on attachment: 182068 Committed r139287: <http://trac.webkit.org/changeset/139287>
All reviewed patches have been landed. Closing bug.
(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.
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.