Remove Scope::scope_contains_with_ bit

test/mjsunit/debug-scopes.js

 // Copyright 2011 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 134 matching lines @@
 }
 
 
 // Check that the scope chain contains the expected names of scopes.
 function CheckScopeChainNames(names, exec_state) {
   var all_scopes = exec_state.frame().allScopes();
   assertEquals(names.length, all_scopes.length, "FrameMirror.allScopes length");
   for (var i = 0; i < names.length; i++) {
     var scope = exec_state.frame().scope(i);
     assertTrue(scope.isScope());
-    assertEquals(scope.details().name(), names[i])
+    assertEquals(names[i], scope.details().name())
   }
 }
 
 
 // Check that the content of the scope is as expected. For functions just check
 // that there is a function.
 function CheckScopeContent(content, number, exec_state) {
   var scope = exec_state.frame().scope(number);
   var count = 0;
   for (var p in content) {
@@ skipping 664 matching lines @@
   with({x:1}) {
     (function inner(x) {
       debugger;
     })(2);
   }
 }
 
 listener_delegate = function(exec_state) {
   CheckScopeChain([debug.ScopeType.Local,
                    debug.ScopeType.With,
-                   debug.ScopeType.Closure,

[adamk, 2016/03/14 22:22:11]

My understanding is that we simply don't need this one anymore.

                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent({x: 2}, 0, exec_state);
+  CheckScopeContent({x: 1}, 1, exec_state);
 };
 closure_in_with_1();
 EndTest();
 
 
 BeginTest("Closure inside With 2");
 function closure_in_with_2() {
   with({x:1}) {
     (function inner(x) {
       with({x:3}) {
         debugger;
       }
     })(2);
   }
 }
 
 listener_delegate = function(exec_state) {
   CheckScopeChain([debug.ScopeType.With,
                    debug.ScopeType.Local,
                    debug.ScopeType.With,
-                   debug.ScopeType.Closure,

[adamk, 2016/03/14 22:22:11]

Same here.

                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent({x: 3}, 0, exec_state);
   CheckScopeContent({x: 2}, 1, exec_state);
   CheckScopeContent({x: 1}, 2, exec_state);
-  CheckScopeChainNames(["inner", "inner", "closure_in_with_2", "closure_in_with_2", undefined, undefined], exec_state)

[adamk, 2016/03/14 22:22:11]

This is the main bit I don't understand: why did we lose the name entirely?

[Yang, 2016/03/15 06:53:48]

This seems to be a bug.

In ScopeIterator::MaterializeScopeDetails, we get the js_function value from the
context. If we don't have a context, the js_function is an empty handle. At this
point we decide which closure name the scope will have.

However, a couple lines below, we have another chance to fill js_function from
GetFunction(), which gives us the expected closure.

We should retrieve the name after that.

[adamk, 2016/03/15 17:45:46]

Thanks for the pointer, just moving the code down seems to pass all our tests.

Will make sure that devtools tests pass before landing.

+  CheckScopeChainNames(["inner", undefined, "closure_in_with_2", undefined, undefined], exec_state)
 };
 closure_in_with_2();
 EndTest();
 
 
 BeginTest("Closure inside With 3");
 function createClosure(a) {
    var b = a + 1;
    return function closure() {
      var c = b;
@@ skipping 10 matching lines @@
   f();
 }
 
 listener_delegate = function(exec_state) {
   CheckScopeChain([debug.ScopeType.With,
                    debug.ScopeType.Local,
                    debug.ScopeType.Closure,
                    debug.ScopeType.Closure,
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
-  CheckScopeChainNames(["inner", "inner", "closure", "createClosure", undefined, undefined], exec_state)
+  CheckScopeChainNames(["inner", undefined, "closure", "createClosure", undefined, undefined], exec_state)
 }
 closure_in_with_3();
 EndTest();
 
 
 BeginTest("Closure inside With 4");
 listener_delegate = function(exec_state) {
   CheckScopeChain([debug.ScopeType.Local,
                    debug.ScopeType.With,
                    debug.ScopeType.Script,
@@ skipping 56 matching lines @@
 
 
 listener_delegate = function(exec_state) {
   CheckScopeChain([debug.ScopeType.With,
                    debug.ScopeType.Catch,
                    debug.ScopeType.Local,
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent({n:10}, 0, exec_state);
   CheckScopeContent({e:'Exception'}, 1, exec_state);
-  CheckScopeChainNames(["catch_block_2", "catch_block_2", "catch_block_2", undefined, undefined], exec_state)
+  CheckScopeChainNames(["catch_block_2", "catch_block_2", undefined, undefined, undefined], exec_state)
 };
 catch_block_2();
 EndTest();
 
 
 BeginTest("Catch block 3");
 function catch_block_3() {
   // Do eval to dynamically declare a local variable so that the context's
   // extension slot is initialized with JSContextExtensionObject.
   eval("var y = 78;");
@@ skipping 197 matching lines @@
                    debug.ScopeType.Global], exec_state);
   CheckScopeChainPositions([{start: 52, end: 111}, {start: 42, end: 111}, {start: 22, end: 145}, {}, {}], exec_state);
 }
 eval(code3);
 EndTest();
 
 assertEquals(begin_test_count, break_count,
              'one or more tests did not enter the debugger');
 assertEquals(begin_test_count, end_test_count,
              'one or more tests did not have its result checked');