Bug 147246 – Remove runtime flags for symbols

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Remove runtime flags for symbols

[Yusuke Suzuki]

Created attachment 257415 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 257415 [details]
Patch

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

Added comments

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:-422
> -        putDirectWithoutTransition(vm, vm.propertyNames->Symbol, symbolConstructor, DontEnum);

This is done in FOR_EACH_SIMPLE_BUILTIN_TYPE_WITH_CONSTRUCTOR.

> Source/JavaScriptCore/runtime/JSGlobalObject.h:100
> +    macro(Symbol, symbol, symbolObject, SymbolObject, Symbol) \

Move Symbols from FOR_EACH_EXPERIMENTAL_BUILTIN_TYPE_WITH_CONSTRUCTOR to FOR_EACH_SIMPLE_BUILTIN_TYPE_WITH_CONSTRUCTOR.

[Yusuke Suzuki]

Comment on attachment 257415 [details]
Patch

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

Added comments

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:-422
> -        putDirectWithoutTransition(vm, vm.propertyNames->Symbol, symbolConstructor, DontEnum);

This is done in FOR_EACH_SIMPLE_BUILTIN_TYPE_WITH_CONSTRUCTOR.

> Source/JavaScriptCore/runtime/JSGlobalObject.h:100
> +    macro(Symbol, symbol, symbolObject, SymbolObject, Symbol) \

Move Symbols from FOR_EACH_EXPERIMENTAL_BUILTIN_TYPE_WITH_CONSTRUCTOR to FOR_EACH_SIMPLE_BUILTIN_TYPE_WITH_CONSTRUCTOR.

[Alex Christensen]

Comment on attachment 257415 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:55
>  enum WKJavaScriptRuntimeFlags {
> -    kWKJavaScriptRuntimeFlagsSymbolDisabled = 1 << 0,
>      kWKJavaScriptRuntimeFlagsAllEnabled = 0
>  };
>  typedef unsigned WKJavaScriptRuntimeFlagSet;

I think you should get rid of all of these here and elsewhere.

[Yusuke Suzuki]

Comment on attachment 257415 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:55
>>  typedef unsigned WKJavaScriptRuntimeFlagSet;
> 
> I think you should get rid of all of these here and elsewhere.

Is dropping the runtime flag system preferable?
If we drop this enum, we need to drop the `- (void)_setJavaScriptRuntimeFlags:(_WKJavaScriptRuntimeFlags)javaScriptRuntimeFlags` etc.
It means that we drop the JSC runtime flags system.

[Yusuke Suzuki]

Comment on attachment 257415 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:55
>>  typedef unsigned WKJavaScriptRuntimeFlagSet;
> 
> I think you should get rid of all of these here and elsewhere.

Is dropping the runtime flag system preferable?
If we drop this enum, we need to drop the `- (void)_setJavaScriptRuntimeFlags:(_WKJavaScriptRuntimeFlags)javaScriptRuntimeFlags` etc.
It means that we drop the JSC runtime flags system.

[Yusuke Suzuki]

Oops, I clicked the post button twice...

[Alex Christensen]

I am of the opinion that JSC should not have runtime flags. I think it will lead to strange bugs and bad practices. I also don't work on JSC enough for my opinion to count here.

[Filip Pizlo]

(In reply to comment #8)
> I am of the opinion that JSC should not have runtime flags. I think it will
> lead to strange bugs and bad practices. I also don't work on JSC enough for
> my opinion to count here.

We already use environment-variable-based options to control many things in JSC.  I believe that runtime flags are a refinement of this.

The way JSC is structured, it's no more expensive to use run-time checks than static checks for most features.  Therefore, we prefer run-time checks whenever it's possible.

[Alex Christensen]

Comment on attachment 257415 [details]
Patch

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

True, fpizlo, but I think it's silly to leave a bunch of enums around with no values that mean anything.  I'll let you review this, then.

> Source/JavaScriptCore/runtime/JSGlobalObject.h:-94
>  #define FOR_EACH_EXPERIMENTAL_BUILTIN_TYPE_WITH_CONSTRUCTOR(macro) \
> -    macro(Symbol, symbol, symbolObject, SymbolObject, Symbol) \

This macro should be removed if it doesn't do anything.  It's just used once.

[Yusuke Suzuki]

Comment on attachment 257415 [details]
Patch

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

Thank you. In the meantime, I think leaving these enums is better since it may be leveraged when implementing some features in JSC.
However, adding the enums everywhere when adding the new runtime flag is silly.
The current form is derived from the reason; these enum headers are interface headers and they are independent from the JSC's headers...
We need to look for the way to represent runtime flags more easily, but it should be done in the other patch.

>> Source/JavaScriptCore/runtime/JSGlobalObject.h:-94
>> -    macro(Symbol, symbol, symbolObject, SymbolObject, Symbol) \
> 
> This macro should be removed if it doesn't do anything.  It's just used once.

Thanks, dropped this.

[Yusuke Suzuki]

Created attachment 257421 [details]
Patch

[Alex Christensen]

Comment on attachment 257421 [details]
Patch

All right then.

[WebKit Commit Bot]

Comment on attachment 257421 [details]
Patch

Clearing flags on attachment: 257421

Committed r187356: <http://trac.webkit.org/changeset/187356>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.