Remove runtime flags for symbols
Created attachment 257415 [details] Patch
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 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 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.
Oops, I clicked the post button twice...
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.
(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 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 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.
Created attachment 257421 [details] Patch
Comment on attachment 257421 [details] Patch All right then.
Comment on attachment 257421 [details] Patch Clearing flags on attachment: 257421 Committed r187356: <http://trac.webkit.org/changeset/187356>
All reviewed patches have been landed. Closing bug.