Some files have used unneeded macros though same macros are already wrapped around files. It messes up cmake file.
Created attachment 181007 [details] Patch
Comment on attachment 181007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181007&action=review Overall looks good to me - I like the idea of sharing build rules with other ports and moving logic from the port specific build system into common C/C++ files make sense to me. > Source/WebCore/ChangeLog:9 > + Some files have used unneeded macros though same macros are already wrapped around files. It > + messes up cmake file. More precise description would be better - e.g. "Remove conditionals from CMake build system, where the files being built are already wrapped with the conditionals." > Source/WebCore/PlatformEfl.cmake:-305 > - It seems to me that we can than remove the CMake variables as well - e.g. no need for the following line "cmake/OptionsEfl.cmake: set(WTF_USE_GLX 1)"
Created attachment 181010 [details] Patch
Created attachment 181011 [details] Patch
(In reply to comment #2) > (From update of attachment 181007 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181007&action=review > > Overall looks good to me - I like the idea of sharing build rules with other ports and moving logic from the port specific build system into common C/C++ files make sense to me. It looks GNUMakefile also tries not to use conditionals for .cpp / .h files. But, I'm not sure if I can guide this rule to all C/C++ files now. It looks each port has own policy. > > Source/WebCore/ChangeLog:9 > > + Some files have used unneeded macros though same macros are already wrapped around files. It > > + messes up cmake file. > > More precise description would be better - e.g. "Remove conditionals from CMake build system, where the files being built are already wrapped with the conditionals." > > > Source/WebCore/PlatformEfl.cmake:-305 > > - > > It seems to me that we can than remove the CMake variables as well - e.g. no need for the following line "cmake/OptionsEfl.cmake: set(WTF_USE_GLX 1)" Fix all. Thanks.
Comment on attachment 181011 [details] Patch lgtm.
Created attachment 181102 [details] Patch for landing
Comment on attachment 181102 [details] Patch for landing Clearing flags on attachment: 181102 Committed r138681: <http://trac.webkit.org/changeset/138681>
All reviewed patches have been landed. Closing bug.
I just noticed that some of the files under accessibility/atk/ are not guarded with HAVE(ACCESSIBILITY). This is a regression for builds where ACCESSIBILITY is not enabled. REOPEN the bug to fix the regression.
(In reply to comment #10) > I just noticed that some of the files under accessibility/atk/ are not guarded with HAVE(ACCESSIBILITY). This is a regression for builds where ACCESSIBILITY is not enabled. REOPEN the bug to fix the regression. Ooops, ok, I'm gonna prepare a patch to fix this regression. But, I prefer to use HAVE(ACCESSIBILITY) guard inside file.
I think this can be closed now that the regression was fixed, right?