Bug 156850 – Fixed compilation with !ENABLE(SVG_FONTS).

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Konstantin Tokarev]

Fixed compilation with !ENABLE(SVG_FONTS).

[Konstantin Tokarev]

Created attachment 276921 [details]
Patch

[Csaba Osztrogonác]

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?

[Michael Catanzaro]

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

[Csaba Osztrogonác]

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

[Konstantin Tokarev]

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

[Csaba Osztrogonác]

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.

[Konstantin Tokarev]

Created attachment 276941 [details]
Patch

[Michael Catanzaro]

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.

[WebKit Commit Bot]

Comment on attachment 276941 [details]
Patch

Clearing flags on attachment: 276941

Committed r199830: <http://trac.webkit.org/changeset/199830>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.