Bug 199469 - HyphenationLibHyphen: Include GLib-related headers only if ENABLE(DEVELOPER_MODE) is true
Summary: HyphenationLibHyphen: Include GLib-related headers only if ENABLE(DEVELOPER_M...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Konstantin Tokarev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-03 15:27 PDT by Konstantin Tokarev
Modified: 2019-07-04 06:55 PDT (History)
3 users (show)

See Also:

Attachments
Patch (1.53 KB, patch)
2019-07-03 15:28 PDT, Konstantin Tokarev 		no flags Details | Formatted Diff | Diff
Patch (2.12 KB, patch)
2019-07-03 18:56 PDT, Konstantin Tokarev 		no flags Details | Formatted Diff | Diff
Patch (2.05 KB, patch)
2019-07-04 06:37 PDT, Konstantin Tokarev 		mcatanzaro: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Konstantin Tokarev 2019-07-03 15:27:31 PDT 
GLib is used only inside ENABLE(DEVELOPER_MODE) block
Comment 1 Konstantin Tokarev 2019-07-03 15:28:46 PDT 
Created attachment 373421 [details]
Patch
Comment 2 Michael Catanzaro 2019-07-03 17:44:49 PDT 
Comment on attachment 373421 [details]
Patch

How about #if USE(GLIB)?
Comment 3 Konstantin Tokarev 2019-07-03 18:56:36 PDT 
Created attachment 373447 [details]
Patch
Comment 4 Michael Catanzaro 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?
Comment 5 Konstantin Tokarev 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
Comment 6 Michael Catanzaro 2019-07-04 06:26:07 PDT 
Comment on attachment 373447 [details]
Patch

So it is....
Comment 7 Michael Catanzaro 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.
Comment 8 Konstantin Tokarev 2019-07-04 06:37:18 PDT 
Created attachment 373463 [details]
Patch
Comment 9 Michael Catanzaro 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
Comment 10 Konstantin Tokarev 2019-07-04 06:50:23 PDT 
Oops
Comment 11 Konstantin Tokarev 2019-07-04 06:54:53 PDT 
Committed r247141: <https://trac.webkit.org/changeset/247141>
Comment 12 Radar WebKit Bug Importer 2019-07-04 06:55:16 PDT 
<rdar://problem/52645215>