Bug 199469

Summary: HyphenationLibHyphen: Include GLib-related headers only if ENABLE(DEVELOPER_MODE) is true
Product: WebKit Reporter: Konstantin Tokarev <annulen>
Component: WebCore Misc.Assignee: Konstantin Tokarev <annulen>
Status: RESOLVED FIXED    
Severity: Normal CC: mcatanzaro, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch mcatanzaro: review+

Konstantin Tokarev
Reported 2019-07-03 15:27:31 PDT
GLib is used only inside ENABLE(DEVELOPER_MODE) block
Attachments
Patch (1.53 KB, patch)
2019-07-03 15:28 PDT, Konstantin Tokarev 		no flags
DetailsFormatted DiffDiff
Patch (2.12 KB, patch)
2019-07-03 18:56 PDT, Konstantin Tokarev 		no flags
DetailsFormatted DiffDiff
Patch (2.05 KB, patch)
2019-07-04 06:37 PDT, Konstantin Tokarev 		mcatanzaro: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Konstantin Tokarev
Comment 1 2019-07-03 15:28:46 PDT
Created attachment 373421 [details] Patch
Michael Catanzaro
Comment 2 2019-07-03 17:44:49 PDT
Comment on attachment 373421 [details] Patch How about #if USE(GLIB)?
Konstantin Tokarev
Comment 3 2019-07-03 18:56:36 PDT
Created attachment 373447 [details] Patch
Michael Catanzaro
Comment 4 2019-07-04 06:21:26 PDT
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
Comment 5 2019-07-04 06:24:36 PDT
(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 6 2019-07-04 06:26:07 PDT
Comment on attachment 373447 [details] Patch So it is....
Michael Catanzaro
Comment 7 2019-07-04 06:27:25 PDT
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
Comment 8 2019-07-04 06:37:18 PDT
Created attachment 373463 [details] Patch
Michael Catanzaro
Comment 9 2019-07-04 06:49:38 PDT
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
Comment 10 2019-07-04 06:50:23 PDT
Oops
Konstantin Tokarev
Comment 11 2019-07-04 06:54:53 PDT
Committed r247141: <https://trac.webkit.org/changeset/247141>
Radar WebKit Bug Importer
Comment 12 2019-07-04 06:55:16 PDT
<rdar://problem/52645215>
Note You need to log in before you can comment on or make changes to this bug.