Bug 199469 – HyphenationLibHyphen: Include GLib-related headers only if ENABLE(DEVELOPER_MODE) is true

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

[Konstantin Tokarev]

GLib is used only inside ENABLE(DEVELOPER_MODE) block

[Konstantin Tokarev]

Created attachment 373421 [details]
Patch

[Michael Catanzaro]

Comment on attachment 373421 [details]
Patch

How about #if USE(GLIB)?

[Konstantin Tokarev]

Created attachment 373447 [details]
Patch

[Michael Catanzaro]

Comment on attachment 373447 [details]
Patch

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

> Source/WebCore/platform/text/hyphen/HyphenationLibHyphen.cpp:43
> +#if ENABLE(DEVELOPER_MODE) && PLATFORM(GTK)

Well this is even worse, now they won't work for WPE layout tests, right?

If you're using USE(GLIB) then what's the problem with including these headers?

[Konstantin Tokarev]

(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 373447 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373447&action=review
> 
> > Source/WebCore/platform/text/hyphen/HyphenationLibHyphen.cpp:43
> > +#if ENABLE(DEVELOPER_MODE) && PLATFORM(GTK)
> 
> Well this is even worse, now they won't work for WPE layout tests, right?

No. Code below (it's really sad that we don't have an easy way to expand context in patch view) is explicitly using PLATFORM(GTK). WPE should not be affected at all by this change

>If you're using USE(GLIB) then what's the problem with including these headers?

I'm just trying to match ifdefs on headers with ifdefs on code which uses them

[Michael Catanzaro]

Comment on attachment 373447 [details]
Patch

So it is....

[Michael Catanzaro]

Maybe just use PLATFORM(GTK) guards and not ENABLE(DEVELOPER_MODE). My worry is that someone will in the future use these headers outside developer mode, and nobody will notice the build is broken until release time. In general, headers should only be behind ifdefs if really required, to avoid gratuitous build breakage down the road.

[Konstantin Tokarev]

Created attachment 373463 [details]
Patch

[Michael Catanzaro]

Comment on attachment 373463 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        HyphenationLibHyphen: Include GLib-related headers only if ENABLE(DEVELOPER_MODE) is true

Change the title, of course

[Konstantin Tokarev]

Oops

[Konstantin Tokarev]

Committed r247141: <https://trac.webkit.org/changeset/247141>

[Radar WebKit Bug Importer]

<rdar://problem/52645215>