[debugger] refactor debug-scopes.

src/debug/debug-scopes.cc

Index: src/debug/debug-scopes.cc
diff --git a/src/debug/debug-scopes.cc b/src/debug/debug-scopes.cc
index 76beed3b9e54f0d8493dce38b0889704d578b048..9d4c38343a0e1a359da75a28612f5f582ee9b544 100644
--- a/src/debug/debug-scopes.cc
+++ b/src/debug/debug-scopes.cc
@@ -607,12 +607,43 @@ MaybeHandle<JSObject> ScopeIterator::MaterializeModuleScope() {
   return module_scope;
 }
 

[jgruber1, 2016/05/09 09:17:10]

Nit: Here and below, two lines between functions.

[Yang, 2016/05/09 12:58:28]

Actually the new guideline is to have only one line between functions, and
clang-format enforces that for new code.

+bool ScopeIterator::SetParameterValue(Handle<ScopeInfo> scope_info,
+                                      JavaScriptFrame* frame,
+                                      Handle<String> parameter_name,
+                                      Handle<Object> new_value) {
+  // Setting stack locals of optimized frames is not supported.
+  if (frame->is_optimized()) return false;
+  HandleScope scope(isolate_);
+  for (int i = 0; i < scope_info->ParameterCount(); ++i) {
+    if (String::Equals(handle(scope_info->ParameterName(i)), parameter_name)) {
+      frame->SetParameterValue(i, *new_value);
+      return true;
+    }
+  }
+  return false;
+}
+
+bool ScopeIterator::SetStackVariableValue(Handle<ScopeInfo> scope_info,
+                                          JavaScriptFrame* frame,
+                                          Handle<String> variable_name,
+                                          Handle<Object> new_value) {
+  // Setting stack locals of optimized frames is not supported.
+  if (frame->is_optimized()) return false;
+  HandleScope scope(isolate_);
+  for (int i = 0; i < scope_info->StackLocalCount(); ++i) {
+    if (String::Equals(handle(scope_info->StackLocalName(i)), variable_name)) {
+      frame->SetExpression(scope_info->StackLocalIndex(i), *new_value);
+      return true;
+    }
+  }
+  return false;
+}
 
-// Set the context local variable value.
-bool ScopeIterator::SetContextLocalValue(Handle<ScopeInfo> scope_info,
-                                         Handle<Context> context,
-                                         Handle<String> variable_name,
-                                         Handle<Object> new_value) {
+bool ScopeIterator::SetContextVariableValue(Handle<ScopeInfo> scope_info,
+                                            Handle<Context> context,
+                                            Handle<String> variable_name,
+                                            Handle<Object> new_value) {
+  HandleScope scope(isolate_);
   for (int i = 0; i < scope_info->ContextLocalCount(); i++) {
     Handle<String> next_name(scope_info->ContextLocalName(i));
     if (String::Equals(variable_name, next_name)) {
@@ -626,143 +657,70 @@ bool ScopeIterator::SetContextLocalValue(Handle<ScopeInfo> scope_info,
     }
   }
 
+  Handle<JSObject> ext(context->extension_object());
+  if (!ext.is_null()) {

[jgruber1, 2016/05/09 09:17:11]

Nit: Possibly context->has_extension() for readability?

+    Maybe<bool> maybe = JSReceiver::HasOwnProperty(ext, variable_name);
+    DCHECK(maybe.IsJust());
+    if (maybe.FromJust()) {
+      // We don't expect this to do anything except replacing property value.
+      JSObject::SetOwnPropertyIgnoreAttributes(ext, variable_name, new_value,
+                                               NONE)
+          .Check();
+      return true;
+    }
+  }
+
   return false;
 }
 
-
 bool ScopeIterator::SetLocalVariableValue(Handle<String> variable_name,
                                           Handle<Object> new_value) {
   JavaScriptFrame* frame = GetFrame();
-  // Optimized frames are not supported.
-  if (frame->is_optimized()) return false;
+  Handle<ScopeInfo> scope_info(frame->function()->shared()->scope_info());
 
-  Handle<JSFunction> function(frame->function());
-  Handle<SharedFunctionInfo> shared(function->shared());
-  Handle<ScopeInfo> scope_info(shared->scope_info());
-
-  bool default_result = false;
-
-  // Parameters.
-  for (int i = 0; i < scope_info->ParameterCount(); ++i) {
-    HandleScope scope(isolate_);
-    if (String::Equals(handle(scope_info->ParameterName(i)), variable_name)) {
-      frame->SetParameterValue(i, *new_value);
-      // Argument might be shadowed in heap context, don't stop here.
-      default_result = true;
-    }
-  }
+  // Parameter might be shadowed in context. Don't stop here.
+  bool result = SetParameterValue(scope_info, frame, variable_name, new_value);
 
   // Stack locals.
-  for (int i = 0; i < scope_info->StackLocalCount(); ++i) {
-    HandleScope scope(isolate_);
-    if (String::Equals(handle(scope_info->StackLocalName(i)), variable_name)) {
-      frame->SetExpression(scope_info->StackLocalIndex(i), *new_value);
-      return true;
-    }
+  if (SetStackVariableValue(scope_info, frame, variable_name, new_value)) {
+    return true;
   }
 
-  if (scope_info->HasContext()) {
-    // Context locals.
-    Handle<Context> frame_context(Context::cast(frame->context()));
-    Handle<Context> function_context(frame_context->declaration_context());
-    if (SetContextLocalValue(scope_info, function_context, variable_name,
-                             new_value)) {
-      return true;
-    }
-
-    // Function context extension. These are variables introduced by eval.
-    if (function_context->closure() == *function) {
-      if (function_context->has_extension() &&
-          !function_context->IsNativeContext()) {
-        Handle<JSObject> ext(function_context->extension_object());
-
-        Maybe<bool> maybe = JSReceiver::HasProperty(ext, variable_name);
-        DCHECK(maybe.IsJust());
-        if (maybe.FromJust()) {
-          // We don't expect this to do anything except replacing
-          // property value.
-          Runtime::SetObjectProperty(isolate_, ext, variable_name, new_value,
-                                     SLOPPY)
-              .Assert();
-          return true;
-        }
-      }
-    }
+  if (scope_info->HasContext() &&
+      SetContextVariableValue(scope_info, CurrentContext(), variable_name,
+                              new_value)) {
+    return true;
   }
 
-  return default_result;
+  return result;
 }
 
-
 bool ScopeIterator::SetBlockVariableValue(Handle<String> variable_name,
                                           Handle<Object> new_value) {
   Handle<ScopeInfo> scope_info = CurrentScopeInfo();
   JavaScriptFrame* frame = GetFrame();
 
-  for (int i = 0; i < scope_info->StackLocalCount(); ++i) {
-    HandleScope scope(isolate_);
-    if (String::Equals(handle(scope_info->StackLocalName(i)), variable_name)) {
-      frame->SetExpression(scope_info->StackLocalIndex(i), *new_value);
-      return true;
-    }
+  // Setting stack locals of optimized frames is not supported.
+  if (SetStackVariableValue(scope_info, frame, variable_name, new_value)) {
+    return true;
   }
 
-  if (HasContext()) {
-    Handle<Context> context = CurrentContext();
-    if (SetContextLocalValue(scope_info, context, variable_name, new_value)) {
-      return true;
-    }
-
-    Handle<JSObject> ext(context->extension_object(), isolate_);
-    if (!ext.is_null()) {
-      Maybe<bool> maybe = JSReceiver::HasOwnProperty(ext, variable_name);
-      DCHECK(maybe.IsJust());
-      if (maybe.FromJust()) {
-        // We don't expect this to do anything except replacing property value.
-        JSObject::SetOwnPropertyIgnoreAttributes(ext, variable_name, new_value,
-                                                 NONE)
-            .Check();
-        return true;
-      }
-    }
+  if (HasContext() && SetContextVariableValue(scope_info, CurrentContext(),
+                                              variable_name, new_value)) {
+    return true;
   }
 
   return false;
 }
 
-
 // This method copies structure of MaterializeClosure method above.
 bool ScopeIterator::SetClosureVariableValue(Handle<String> variable_name,
                                             Handle<Object> new_value) {
-  Handle<Context> context = CurrentContext();
-  DCHECK(context->IsFunctionContext());
-
-  // Context locals to the context extension.
-  Handle<SharedFunctionInfo> shared(context->closure()->shared());
-  Handle<ScopeInfo> scope_info(shared->scope_info());
-  if (SetContextLocalValue(scope_info, context, variable_name, new_value)) {
-    return true;
-  }
-
-  // Properties from the function context extension. This will
-  // be variables introduced by eval.
-  if (context->has_extension()) {
-    Handle<JSObject> ext(JSObject::cast(context->extension_object()));
-    Maybe<bool> maybe = JSReceiver::HasOwnProperty(ext, variable_name);
-    DCHECK(maybe.IsJust());
-    if (maybe.FromJust()) {
-      // We don't expect this to do anything except replacing property value.
-      JSObject::SetOwnPropertyIgnoreAttributes(ext, variable_name, new_value,
-                                               NONE)
-          .Check();
-      return true;
-    }
-  }
-
-  return false;
+  DCHECK(CurrentContext()->IsFunctionContext());
+  return SetContextVariableValue(CurrentScopeInfo(), CurrentContext(),
+                                 variable_name, new_value);
 }
 
-
 bool ScopeIterator::SetScriptVariableValue(Handle<String> variable_name,
                                            Handle<Object> new_value) {
   Handle<Context> context = CurrentContext();
@@ -780,7 +738,6 @@ bool ScopeIterator::SetScriptVariableValue(Handle<String> variable_name,
   return false;
 }
 
-
 bool ScopeIterator::SetCatchVariableValue(Handle<String> variable_name,
                                           Handle<Object> new_value) {
   Handle<Context> context = CurrentContext();