Bug 188081 - [INTL] Remove INTL sub-feature compile flags
Summary: [INTL] Remove INTL sub-feature compile flags
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy VanWagoner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-26 15:00 PDT by Andy VanWagoner
Modified: 2018-07-26 22:07 PDT (History)
4 users (show)

See Also:


Attachments
Patch (54.47 KB, patch)
2018-07-26 15:07 PDT, Andy VanWagoner
no flags Details | Formatted Diff | Diff
Patch (55.37 KB, patch)
2018-07-26 15:19 PDT, Andy VanWagoner
no flags Details | Formatted Diff | Diff
Patch (55.20 KB, patch)
2018-07-26 17:58 PDT, Andy VanWagoner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy VanWagoner 2018-07-26 15:00:21 PDT
[INTL] Remove INTL sub-feature compile flags
Comment 1 Andy VanWagoner 2018-07-26 15:07:57 PDT
Created attachment 345877 [details]
Patch
Comment 2 Andy VanWagoner 2018-07-26 15:14:04 PDT
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
Comment 3 Andy VanWagoner 2018-07-26 15:19:24 PDT
Created attachment 345880 [details]
Patch
Comment 4 Michael Catanzaro 2018-07-26 16:30:56 PDT
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?
Comment 5 Andy VanWagoner 2018-07-26 16:45:26 PDT
(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.
Comment 6 Michael Catanzaro 2018-07-26 17:40:31 PDT
(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.
Comment 7 Andy VanWagoner 2018-07-26 17:58:11 PDT
Created attachment 345891 [details]
Patch
Comment 8 WebKit Commit Bot 2018-07-26 22:06:10 PDT
Comment on attachment 345891 [details]
Patch

Clearing flags on attachment: 345891

Committed r234293: <https://trac.webkit.org/changeset/234293>
Comment 9 WebKit Commit Bot 2018-07-26 22:06:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2018-07-26 22:07:18 PDT
<rdar://problem/42650262>