Bug 105905 – [EFL][CMAKE] Remove duplicated conditionals

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

Some files have used unneeded macros though same macros are already wrapped around files. It messes up cmake file.

[Gyuyoung Kim]

Created attachment 181007 [details]
Patch

[Laszlo Gombos]

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)"

[Gyuyoung Kim]

Created attachment 181010 [details]
Patch

[Gyuyoung Kim]

Created attachment 181011 [details]
Patch

[Gyuyoung Kim]

(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.

[Laszlo Gombos]

Comment on attachment 181011 [details]
Patch

lgtm.

[Gyuyoung Kim]

Created attachment 181102 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 181102 [details]
Patch for landing

Clearing flags on attachment: 181102

Committed r138681: <http://trac.webkit.org/changeset/138681>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Laszlo Gombos]

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.

[Gyuyoung Kim]

(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.

[Hugo Parente Lima]

I think this can be closed now that the regression was fixed, right?