Bug 141859

I think we should roll out <http://trac.webkit.org/changeset/179429> until we've resolved this. Yusuke, can you prepare the rollout patch? I tried asking webkitbot, but the rollout needs some by-hand editing to work.

(In reply to comment #1) > I think we should roll out <http://trac.webkit.org/changeset/179429> until > we've resolved this. > > Yusuke, can you prepare the rollout patch? I tried asking webkitbot, but the > rollout needs some by-hand editing to work. Oops. Thank you. I'll do this.

Created attachment 247012 [details] Patch

Attachment 247012 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/NameConstructor.cpp:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/NameInstance.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/CMakeLists.txt:591: Alphabetical sorting problem. "disassembler/UDis86Disassembler.cpp" should be before "disassembler/udis86/udis86_syn.c". [list/order] [5] Total errors found: 4 in 51 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #1) > I think we should roll out <http://trac.webkit.org/changeset/179429> until > we've resolved this. > > Yusuke, can you prepare the rollout patch? I tried asking webkitbot, but the > rollout needs some by-hand editing to work. I've created the patch reverting this change. However, reverted code has some linting errors. Is it preferable to land this reverting patch manually?

Comment on attachment 247012 [details] Patch Attachment 247012 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5346000512221184 New failing tests: inspector/model/parse-script-syntax-tree.html inspector/css/matched-style-properties.html inspector/protocol-promise-result.html inspector/event-listener.html inspector/test-harness-trivially-works.html inspector/css/pseudo-element-matches.html inspector/css/selector-specificity.html inspector/event-listener-set.html

Created attachment 247022 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Oh, inspector already use Symbols in its implementation. Just skipping tests until we've fixed? One concern is that WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js uses Symbol.

Comment on attachment 247012 [details] Patch Attachment 247012 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6387479615635456 New failing tests: inspector/model/parse-script-syntax-tree.html inspector/css/matched-style-properties.html inspector/protocol-promise-result.html inspector/event-listener.html inspector/test-harness-trivially-works.html inspector/css/pseudo-element-matches.html inspector/css/selector-specificity.html inspector/event-listener-set.html

Created attachment 247029 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5

(In reply to comment #1) > I think we should roll out <http://trac.webkit.org/changeset/179429> until > we've resolved this. > > Yusuke, can you prepare the rollout patch? I tried asking webkitbot, but the > rollout needs some by-hand editing to work. After merging, * https://bugs.webkit.org/show_bug.cgi?id=141106 * https://bugs.webkit.org/show_bug.cgi?id=141351 Facebook comment input functionality seems recovered. So it seems that, When there's window.Symbol, facebook's JS use it. But to work with it correctly, it also requires Object.getOwnPropertySymbols and Iterator.next interface additionally.

(In reply to comment #11) > (In reply to comment #1) > > I think we should roll out <http://trac.webkit.org/changeset/179429> until > > we've resolved this. > > > > Yusuke, can you prepare the rollout patch? I tried asking webkitbot, but the > > rollout needs some by-hand editing to work. > > After merging, > > * https://bugs.webkit.org/show_bug.cgi?id=141106 > * https://bugs.webkit.org/show_bug.cgi?id=141351 > > Facebook comment input functionality seems recovered. > So it seems that, > When there's window.Symbol, facebook's JS use it. But to work with it > correctly, it also requires Object.getOwnPropertySymbols and Iterator.next > interface additionally. Is it possible to hide "window.Symbol" in the main world without reverting the whole patch? That way, Inspector can continue to use the symbol and we can unbreak Facebook.

Rather, hide it under some runtime setting so that Inspector can enable it.

(In reply to comment #13) > Rather, hide it under some runtime setting so that Inspector can enable it. What do you think about leveraging JSGlobalObject::m_experimentsEnabled? It works as a runtime flag to switch JS experimental functionality.

(In reply to comment #14) > (In reply to comment #13) > > Rather, hide it under some runtime setting so that Inspector can enable it. > > What do you think about leveraging JSGlobalObject::m_experimentsEnabled? > It works as a runtime flag to switch JS experimental functionality. Or, 1. exposing Symbol with private name. 2. importing Symbol into Inspector WebPage like "InspectorFrontendHost" is just simple.

*** Bug 141580 has been marked as a duplicate of this bug. ***

Created attachment 247043 [details] Patch

Attachment 247043 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 247045 [details] Patch

Comment on attachment 247045 [details] Patch Still checking that inspector works.

<rdar://problem/19837231>

Comment on attachment 247045 [details] Patch Correctly works in inspector context.

Comment on attachment 247045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247045&action=review > Source/WebCore/ChangeLog:8 > + Enable JavaScriptExperiments in inspector context. I'm not certain if it's a good idea to enable all experimental features in JSC. Can we add a new runtime flag to just toggle this?

Comment on attachment 247045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247045&action=review Thank you for your comment! >> Source/WebCore/ChangeLog:8 >> + Enable JavaScriptExperiments in inspector context. > > I'm not certain if it's a good idea to enable all experimental features in JSC. > Can we add a new runtime flag to just toggle this? You're right. More fine-grained runtime flagging is preferable. So I'll introduce runtime flags system instead of reusing this JavaScriptExperiments flag, since there's no system for runtime flagging in JSC.

Comment on attachment 247045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247045&action=review >>> Source/WebCore/ChangeLog:8 >>> + Enable JavaScriptExperiments in inspector context. >> >> I'm not certain if it's a good idea to enable all experimental features in JSC. >> Can we add a new runtime flag to just toggle this? > > You're right. More fine-grained runtime flagging is preferable. > So I'll introduce runtime flags system instead of reusing this JavaScriptExperiments flag, > since there's no system for runtime flagging in JSC. And now, experiments boolean value is not used in any place. So I think replacing it with runtime flags is nice.

Created attachment 247113 [details] Patch

Attachment 247113 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:45: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 247117 [details] Patch

Attachment 247117 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:45: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 247120 [details] Patch

Attachment 247120 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:45: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 247127 [details] Patch

Attachment 247127 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:45: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 247127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247127&action=review Added comments. > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:378 > + putDirectWithoutTransition(vm, vm.propertyNames->Symbol, symbolConstructor, DontEnum); We can switch like this. > Source/WebKit2/UIProcess/API/C/WKPreferencesRef.h:-155 > -WK_EXPORT bool WKPreferencesGetJavaScriptExperimentsEnabled(WKPreferencesRef preferencesRef); One concern is this. In Cocoa, this `JavaScriptExperimentsEnabled` API is hidden as Private. However, C API, `WKPreferencesGetJavaScriptExperimentsEnabled` is exposed. If dropping `WKPreferencesGetJavaScriptExperimentsEnabled` is not good, I suggest that, adding JavaScriptExperimentsEnabled flag into RuntimeFlags and specially handling it (Don't override it with RuntimeFlags setter functions like `WKPreferencesSetJavaScriptRuntimeFlags`). This way can merge JavaScriptExperiments into RuntimeFlags system and still maintain the above API. `WKPreferencesSetJavaScriptExperimentsEnabled`. What do you think of? > Source/WebKit2/UIProcess/gtk/WebInspectorProxyGtk.cpp:73 > + JSC::RuntimeFlags::SymbolEnabled We can specify RuntimeFlags like this.

Comment on attachment 247127 [details] Patch r=me I think it's OK to remove WKPreferencesGetJavaScriptExperimentsEnabled because I think it doesn't have clients.

(In reply to comment #35) > Comment on attachment 247127 [details] > Patch > > r=me > > I think it's OK to remove WKPreferencesGetJavaScriptExperimentsEnabled > because I think it doesn't have clients. OK! So I've cq+ :) Thank you for your review.

Comment on attachment 247127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247127&action=review > Source/JavaScriptCore/runtime/RuntimeFlags.h:34 > +#define JSC_RUNTIME_FLAG(macro) \ > + macro(SymbolEnabled) Nice! I think we can use this to add a runtime flag to enable/disable Promises. Currently in a JSContext (the Cocoa API) Promises don't really work, since there is no concept of a run loop in JSC, and the promises never "fire". Until they work, I would expect Promises to be unavailable.

Comment on attachment 247127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247127&action=review >> Source/JavaScriptCore/runtime/RuntimeFlags.h:34 >> + macro(SymbolEnabled) > > Nice! I think we can use this to add a runtime flag to enable/disable Promises. Currently in a JSContext (the Cocoa API) Promises don't really work, since there is no concept of a run loop in JSC, and the promises never "fire". Until they work, I would expect Promises to be unavailable. That sounds very nice! And by leveraging runtime flags system, we can implement ES6 aggressively :) I think ES6_CLASS_SYNTAX can be moved from a compile time flag to a runtime flag.

Comment on attachment 247127 [details] Patch Rejecting attachment 247127 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: p -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCoreTestSupport.build/Objects-normal/x86_64/MockCDM.o ** BUILD FAILED ** The following build commands failed: CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCoreTestSupport.build/Objects-normal/x86_64/JSInternalSettingsGenerated.o /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSInternalSettingsGenerated.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) Full output: http://webkit-queues.appspot.com/results/5792982053683200

(In reply to comment #39) > Comment on attachment 247127 [details] > Patch > > Rejecting attachment 247127 [details] from commit-queue. > > Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', > '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'build', > '--no-clean', '--no-update', '--build-style=release', '--port=mac']" > exit_code: 2 cwd: /Volumes/Data/EWS/WebKit > > Last 500 characters of output: > p -o > /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/ > WebCoreTestSupport.build/Objects-normal/x86_64/MockCDM.o > > ** BUILD FAILED ** > > > The following build commands failed: > CompileC > /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/ > WebCoreTestSupport.build/Objects-normal/x86_64/JSInternalSettingsGenerated.o > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/ > JSInternalSettingsGenerated.cpp normal x86_64 c++ > com.apple.compilers.llvm.clang.1_0.compiler > (1 failure) > > Full output: http://webkit-queues.appspot.com/results/5792982053683200 Hm, in the patched repository, `git grep -i JavaScriptExperiments` only shows ChangeLogs. So it seems that auto generated DerivedSources/WebCore/JSInternalSettingsGenerated.cpp is not cleaned up and used previously generated.

Comment on attachment 247127 [details] Patch re cq+, previous attempt failed because JSInternalSettingsGenerated.cpp is not cleaned up despite Settings.in is updated.

Comment on attachment 247127 [details] Patch Clearing flags on attachment: 247127 Committed r180547: <http://trac.webkit.org/changeset/180547>

All reviewed patches have been landed. Closing bug.

This patch broke several JavaScript tests on Windows: CONSOLE MESSAGE: line 5: ReferenceError: Can't find variable: Symbol js/symbol-abstract-equality-comparison.html js/symbol-abstract-relational-comparison.html js/symbol-in-map.html js/symbol-object.html js/symbol-prototype-is-ordinary-object.html js/symbol-strict-equality-comparison.html js/symbol-tostring.html js/symbols.html

Re-opened since this is blocked by bug 141957

Created attachment 247208 [details] Patch

Attachment 247208 [details] did not pass style-queue: ERROR: Source/WebKit/win/WebPreferences.cpp:828: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/win/WebPreferences.cpp:835: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/win/WebPreferences.h:155: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/win/WebPreferences.h:158: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:45: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 5 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 247209 [details] Patch

Attachment 247209 [details] did not pass style-queue: ERROR: Source/WebKit/win/WebPreferences.cpp:828: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/win/WebPreferences.cpp:835: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/win/WebPreferences.h:155: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/win/WebPreferences.h:158: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:45: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 5 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 247210 [details] Patch

Attachment 247210 [details] did not pass style-queue: ERROR: Source/WebKit/win/WebPreferences.cpp:828: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/win/WebPreferences.cpp:835: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/win/WebPreferences.h:155: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit/win/WebPreferences.h:158: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:45: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 5 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 247210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247210&action=review Submitted the revised patch with Windows support. I've annotated the difference from the previous patch. > Source/WebKit/win/Interfaces/IWebPreferences.idl:65 > +} WebKitJavaScriptRuntimeFlags; Added enum to COM IDL. > Source/WebKit/win/Interfaces/IWebPreferencesPrivate.idl:117 > + HRESULT setJavaScriptRuntimeFlags([in] unsigned flags); Added COM interface for RuntimeFlags in IWebPreferencesPrivate. > Source/WebKit/win/WebPreferenceKeysPrivate.h:51 > +#define WebKitJavaScriptRuntimeFlagsPreferenceKey "WebKitJavaScriptRuntimeFlags" Key for storing RuntimeFlags into CFDictionary. > Source/WebKit/win/WebPreferences.cpp:218 > + CFDictionaryAddValue(defaults, CFSTR(WebKitJavaScriptRuntimeFlagsPreferenceKey), CFSTR("0")); Default to CFSTR("0"). > Source/WebKit/win/WebPreferences.cpp:840 > + javaScriptRuntimeFlags getter / setter. > Source/WebKit/win/WebPreferences.h:158 > + /* [in] */ unsigned); Added implementation header. > Source/WebKit/win/WebView.cpp:5147 > + settings.setJavaScriptRuntimeFlags(JSC::RuntimeFlags(javaScriptRuntimeFlags)); Transfer RuntimeFlags from Windows specific preference object to WebCore. > Tools/DumpRenderTree/win/DumpRenderTree.cpp:819 > + prefsPrivate->setJavaScriptRuntimeFlags(WebKitJavaScriptRuntimeFlagsAllEnabled); DumpRenderTree in Windows support.

Comment on attachment 247210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247210&action=review Thank you for fixing this! Looks great. r=me. > Tools/DumpRenderTree/win/DumpRenderTree.cpp:820 > // Set JS experiments enabled: YES You can remove this comment, as that option no longer exists (and has been superseded by your new runtime flags)

Comment on attachment 247210 [details] Patch Clearing flags on attachment: 247210 Committed r180570: <http://trac.webkit.org/changeset/180570>

All reviewed patches have been landed. Closing bug.

(In reply to comment #53) > Comment on attachment 247210 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247210&action=review > > Thank you for fixing this! Looks great. r=me. > > > Tools/DumpRenderTree/win/DumpRenderTree.cpp:820 > > // Set JS experiments enabled: YES > > You can remove this comment, as that option no longer exists (and has been > superseded by your new runtime flags) Thank you for your review and investigation of failure caused by my patch! Now Windows Buildbot passed its test :)