| Summary: | [CMAKE] Remove duplicated #ifdef guard in CMakeLists.txt | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||
| Component: | New Bugs | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | dbates, paroga, rakuco, rwlbuis, tonikitoo, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 96346 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Gyuyoung Kim
2012-07-17 20:20:40 PDT
Created attachment 152909 [details]
Patch
CC'ing Patrick Created attachment 152927 [details]
Patch
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? (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 CC'ing dbate as well. (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) (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. (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 Created attachment 152973 [details]
Patch
(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. Patrick, could you give informal review ? (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. (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. Comment on attachment 152973 [details]
Patch
This is pretty much a rubber stamp, but it looks reasonable. Please watch this when it lands :).
Created attachment 153164 [details]
Patch
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 ?
Comment on attachment 153164 [details] Patch Clearing flags on attachment: 153164 Committed r123068: <http://trac.webkit.org/changeset/123068> All reviewed patches have been landed. Closing bug. |