Bug 188081 – [INTL] Remove INTL sub-feature compile flags

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Andy VanWagoner]

[INTL] Remove INTL sub-feature compile flags

[Andy VanWagoner]

Created attachment 345877 [details]
Patch

[Andy VanWagoner]

There was an ENABLE_JAVASCRIPT_I18N_API flag I couldn't find actually used anywhere, and the ENABLE_INTL_NUMBER_FORMAT_TO_PARTS and ENABLE_INTL_PLURAL_RULES only served to set the default for their respective runtime flags. Neither were used substantially to change compile behavior, so this removes them, and the runtime flags will be used exclusively.

Hopefully this makes it easier for GTK to enable these features.
https://bugs.webkit.org/show_bug.cgi?id=185714

[Andy VanWagoner]

Created attachment 345880 [details]
Patch

[Michael Catanzaro]

Comment on attachment 345880 [details]
Patch

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

Yay, fewer build flags! This looks good, except perhaps for one bit I have questions about:

> Source/JavaScriptCore/runtime/Options.h:115
> -#if ENABLE(INTL_NUMBER_FORMAT_TO_PARTS) && (!PLATFORM(COCOA) || ENABLE(EXPERIMENTAL_FEATURES))
> +#if (!PLATFORM(COCOA) || ENABLE(EXPERIMENTAL_FEATURES))

So the intention is for these features to be enabled for Apple only if experimental features are enabled, but enabled on other ports always? Isn't that pretty weird? What's the reasoning behind this? Are the features expected to be more stable on non-Apple ports?

Surely this is enabling the feature for WPE/GTK right here?

[Andy VanWagoner]

(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 345880 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345880&action=review
> 
> Yay, fewer build flags! This looks good, except perhaps for one bit I have
> questions about:
> 
> > Source/JavaScriptCore/runtime/Options.h:115
> > -#if ENABLE(INTL_NUMBER_FORMAT_TO_PARTS) && (!PLATFORM(COCOA) || ENABLE(EXPERIMENTAL_FEATURES))
> > +#if (!PLATFORM(COCOA) || ENABLE(EXPERIMENTAL_FEATURES))
> 
> So the intention is for these features to be enabled for Apple only if
> experimental features are enabled, but enabled on other ports always? Isn't
> that pretty weird? What's the reasoning behind this? Are the features
> expected to be more stable on non-Apple ports?
> 
> Surely this is enabling the feature for WPE/GTK right here?

I could remove the constexpr here, and set it to default false below. Otherwise, as is, it is enabling it by default for all non-Apple ports. Is there another file where ports can enable runtime flags?

I didn't add those conditions before, so I'm not positive they are correct without the INTL flags. That's why I haven't cq=? yet.

[Michael Catanzaro]

(In reply to Andy VanWagoner from comment #5)
> I could remove the constexpr here, and set it to default false below.
> Otherwise, as is, it is enabling it by default for all non-Apple ports. Is
> there another file where ports can enable runtime flags?

No, this is the right place for JavaScriptCore options.

It was changed in bug #186319.

> I didn't add those conditions before, so I'm not positive they are correct
> without the INTL flags. That's why I haven't cq=? yet.

I think it's not worse than before, but it does enable the feature for non-Cocoa ports that use ENABLE(INTL), so that should be mentioned in the ChangeLog... but, looking through bug #186319, I think we should just change the guard to:

#if ENABLE(EXPERIMENTAL_FEATURES)

and then close bug #185714. Then, whenever the ENABLE(EXPERIMENTAL(FEATURES) guard is removed, it will become available automatically for all ports with newer ICU.

[Andy VanWagoner]

Created attachment 345891 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 345891 [details]
Patch

Clearing flags on attachment: 345891

Committed r234293: <https://trac.webkit.org/changeset/234293>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42650262>