Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1653083002:
Devtools: expose scopes source location to debugger (Closed)
DescriptionDevtools: expose scopes source location to debugger
blink-side cl: https://codereview.chromium.org/1653053002/
BUG=327092
LOG=Y
Committed: https://crrev.com/294d17a7d8f5743eded3f413ff8cfa604eb2e81c
Cr-Commit-Position: refs/heads/master@{#34417}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 15
Patch Set 5 : Address comments #
Messages
Total messages: 39 (14 generated)
sergeyv@chromium.org changed reviewers: + dgozman@chromium.org
Description was changed from
==========
[WIP] Devtools: expose scopes source location to debugger
Version 4.10.70
Performance and stability improvements on all platforms.
Cr-Commit-Position: refs/heads/4.10.70@{#1}
Cr-Branched-From:
8f0fd8c0370ae8c5aab56491b879d7e30c329062-refs/heads/master@{#33523}
BUG=
==========
to
==========
Devtools: expose scopes source location to debugger
blink-side cl: https://codereview.chromium.org/1653053002/
BUG=
==========
Description was changed from ========== Devtools: expose scopes source location to debugger blink-side cl: https://codereview.chromium.org/1653053002/ BUG= ========== to ========== Devtools: expose scopes source location to debugger blink-side cl: https://codereview.chromium.org/1653053002/ BUG=327092 ==========
sergeyv@chromium.org changed reviewers: + yangguo@chromium.org
Yang, can you please take a look on this? https://codereview.chromium.org/1653083002/diff/60001/src/ast/scopes.h File src/ast/scopes.h (left): https://codereview.chromium.org/1653083002/diff/60001/src/ast/scopes.h#oldcod... src/ast/scopes.h:563: void GetNestedScopeChain(Isolate* isolate, List<Handle<ScopeInfo> >* chain, I'm not sure if this is a good idea to move it to debug-scopes.h, but now we need scopes locations along with ScopeInfo. Looks like, to push location into ScopeInfo isn't good way to go: it is especially compressed, while locations are needed only by debugger. I would be glad to hear better options. https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... File test/mjsunit/debug-scopes.js (right): https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... test/mjsunit/debug-scopes.js:1182: CheckScopeChainPositions([{start: 52, end: 111}, {start: 42, end: 111}, {start: 22, end: 145}, {}, {}], exec_state); I don't quite get why do we have 5 scopes instead of 4. Btw, start position for the scope at 0-position looks quite strange. Does it work as intended?
looking good. https://codereview.chromium.org/1653083002/diff/60001/src/ast/scopes.h File src/ast/scopes.h (left): https://codereview.chromium.org/1653083002/diff/60001/src/ast/scopes.h#oldcod... src/ast/scopes.h:563: void GetNestedScopeChain(Isolate* isolate, List<Handle<ScopeInfo> >* chain, On 2016/02/25 20:45:55, sergeyv wrote: > I'm not sure if this is a good idea to move it to debug-scopes.h, but now we > need scopes locations along with ScopeInfo. Looks like, to push location into > ScopeInfo isn't good way to go: it is especially compressed, while locations are > needed only by debugger. > I would be glad to hear better options. ExtendedScopeInfo sgtm. https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.cc File src/debug/debug-scopes.cc (right): https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... src/debug/debug-scopes.cc:76: nested_scope_chain_.Add(ExtendedScopeInfo(scope_info, 0, 0)); the function has a start and an end position, why do we use 0 here? https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... src/debug/debug-scopes.cc:133: Handle<JSFunction> jsFunction = HasContext() naming convention would be js_function https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... src/debug/debug-scopes.cc:141: if (Type() == ScopeTypeGlobal || Type() == ScopeTypeScript) curly brackets on line wrapped if-statement please. https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... src/debug/debug-scopes.cc:842: if (!scope->is_eval_scope()) curly brackets https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... File test/mjsunit/debug-scopes.js (right): https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... test/mjsunit/debug-scopes.js:1139: CheckScopeChainPositions([{start: 58 , end: 118}, {start: 10, end: 162}, {}, {}], exec_state); whitespace before comma is weird https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... test/mjsunit/debug-scopes.js:1182: CheckScopeChainPositions([{start: 52, end: 111}, {start: 42, end: 111}, {start: 22, end: 145}, {}, {}], exec_state); On 2016/02/25 20:45:55, sergeyv wrote: > I don't quite get why do we have 5 scopes instead of 4. > Btw, start position for the scope at 0-position looks quite strange. Does it > work as intended? not sure. can you also add scope type to the check and expectation? might be block scope for the let-variable inside the for-loop.
On 2016/02/25 21:25:17, Yang wrote: > looking good. > > https://codereview.chromium.org/1653083002/diff/60001/src/ast/scopes.h > File src/ast/scopes.h (left): > > https://codereview.chromium.org/1653083002/diff/60001/src/ast/scopes.h#oldcod... > src/ast/scopes.h:563: void GetNestedScopeChain(Isolate* isolate, > List<Handle<ScopeInfo> >* chain, > On 2016/02/25 20:45:55, sergeyv wrote: > > I'm not sure if this is a good idea to move it to debug-scopes.h, but now we > > need scopes locations along with ScopeInfo. Looks like, to push location into > > ScopeInfo isn't good way to go: it is especially compressed, while locations > are > > needed only by debugger. > > I would be glad to hear better options. > > ExtendedScopeInfo sgtm. > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.cc > File src/debug/debug-scopes.cc (right): > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > src/debug/debug-scopes.cc:76: > nested_scope_chain_.Add(ExtendedScopeInfo(scope_info, 0, 0)); > the function has a start and an end position, why do we use 0 here? > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > src/debug/debug-scopes.cc:133: Handle<JSFunction> jsFunction = HasContext() > naming convention would be js_function > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > src/debug/debug-scopes.cc:141: if (Type() == ScopeTypeGlobal || Type() == > ScopeTypeScript) > curly brackets on line wrapped if-statement please. > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > src/debug/debug-scopes.cc:842: if (!scope->is_eval_scope()) > curly brackets > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > File test/mjsunit/debug-scopes.js (right): > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > test/mjsunit/debug-scopes.js:1139: CheckScopeChainPositions([{start: 58 , end: > 118}, {start: 10, end: 162}, {}, {}], exec_state); > whitespace before comma is weird > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > test/mjsunit/debug-scopes.js:1182: CheckScopeChainPositions([{start: 52, end: > 111}, {start: 42, end: 111}, {start: 22, end: 145}, {}, {}], exec_state); > On 2016/02/25 20:45:55, sergeyv wrote: > > I don't quite get why do we have 5 scopes instead of 4. > > Btw, start position for the scope at 0-position looks quite strange. Does it > > work as intended? > > not sure. can you also add scope type to the check and expectation? might be > block scope for the let-variable inside the for-loop. Actually, now that I think about this, maybe using ScopeInfo to store information for the nested scope chain is a bad idea to begin with. We would allocate objects onto the heap that we only need temporarily for the ScopeIterator. Can't we instead just put everything we are interested in directly into that struct that you call ExtendedScopeInfo? We could rename it to ScopeDetails to mirror the JS object that we have in mirrors.js. So instead of calling Scope::GetScopeInfo, we would just copy over from the Scope object what we need, into the struct.
On 2016/02/25 21:33:22, Yang wrote: > On 2016/02/25 21:25:17, Yang wrote: > > looking good. > > > > https://codereview.chromium.org/1653083002/diff/60001/src/ast/scopes.h > > File src/ast/scopes.h (left): > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/ast/scopes.h#oldcod... > > src/ast/scopes.h:563: void GetNestedScopeChain(Isolate* isolate, > > List<Handle<ScopeInfo> >* chain, > > On 2016/02/25 20:45:55, sergeyv wrote: > > > I'm not sure if this is a good idea to move it to debug-scopes.h, but now we > > > need scopes locations along with ScopeInfo. Looks like, to push location > into > > > ScopeInfo isn't good way to go: it is especially compressed, while locations > > are > > > needed only by debugger. > > > I would be glad to hear better options. > > > > ExtendedScopeInfo sgtm. > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.cc > > File src/debug/debug-scopes.cc (right): > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > src/debug/debug-scopes.cc:76: > > nested_scope_chain_.Add(ExtendedScopeInfo(scope_info, 0, 0)); > > the function has a start and an end position, why do we use 0 here? > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > src/debug/debug-scopes.cc:133: Handle<JSFunction> jsFunction = HasContext() > > naming convention would be js_function > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > src/debug/debug-scopes.cc:141: if (Type() == ScopeTypeGlobal || Type() == > > ScopeTypeScript) > > curly brackets on line wrapped if-statement please. > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > src/debug/debug-scopes.cc:842: if (!scope->is_eval_scope()) > > curly brackets > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > File test/mjsunit/debug-scopes.js (right): > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > test/mjsunit/debug-scopes.js:1139: CheckScopeChainPositions([{start: 58 , > end: > > 118}, {start: 10, end: 162}, {}, {}], exec_state); > > whitespace before comma is weird > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > test/mjsunit/debug-scopes.js:1182: CheckScopeChainPositions([{start: 52, end: > > 111}, {start: 42, end: 111}, {start: 22, end: 145}, {}, {}], exec_state); > > On 2016/02/25 20:45:55, sergeyv wrote: > > > I don't quite get why do we have 5 scopes instead of 4. > > > Btw, start position for the scope at 0-position looks quite strange. Does it > > > work as intended? > > > > not sure. can you also add scope type to the check and expectation? might be > > block scope for the let-variable inside the for-loop. > > Actually, now that I think about this, maybe using ScopeInfo to store > information for the nested scope chain is a bad idea to begin with. We would > allocate objects onto the heap that we only need temporarily for the > ScopeIterator. Can't we instead just put everything we are interested in > directly into that struct that you call ExtendedScopeInfo? We could rename it to > ScopeDetails to mirror the JS object that we have in mirrors.js. So instead of > calling Scope::GetScopeInfo, we would just copy over from the Scope object what > we need, into the struct. Sounds cool! I l like your idea and I have just explored how this could be done. Everything is fine, except the one case - BlockStatement: scope_info from nested_scope_chain is used in this case for "frame_inspector_->MaterializeStackLocals..." (line 545) and in "::setBlockVariableValue()". Currently, I can't see how to avoid ScopeInfo in these cases.
On 2016/02/25 22:53:46, sergeyv wrote: > On 2016/02/25 21:33:22, Yang wrote: > > On 2016/02/25 21:25:17, Yang wrote: > > > looking good. > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/ast/scopes.h > > > File src/ast/scopes.h (left): > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/ast/scopes.h#oldcod... > > > src/ast/scopes.h:563: void GetNestedScopeChain(Isolate* isolate, > > > List<Handle<ScopeInfo> >* chain, > > > On 2016/02/25 20:45:55, sergeyv wrote: > > > > I'm not sure if this is a good idea to move it to debug-scopes.h, but now > we > > > > need scopes locations along with ScopeInfo. Looks like, to push location > > into > > > > ScopeInfo isn't good way to go: it is especially compressed, while > locations > > > are > > > > needed only by debugger. > > > > I would be glad to hear better options. > > > > > > ExtendedScopeInfo sgtm. > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.cc > > > File src/debug/debug-scopes.cc (right): > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > src/debug/debug-scopes.cc:76: > > > nested_scope_chain_.Add(ExtendedScopeInfo(scope_info, 0, 0)); > > > the function has a start and an end position, why do we use 0 here? > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > src/debug/debug-scopes.cc:133: Handle<JSFunction> jsFunction = HasContext() > > > naming convention would be js_function > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > src/debug/debug-scopes.cc:141: if (Type() == ScopeTypeGlobal || Type() == > > > ScopeTypeScript) > > > curly brackets on line wrapped if-statement please. > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > src/debug/debug-scopes.cc:842: if (!scope->is_eval_scope()) > > > curly brackets > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > > File test/mjsunit/debug-scopes.js (right): > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > > test/mjsunit/debug-scopes.js:1139: CheckScopeChainPositions([{start: 58 , > > end: > > > 118}, {start: 10, end: 162}, {}, {}], exec_state); > > > whitespace before comma is weird > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > > test/mjsunit/debug-scopes.js:1182: CheckScopeChainPositions([{start: 52, > end: > > > 111}, {start: 42, end: 111}, {start: 22, end: 145}, {}, {}], exec_state); > > > On 2016/02/25 20:45:55, sergeyv wrote: > > > > I don't quite get why do we have 5 scopes instead of 4. > > > > Btw, start position for the scope at 0-position looks quite strange. Does > it > > > > work as intended? > > > > > > not sure. can you also add scope type to the check and expectation? might be > > > block scope for the let-variable inside the for-loop. > > > > Actually, now that I think about this, maybe using ScopeInfo to store > > information for the nested scope chain is a bad idea to begin with. We would > > allocate objects onto the heap that we only need temporarily for the > > ScopeIterator. Can't we instead just put everything we are interested in > > directly into that struct that you call ExtendedScopeInfo? We could rename it > to > > ScopeDetails to mirror the JS object that we have in mirrors.js. So instead of > > calling Scope::GetScopeInfo, we would just copy over from the Scope object > what > > we need, into the struct. > > Sounds cool! I l like your idea and I have just explored how this could be done. > Everything is fine, except the one case - BlockStatement: scope_info from > nested_scope_chain is used in this case for > "frame_inspector_->MaterializeStackLocals..." (line 545) and in > "::setBlockVariableValue()". Currently, I can't see how to avoid ScopeInfo in > these cases. You are right. Never mind that idea...
Addressed comments https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.cc File src/debug/debug-scopes.cc (right): https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... src/debug/debug-scopes.cc:76: nested_scope_chain_.Add(ExtendedScopeInfo(scope_info, 0, 0)); On 2016/02/25 21:25:17, Yang wrote: > the function has a start and an end position, why do we use 0 here? Done. https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... src/debug/debug-scopes.cc:133: Handle<JSFunction> jsFunction = HasContext() On 2016/02/25 21:25:17, Yang wrote: > naming convention would be js_function Done. https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... src/debug/debug-scopes.cc:141: if (Type() == ScopeTypeGlobal || Type() == ScopeTypeScript) On 2016/02/25 21:25:17, Yang wrote: > curly brackets on line wrapped if-statement please. Done. https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... src/debug/debug-scopes.cc:842: if (!scope->is_eval_scope()) On 2016/02/25 21:25:17, Yang wrote: > curly brackets Done. https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... File test/mjsunit/debug-scopes.js (right): https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... test/mjsunit/debug-scopes.js:1139: CheckScopeChainPositions([{start: 58 , end: 118}, {start: 10, end: 162}, {}, {}], exec_state); On 2016/02/25 21:25:17, Yang wrote: > whitespace before comma is weird Done. https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... test/mjsunit/debug-scopes.js:1182: CheckScopeChainPositions([{start: 52, end: 111}, {start: 42, end: 111}, {start: 22, end: 145}, {}, {}], exec_state); On 2016/02/25 21:25:17, Yang wrote: > On 2016/02/25 20:45:55, sergeyv wrote: > > I don't quite get why do we have 5 scopes instead of 4. > > Btw, start position for the scope at 0-position looks quite strange. Does it > > work as intended? > > not sure. can you also add scope type to the check and expectation? might be > block scope for the let-variable inside the for-loop. Added scope type. We have two block-statement here, from Devtools point of view it looks strange: there are two scopes in both of them i is defined and have the same value.
Description was changed from ========== Devtools: expose scopes source location to debugger blink-side cl: https://codereview.chromium.org/1653053002/ BUG=327092 ========== to ========== Devtools: expose scopes source location to debugger blink-side cl: https://codereview.chromium.org/1653053002/ BUG=327092 LOG=Y ==========
On 2016/02/26 00:29:30, sergeyv wrote: > Addressed comments > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.cc > File src/debug/debug-scopes.cc (right): > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > src/debug/debug-scopes.cc:76: > nested_scope_chain_.Add(ExtendedScopeInfo(scope_info, 0, 0)); > On 2016/02/25 21:25:17, Yang wrote: > > the function has a start and an end position, why do we use 0 here? > > Done. > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > src/debug/debug-scopes.cc:133: Handle<JSFunction> jsFunction = HasContext() > On 2016/02/25 21:25:17, Yang wrote: > > naming convention would be js_function > > Done. > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > src/debug/debug-scopes.cc:141: if (Type() == ScopeTypeGlobal || Type() == > ScopeTypeScript) > On 2016/02/25 21:25:17, Yang wrote: > > curly brackets on line wrapped if-statement please. > > Done. > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > src/debug/debug-scopes.cc:842: if (!scope->is_eval_scope()) > On 2016/02/25 21:25:17, Yang wrote: > > curly brackets > > Done. > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > File test/mjsunit/debug-scopes.js (right): > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > test/mjsunit/debug-scopes.js:1139: CheckScopeChainPositions([{start: 58 , end: > 118}, {start: 10, end: 162}, {}, {}], exec_state); > On 2016/02/25 21:25:17, Yang wrote: > > whitespace before comma is weird > > Done. > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > test/mjsunit/debug-scopes.js:1182: CheckScopeChainPositions([{start: 52, end: > 111}, {start: 42, end: 111}, {start: 22, end: 145}, {}, {}], exec_state); > On 2016/02/25 21:25:17, Yang wrote: > > On 2016/02/25 20:45:55, sergeyv wrote: > > > I don't quite get why do we have 5 scopes instead of 4. > > > Btw, start position for the scope at 0-position looks quite strange. Does it > > > work as intended? > > > > not sure. can you also add scope type to the check and expectation? might be > > block scope for the let-variable inside the for-loop. > > Added scope type. We have two block-statement here, from Devtools point of view > it looks strange: there are two scopes in both of them i is defined and have the > same value. I'm on vacation today. Can you wait till Monday so that I figure out what's up with the two block scopes? Sorry!
On 2016/02/26 03:34:12, Yang wrote: > On 2016/02/26 00:29:30, sergeyv wrote: > > Addressed comments > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.cc > > File src/debug/debug-scopes.cc (right): > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > src/debug/debug-scopes.cc:76: > > nested_scope_chain_.Add(ExtendedScopeInfo(scope_info, 0, 0)); > > On 2016/02/25 21:25:17, Yang wrote: > > > the function has a start and an end position, why do we use 0 here? > > > > Done. > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > src/debug/debug-scopes.cc:133: Handle<JSFunction> jsFunction = HasContext() > > On 2016/02/25 21:25:17, Yang wrote: > > > naming convention would be js_function > > > > Done. > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > src/debug/debug-scopes.cc:141: if (Type() == ScopeTypeGlobal || Type() == > > ScopeTypeScript) > > On 2016/02/25 21:25:17, Yang wrote: > > > curly brackets on line wrapped if-statement please. > > > > Done. > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > src/debug/debug-scopes.cc:842: if (!scope->is_eval_scope()) > > On 2016/02/25 21:25:17, Yang wrote: > > > curly brackets > > > > Done. > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > File test/mjsunit/debug-scopes.js (right): > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > test/mjsunit/debug-scopes.js:1139: CheckScopeChainPositions([{start: 58 , > end: > > 118}, {start: 10, end: 162}, {}, {}], exec_state); > > On 2016/02/25 21:25:17, Yang wrote: > > > whitespace before comma is weird > > > > Done. > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > test/mjsunit/debug-scopes.js:1182: CheckScopeChainPositions([{start: 52, end: > > 111}, {start: 42, end: 111}, {start: 22, end: 145}, {}, {}], exec_state); > > On 2016/02/25 21:25:17, Yang wrote: > > > On 2016/02/25 20:45:55, sergeyv wrote: > > > > I don't quite get why do we have 5 scopes instead of 4. > > > > Btw, start position for the scope at 0-position looks quite strange. Does > it > > > > work as intended? > > > > > > not sure. can you also add scope type to the check and expectation? might be > > > block scope for the let-variable inside the for-loop. > > > > Added scope type. We have two block-statement here, from Devtools point of > view > > it looks strange: there are two scopes in both of them i is defined and have > the > > same value. > > I'm on vacation today. Can you wait till Monday so that I figure out what's up > with the two block scopes? Sorry! I thought about this and I think I know what's up with the two block scopes. The for-let loop creates a new block context in every iteration so that when you bind thelet variable i inside the loop body, you get distinct bindings in every loop and dont share the same variable across loops like with for-var loops. However, the value of the loop variable also needs to carry over between two iterations, so we create a outer block scope with a temporary variable to do that. You can find the desugaring in src/parsing/parser.cc ParseForStatement. There are a few more desugarings like this where we introduce additional scopes (for default arguments for example). I'm not sure whether these desugarings assign sensical variable names or scope positions to additional scopes. Maybe we should have a way to flag those scopes as hidden and not show them in the debugger?
On 2016/02/26 04:06:40, Yang wrote: > On 2016/02/26 03:34:12, Yang wrote: > > On 2016/02/26 00:29:30, sergeyv wrote: > > > Addressed comments > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.cc > > > File src/debug/debug-scopes.cc (right): > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > src/debug/debug-scopes.cc:76: > > > nested_scope_chain_.Add(ExtendedScopeInfo(scope_info, 0, 0)); > > > On 2016/02/25 21:25:17, Yang wrote: > > > > the function has a start and an end position, why do we use 0 here? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > src/debug/debug-scopes.cc:133: Handle<JSFunction> jsFunction = HasContext() > > > On 2016/02/25 21:25:17, Yang wrote: > > > > naming convention would be js_function > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > src/debug/debug-scopes.cc:141: if (Type() == ScopeTypeGlobal || Type() == > > > ScopeTypeScript) > > > On 2016/02/25 21:25:17, Yang wrote: > > > > curly brackets on line wrapped if-statement please. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > src/debug/debug-scopes.cc:842: if (!scope->is_eval_scope()) > > > On 2016/02/25 21:25:17, Yang wrote: > > > > curly brackets > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > > File test/mjsunit/debug-scopes.js (right): > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > > test/mjsunit/debug-scopes.js:1139: CheckScopeChainPositions([{start: 58 , > > end: > > > 118}, {start: 10, end: 162}, {}, {}], exec_state); > > > On 2016/02/25 21:25:17, Yang wrote: > > > > whitespace before comma is weird > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > > test/mjsunit/debug-scopes.js:1182: CheckScopeChainPositions([{start: 52, > end: > > > 111}, {start: 42, end: 111}, {start: 22, end: 145}, {}, {}], exec_state); > > > On 2016/02/25 21:25:17, Yang wrote: > > > > On 2016/02/25 20:45:55, sergeyv wrote: > > > > > I don't quite get why do we have 5 scopes instead of 4. > > > > > Btw, start position for the scope at 0-position looks quite strange. > Does > > it > > > > > work as intended? > > > > > > > > not sure. can you also add scope type to the check and expectation? might > be > > > > block scope for the let-variable inside the for-loop. > > > > > > Added scope type. We have two block-statement here, from Devtools point of > > view > > > it looks strange: there are two scopes in both of them i is defined and have > > the > > > same value. > > > > I'm on vacation today. Can you wait till Monday so that I figure out what's up > > with the two block scopes? Sorry! > > I thought about this and I think I know what's up with the two block scopes. > > The for-let loop creates a new block context in every iteration so that when you > bind thelet variable i inside the loop body, you get distinct bindings in every > loop and dont share the same variable across loops like with for-var loops. > However, the value of the loop variable also needs to carry over between two > iterations, so we create a outer block scope with a temporary variable to do > that. You can find the desugaring in src/parsing/parser.cc ParseForStatement. > > There are a few more desugarings like this where we introduce additional scopes > (for default arguments for example). I'm not sure whether these desugarings > assign sensical variable names or scope positions to additional scopes. Maybe we > should have a way to flag those scopes as hidden and not show them in the > debugger? Thank you for explanation! Yeah, I think we should flag and hide this scopes from debugger. Have good vacation and lets continue this on Monday =)
On 2016/02/26 06:06:41, sergeyv wrote: > On 2016/02/26 04:06:40, Yang wrote: > > On 2016/02/26 03:34:12, Yang wrote: > > > On 2016/02/26 00:29:30, sergeyv wrote: > > > > Addressed comments > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.cc > > > > File src/debug/debug-scopes.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > > src/debug/debug-scopes.cc:76: > > > > nested_scope_chain_.Add(ExtendedScopeInfo(scope_info, 0, 0)); > > > > On 2016/02/25 21:25:17, Yang wrote: > > > > > the function has a start and an end position, why do we use 0 here? > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > > src/debug/debug-scopes.cc:133: Handle<JSFunction> jsFunction = > HasContext() > > > > On 2016/02/25 21:25:17, Yang wrote: > > > > > naming convention would be js_function > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > > src/debug/debug-scopes.cc:141: if (Type() == ScopeTypeGlobal || Type() == > > > > ScopeTypeScript) > > > > On 2016/02/25 21:25:17, Yang wrote: > > > > > curly brackets on line wrapped if-statement please. > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > > src/debug/debug-scopes.cc:842: if (!scope->is_eval_scope()) > > > > On 2016/02/25 21:25:17, Yang wrote: > > > > > curly brackets > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > > > File test/mjsunit/debug-scopes.js (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > > > test/mjsunit/debug-scopes.js:1139: CheckScopeChainPositions([{start: 58 , > > > end: > > > > 118}, {start: 10, end: 162}, {}, {}], exec_state); > > > > On 2016/02/25 21:25:17, Yang wrote: > > > > > whitespace before comma is weird > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > > > test/mjsunit/debug-scopes.js:1182: CheckScopeChainPositions([{start: 52, > > end: > > > > 111}, {start: 42, end: 111}, {start: 22, end: 145}, {}, {}], exec_state); > > > > On 2016/02/25 21:25:17, Yang wrote: > > > > > On 2016/02/25 20:45:55, sergeyv wrote: > > > > > > I don't quite get why do we have 5 scopes instead of 4. > > > > > > Btw, start position for the scope at 0-position looks quite strange. > > Does > > > it > > > > > > work as intended? > > > > > > > > > > not sure. can you also add scope type to the check and expectation? > might > > be > > > > > block scope for the let-variable inside the for-loop. > > > > > > > > Added scope type. We have two block-statement here, from Devtools point of > > > view > > > > it looks strange: there are two scopes in both of them i is defined and > have > > > the > > > > same value. > > > > > > I'm on vacation today. Can you wait till Monday so that I figure out what's > up > > > with the two block scopes? Sorry! > > > > I thought about this and I think I know what's up with the two block scopes. > > > > The for-let loop creates a new block context in every iteration so that when > you > > bind thelet variable i inside the loop body, you get distinct bindings in > every > > loop and dont share the same variable across loops like with for-var loops. > > However, the value of the loop variable also needs to carry over between two > > iterations, so we create a outer block scope with a temporary variable to do > > that. You can find the desugaring in src/parsing/parser.cc ParseForStatement. > > > > There are a few more desugarings like this where we introduce additional > scopes > > (for default arguments for example). I'm not sure whether these desugarings > > assign sensical variable names or scope positions to additional scopes. Maybe > we > > should have a way to flag those scopes as hidden and not show them in the > > debugger? > > Thank you for explanation! Yeah, I think we should flag and hide this scopes > from debugger. > Have good vacation and lets continue this on Monday =) I guess what we should do here would be setting a flag on the Scope object during parsing to hide it for debugging. GetNestedScopeChain would then ignore scopes with this flag. Would you be willing to implement that in a separate CL? This CL lgtm.
On 2016/02/29 08:14:43, Yang wrote: > On 2016/02/26 06:06:41, sergeyv wrote: > > On 2016/02/26 04:06:40, Yang wrote: > > > On 2016/02/26 03:34:12, Yang wrote: > > > > On 2016/02/26 00:29:30, sergeyv wrote: > > > > > Addressed comments > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.cc > > > > > File src/debug/debug-scopes.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > > > src/debug/debug-scopes.cc:76: > > > > > nested_scope_chain_.Add(ExtendedScopeInfo(scope_info, 0, 0)); > > > > > On 2016/02/25 21:25:17, Yang wrote: > > > > > > the function has a start and an end position, why do we use 0 here? > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > > > src/debug/debug-scopes.cc:133: Handle<JSFunction> jsFunction = > > HasContext() > > > > > On 2016/02/25 21:25:17, Yang wrote: > > > > > > naming convention would be js_function > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > > > src/debug/debug-scopes.cc:141: if (Type() == ScopeTypeGlobal || Type() > == > > > > > ScopeTypeScript) > > > > > On 2016/02/25 21:25:17, Yang wrote: > > > > > > curly brackets on line wrapped if-statement please. > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/src/debug/debug-scopes.... > > > > > src/debug/debug-scopes.cc:842: if (!scope->is_eval_scope()) > > > > > On 2016/02/25 21:25:17, Yang wrote: > > > > > > curly brackets > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > > > > File test/mjsunit/debug-scopes.js (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > > > > test/mjsunit/debug-scopes.js:1139: CheckScopeChainPositions([{start: 58 > , > > > > end: > > > > > 118}, {start: 10, end: 162}, {}, {}], exec_state); > > > > > On 2016/02/25 21:25:17, Yang wrote: > > > > > > whitespace before comma is weird > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1653083002/diff/60001/test/mjsunit/debug-scop... > > > > > test/mjsunit/debug-scopes.js:1182: CheckScopeChainPositions([{start: 52, > > > end: > > > > > 111}, {start: 42, end: 111}, {start: 22, end: 145}, {}, {}], > exec_state); > > > > > On 2016/02/25 21:25:17, Yang wrote: > > > > > > On 2016/02/25 20:45:55, sergeyv wrote: > > > > > > > I don't quite get why do we have 5 scopes instead of 4. > > > > > > > Btw, start position for the scope at 0-position looks quite strange. > > > Does > > > > it > > > > > > > work as intended? > > > > > > > > > > > > not sure. can you also add scope type to the check and expectation? > > might > > > be > > > > > > block scope for the let-variable inside the for-loop. > > > > > > > > > > Added scope type. We have two block-statement here, from Devtools point > of > > > > view > > > > > it looks strange: there are two scopes in both of them i is defined and > > have > > > > the > > > > > same value. > > > > > > > > I'm on vacation today. Can you wait till Monday so that I figure out > what's > > up > > > > with the two block scopes? Sorry! > > > > > > I thought about this and I think I know what's up with the two block scopes. > > > > > > The for-let loop creates a new block context in every iteration so that when > > you > > > bind thelet variable i inside the loop body, you get distinct bindings in > > every > > > loop and dont share the same variable across loops like with for-var loops. > > > However, the value of the loop variable also needs to carry over between two > > > iterations, so we create a outer block scope with a temporary variable to do > > > that. You can find the desugaring in src/parsing/parser.cc > ParseForStatement. > > > > > > There are a few more desugarings like this where we introduce additional > > scopes > > > (for default arguments for example). I'm not sure whether these desugarings > > > assign sensical variable names or scope positions to additional scopes. > Maybe > > we > > > should have a way to flag those scopes as hidden and not show them in the > > > debugger? > > > > Thank you for explanation! Yeah, I think we should flag and hide this scopes > > from debugger. > > Have good vacation and lets continue this on Monday =) > > I guess what we should do here would be setting a flag on the Scope object > during parsing to hide it for debugging. GetNestedScopeChain would then ignore > scopes with this flag. > > Would you be willing to implement that in a separate CL? Unfortunately, I think I wouldn't have a chance to do this, but @kozyatinskiy will cover me. > This CL lgtm. Thank you.
sergeyv@chromium.org changed reviewers: + kozyatinskiy@chromium.org
The CQ bit was checked by sergeyv@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1653083002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1653083002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/11743)
The CQ bit was checked by sergeyv@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1653083002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1653083002/80001
sergeyv@chromium.org changed reviewers: + adamk@chromium.org
@adamk, can you please take a look on this? I need an owner LGTM for src/ast
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ast/ lgtm
The CQ bit was checked by sergeyv@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1653083002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1653083002/80001
Message was sent while issue was closed.
Description was changed from ========== Devtools: expose scopes source location to debugger blink-side cl: https://codereview.chromium.org/1653053002/ BUG=327092 LOG=Y ========== to ========== Devtools: expose scopes source location to debugger blink-side cl: https://codereview.chromium.org/1653053002/ BUG=327092 LOG=Y ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Devtools: expose scopes source location to debugger blink-side cl: https://codereview.chromium.org/1653053002/ BUG=327092 LOG=Y ========== to ========== Devtools: expose scopes source location to debugger blink-side cl: https://codereview.chromium.org/1653053002/ BUG=327092 LOG=Y Committed: https://crrev.com/294d17a7d8f5743eded3f413ff8cfa604eb2e81c Cr-Commit-Position: refs/heads/master@{#34417} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/294d17a7d8f5743eded3f413ff8cfa604eb2e81c Cr-Commit-Position: refs/heads/master@{#34417}
Message was sent while issue was closed.
On 2016/03/02 02:20:11, commit-bot: I haz the power wrote: > Patchset 5 (id:??) landed as > https://crrev.com/294d17a7d8f5743eded3f413ff8cfa604eb2e81c > Cr-Commit-Position: refs/heads/master@{#34417} Sergey, can you file a bug for hiding scopes and assign Alexey to it, and CC me, so that this stays on our radar? Thanks.
Message was sent while issue was closed.
On 2016/03/02 03:29:27, Yang wrote: > On 2016/03/02 02:20:11, commit-bot: I haz the power wrote: > > Patchset 5 (id:??) landed as > > https://crrev.com/294d17a7d8f5743eded3f413ff8cfa604eb2e81c > > Cr-Commit-Position: refs/heads/master@{#34417} > > Sergey, can you file a bug for hiding scopes and assign Alexey to it, and CC me, > so that this stays on our radar? Thanks. Sure! Done : https://bugs.chromium.org/p/chromium/issues/detail?id=591273
Message was sent while issue was closed.
On 2016/03/02 03:45:07, sergeyv wrote: > On 2016/03/02 03:29:27, Yang wrote: > > On 2016/03/02 02:20:11, commit-bot: I haz the power wrote: > > > Patchset 5 (id:??) landed as > > > https://crrev.com/294d17a7d8f5743eded3f413ff8cfa604eb2e81c > > > Cr-Commit-Position: refs/heads/master@{#34417} > > > > Sergey, can you file a bug for hiding scopes and assign Alexey to it, and CC > me, > > so that this stays on our radar? Thanks. > > Sure! Done : https://bugs.chromium.org/p/chromium/issues/detail?id=591273 Thank you! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
