Bug 147246

Summary: Remove runtime flags for symbols
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, fpizlo, ggaren
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Yusuke Suzuki 2015-07-23 18:03:33 PDT
Remove runtime flags for symbols
Comment 1 Yusuke Suzuki 2015-07-23 18:04:14 PDT
Created attachment 257415 [details]
Patch
Comment 2 Yusuke Suzuki 2015-07-23 18:06:07 PDT
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.
Comment 3 Yusuke Suzuki 2015-07-23 18:06:08 PDT
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.
Comment 4 Alex Christensen 2015-07-23 18:07:27 PDT
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.
Comment 5 Yusuke Suzuki 2015-07-23 18:19:29 PDT
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.
Comment 6 Yusuke Suzuki 2015-07-23 18:19:30 PDT
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.
Comment 7 Yusuke Suzuki 2015-07-23 18:19:54 PDT
Oops, I clicked the post button twice...
Comment 8 Alex Christensen 2015-07-23 18:21:54 PDT
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.
Comment 9 Filip Pizlo 2015-07-23 18:25:25 PDT
(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.
Comment 10 Alex Christensen 2015-07-23 18:40:14 PDT
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.
Comment 11 Yusuke Suzuki 2015-07-23 19:26:00 PDT
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.
Comment 12 Yusuke Suzuki 2015-07-23 19:33:11 PDT
Created attachment 257421 [details]
Patch
Comment 13 Alex Christensen 2015-07-24 11:15:24 PDT
Comment on attachment 257421 [details]
Patch

All right then.
Comment 14 WebKit Commit Bot 2015-07-24 12:49:49 PDT
Comment on attachment 257421 [details]
Patch

Clearing flags on attachment: 257421

Committed r187356: <http://trac.webkit.org/changeset/187356>
Comment 15 WebKit Commit Bot 2015-07-24 12:49:54 PDT
All reviewed patches have been landed.  Closing bug.