Bug 156850

Created attachment 276921 [details] Patch

Comment on attachment 276921 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276921&action=review > Source/WebCore/ChangeLog:15 > + (WebCore::FontCascade::drawGlyphBuffer): Deleted. Please remove "Deleted." It must be a prepare-changelog bug. > Source/WebCore/CMakeLists.txt:2864 > +if (ENABLE_SVG_FONTS) > + list(APPEND WebCore_SOURCES > + svg/SVGToOTFFontConversion.cpp > + ) > +endif () We prefer guarding source files to guarding build sytem when it is possible. Myles, is it OK to add ENABLE(SVG_FONTS) guard to SVGToOTFFontConversion.cpp?

(In reply to comment #2) > We prefer guarding source files to guarding build sytem when it is possible. It's the opposite of what we've been doing recently. Maybe we should discuss this on webkit-dev?

(In reply to comment #3) > (In reply to comment #2) > > We prefer guarding source files to guarding build sytem when it is possible. > > It's the opposite of what we've been doing recently. Maybe we should discuss > this on webkit-dev? What do you mean? I haven't seen any patch removing if guards from source file. Source files are almost always guarded properly, I can't see a good reason to break this good practice now. I think most of the build system guard is redundant.

(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > We prefer guarding source files to guarding build sytem when it is possible. > > > > It's the opposite of what we've been doing recently. Maybe we should discuss > > this on webkit-dev? > > What do you mean? I haven't seen any patch removing if guards from source > file. https://bugs.webkit.org/show_bug.cgi?id=155944 Though in case of TEXTURE_MAPPER there is no direct condition in build system, instead choice is made by inclusion (or not inclusion) of TextureMapper.cmake in PlatformXXX.cmake. > Source files are almost always guarded properly, I can't see a good reason > to break this good practice now. I think most of the build system guard > is redundant. What is good about guards in .cpp files? They add no information in most cases, and also slightly increase build time, not only because of having to compile empty files, but also extra time is needed to generate large ninja manifest, also ninja has to process more edges.

It's easier to maintain if guards in sources which is platform independent. It's easier to add a new cpp to a simple source list than a super complex one for a person who don't work on cmake port at all. (Most of the WebKit developers.) If we make cmake build system more and more complex, I think Apple will break our build more often. I don't think if building emtpy files could increase the build time significantly.

Created attachment 276941 [details] Patch

Comment on attachment 276941 [details] Patch I guess I agree with Ossy, but we should still discuss this on webkit-dev because not everyone is currently on board with this.

Comment on attachment 276941 [details] Patch Clearing flags on attachment: 276941 Committed r199830: <http://trac.webkit.org/changeset/199830>

All reviewed patches have been landed. Closing bug.