Bug 91575 – [CMAKE] Remove duplicated #ifdef guard in CMakeLists.txt

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

CMake files have used ENABLE_XXX macro. However, almost source and idl file are already protected by #ifdef. So, we don't need to guard in CMake files again.

[Gyuyoung Kim]

Created attachment 152909 [details]
Patch

[Gyuyoung Kim]

CC'ing Patrick

[Gyuyoung Kim]

Created attachment 152927 [details]
Patch

[Patrick R. Gansterer]

Comment on attachment 152927 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        So, we don't need to guard them in CMake files again.

Guarding them in CMake helps in not compiling a bunch of unneeded files. This way you increase the compile time. So I'm not the biggest fan of this change, but it's ok.

> Source/WebCore/CMakeLists.txt:566
> +    # FIXME: The BlackBerry port doesn't support generating DOM bindings from the SVG IDL files.
> +    # For now, we explicitly demarcate the SVG IDL files so that the BlackBerry port can skip them
> +    # during DOM binding generation. See <https://bugs.webkit.org/show_bug.cgi?id=72773>.

How do you skip them now?

[Gyuyoung Kim]

(In reply to comment #4)
> (From update of attachment 152927 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152927&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        So, we don't need to guard them in CMake files again.
> 
> Guarding them in CMake helps in not compiling a bunch of unneeded files. This way you increase the compile time. So I'm not the biggest fan of this change, but it's ok.

Yes, those source files already have #ifdef guard by themselves. So, I thought CMake doesn't need to protect them again. In my humble opinion, if we continue to use existing method, CMake file can be littered by ENABLE_XXX.

> > Source/WebCore/CMakeLists.txt:566
> > +    # FIXME: The BlackBerry port doesn't support generating DOM bindings from the SVG IDL files.
> > +    # For now, we explicitly demarcate the SVG IDL files so that the BlackBerry port can skip them
> > +    # during DOM binding generation. See <https://bugs.webkit.org/show_bug.cgi?id=72773>.
> 
> How do you skip them now?

Should I fix it in this patch ?  This patch is also to move ENABLE(SVG) to main .idl file list because .idl files can be used when SVG is enabled as below,

 interface [
        Conditional=SVG

[Gyuyoung Kim]

CC'ing dbate as well.

[Patrick R. Gansterer]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 152927 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=152927&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        So, we don't need to guard them in CMake files again.
> > 
> > Guarding them in CMake helps in not compiling a bunch of unneeded files. This way you increase the compile time. So I'm not the biggest fan of this change, but it's ok.
> 
> Yes, those source files already have #ifdef guard by themselves. So, I thought CMake doesn't need to protect them again. In my humble opinion, if we continue to use existing method, CMake file can be littered by ENABLE_XXX.

Both solutions are ok for me, so decide yourself. ;-)

> > > Source/WebCore/CMakeLists.txt:566
> > > +    # FIXME: The BlackBerry port doesn't support generating DOM bindings from the SVG IDL files.
> > > +    # For now, we explicitly demarcate the SVG IDL files so that the BlackBerry port can skip them
> > > +    # during DOM binding generation. See <https://bugs.webkit.org/show_bug.cgi?id=72773>.
> > 
> > How do you skip them now?
> 
> Should I fix it in this patch ?  This patch is also to move ENABLE(SVG) to main .idl file list because .idl files can be used when SVG is enabled as below,
> 
>  interface [
>         Conditional=SVG

Only the IDL generation for SVG is disabled! AFAIK Blackberry builds with ENABLE(SVG), but only skips IDL generation for it. (Since the CPP IDL generator can not handle SVG IDL files correctly ATM)

[Gyuyoung Kim]

(In reply to comment #7)

> Only the IDL generation for SVG is disabled! AFAIK Blackberry builds with ENABLE(SVG), but only skips IDL generation for it. (Since the CPP IDL generator can not handle SVG IDL files correctly ATM)

I would decide how to handle SVG idl files after getting comments from blackberry port guys. CC'ing Antonio and Rob as well.

[Patrick R. Gansterer]

(In reply to comment #8)
> (In reply to comment #7)
> 
> > Only the IDL generation for SVG is disabled! AFAIK Blackberry builds with ENABLE(SVG), but only skips IDL generation for it. (Since the CPP IDL generator can not handle SVG IDL files correctly ATM)
> 
> I would decide how to handle SVG idl files after getting comments from blackberry port guys. CC'ing Antonio and Rob as well.

The easiest way is to split it again in WebCore_IDL and WebCore_SVG_IDL files. So PlatformBlackberry.cmake can handle them again in a special way. See http://trac.webkit.org/browser/trunk/Source/WebCore/PlatformBlackBerry.cmake?rev=122250#L315

[Gyuyoung Kim]

Created attachment 152973 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #9)
> The easiest way is to split it again in WebCore_IDL and WebCore_SVG_IDL files. So PlatformBlackberry.cmake can handle them again in a special way. See http://trac.webkit.org/browser/trunk/Source/WebCore/PlatformBlackBerry.cmake?rev=122250#L315

Thank you, I also think WebCore_SVG_IDL_FILES is good solution. Patch is updated according to your comment.

[Gyuyoung Kim]

Patrick, could you give informal review ?

[Patrick R. Gansterer]

(In reply to comment #12)
> Patrick, could you give informal review ?

I didn't check if the copy&paste of the file names is correct, since the compiler can do this better. ;-)
Otherwise I'm still not a big fan (since the increase of compile time, which might can be changed somehow else), but looks ok to me now.

[Gyuyoung Kim]

(In reply to comment #13)
> (In reply to comment #12)
> > Patrick, could you give informal review ?
> 
> I didn't check if the copy&paste of the file names is correct, since the compiler can do this better. ;-)
> Otherwise I'm still not a big fan (since the increase of compile time, which might can be changed somehow else), but looks ok to me now.

Yes, this patch should be updated after getting r+. I will not make any problems by this patch. Thank you.

[Dirk Pranke]

Comment on attachment 152973 [details]
Patch

This is pretty much a rubber stamp, but it looks reasonable. Please watch this when it lands :).

[Gyuyoung Kim]

Created attachment 153164 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 153164 [details]
Patch

According to my build time measurement with / without this patch, there is no build time difference. So, I land this patch.


Antonio and Rob Buis,

Could you please check if blackberry port can be built after landing this patch ?

[WebKit Review Bot]

Comment on attachment 153164 [details]
Patch

Clearing flags on attachment: 153164

Committed r123068: <http://trac.webkit.org/changeset/123068>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.