* SUMMARY Add a BlockScope to SymbolTable::ScopeType. This will allow the inspector to show more detailed information in the Scope Chain sidebar while debugging.
We may not want to add a new "BlockType" but just an extra bit that says if a "LexicalScopeType" is an "inner block" or not.
The inspector would like to be able to distinguish a nested lexical block from the function's lexical block. For example: function foo() { var local = 1; let bar = 2; if (true) { let bar = 3; } } In this situation there are two LexicalScope's each containing an identifier "bar". The Inspector would like to better display the scope chain. Informing the user of the local variables owned everywhere within "foo" (the first `let bar`) and the lexically scoped variables inside of a nested block (the second `let bar`). Possible UI, when paused inside the if block: Block Variables: bar: 3 Local Variables (foo): bar: 2 local: 1 Lexical Global Variables: ... Global Variables: ...
Created attachment 267529 [details] [PATCH] Proposed Fix Next steps would be: 1. Eliminating "local" scope from the protocol, as the backend no longer sends it. 2. Better associate CallFrames with the ScopeChain so that the frontend can merge a Function's 1 or 2 base Closure scopes (for `vars` and `lets`) into a single "Local Variables" section. The backend should send enough data to make this possible on the frontend.
Attachment 267529 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/JSJavaScriptCallFrame.h:81: NESTED_LEXICAL_SCOPE is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 267530 [details] [IMAGE] After - Nested Block Scopes different from Function Closure Scopes
Comment on attachment 267529 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=267529&action=review > Source/JavaScriptCore/runtime/SymbolTable.h:692 > + bool m_nestedLexicalScope : 1; // Non-function LexicalScope. Nit: this comment is a bit confusing to me. Maybe: "Lexical scope that is not at the base-level of a function"?
Comment on attachment 267529 [details] [PATCH] Proposed Fix Attachment 267529 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/569931 New failing tests: inspector/debugger/breakpoint-scope.html
Created attachment 267536 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 267529 [details] [PATCH] Proposed Fix Attachment 267529 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/569934 New failing tests: inspector/debugger/breakpoint-scope.html
Created attachment 267537 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 267529 [details] [PATCH] Proposed Fix Attachment 267529 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/569949 New failing tests: inspector/debugger/breakpoint-scope.html
Created attachment 267538 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 267571 [details] [PATCH] Proposed Fix
Attachment 267571 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/JSJavaScriptCallFrame.h:81: NESTED_LEXICAL_SCOPE is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 267571 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=267571&action=review looks good to me, I'll let other people critique > LayoutTests/ChangeLog:3 > + Provide a way to distinguish a nested lexical block from a function's lexical block I like these tests. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3586 > + pushLexicalScopeInternal(environment, true, false, nullptr, TDZRequirement::NotUnderTDZ, ScopeType::CatchScope, ScopeRegisterType::Block); We don't want this to be true?
Comment on attachment 267571 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=267571&action=review >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3586 >> + pushLexicalScopeInternal(environment, true, false, nullptr, TDZRequirement::NotUnderTDZ, ScopeType::CatchScope, ScopeRegisterType::Block); > > We don't want this to be true? Correct. For now, this CatchScope only covers the single identifier in the "catch (e)" portion. Much like WithBlock only covers the single object in with (...)". The braces following it open a new lexical scope, which will correctly be identifier as a Block scope. We may want to clean this up later but that can be done in the frontend. CatchScope / WithBlock will both always be followed by a BlockScope (I think). I doubt "catch(e);" or "with(e);" are legal JS.
Comment on attachment 267571 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=267571&action=review >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3586 >> + pushLexicalScopeInternal(environment, true, false, nullptr, TDZRequirement::NotUnderTDZ, ScopeType::CatchScope, ScopeRegisterType::Block); > > We don't want this to be true? Never mind, we already do something better.
(In reply to comment #16) > Comment on attachment 267571 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=267571&action=review > > >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3586 > >> + pushLexicalScopeInternal(environment, true, false, nullptr, TDZRequirement::NotUnderTDZ, ScopeType::CatchScope, ScopeRegisterType::Block); > > > > We don't want this to be true? > > Correct. For now, this CatchScope only covers the single identifier in the > "catch (e)" portion. Much like WithBlock only covers the single object in > with (...)". The braces following it open a new lexical scope, which will > correctly be identifier as a Block scope. We may want to clean this up later > but that can be done in the frontend. CatchScope / WithBlock will both > always be followed by a BlockScope (I think). I doubt "catch(e);" or > "with(e);" are legal JS. It looks like "catch" must be followed by a block and "with" must be followed by any statement.
<http://trac.webkit.org/changeset/194251>
Comment on attachment 267571 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=267571&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:812 > + pushLexicalScopeInternal(environment, true, false, nullptr, TDZRequirement::UnderTDZ, ScopeType::LetConstScope, ScopeRegisterType::Block); It would be more readable to give these booleans names.
(In reply to comment #20) > Comment on attachment 267571 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=267571&action=review > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:812 > > + pushLexicalScopeInternal(environment, true, false, nullptr, TDZRequirement::UnderTDZ, ScopeType::LetConstScope, ScopeRegisterType::Block); > > It would be more readable to give these booleans names. I think we should just enumify all the boolean arguments to this function.