[DevTools] Move getCollectionEntries to native

third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "platform/v8_inspector/V8DebuggerAgentImpl.h"
 
 #include "platform/inspector_protocol/String16.h"
 #include "platform/inspector_protocol/Values.h"
 #include "platform/v8_inspector/IgnoreExceptionsScope.h"
 #include "platform/v8_inspector/InjectedScript.h"
@@ skipping 125 matching lines @@
 
     for (size_t i = 0; i < hashesSize; ++i)
         hashes[i] = (hashes[i] + zi[i] * (prime[i] - 1)) % prime[i];
 
     String16Builder hash;
     for (size_t i = 0; i < hashesSize; ++i)
         appendUnsignedAsHex(hashes[i], &hash);
     return hash.toString();
 }
 
+static bool checkInternalError(ErrorString* errorString, bool success)

[dgozman, 2016/03/19 05:39:21]

I saw this one already.

[kozy, 2016/03/21 05:18:28]

I've moved it to V8RuntimeAgentImpl. In followup patch I will move it to more
appropriate place.

+{
+    if (!success)
+        *errorString = "Internal error";
+    return success;
+}
+
 PassOwnPtr<V8DebuggerAgent> V8DebuggerAgent::create(V8RuntimeAgent* runtimeAgent)
 {
     V8RuntimeAgentImpl* runtimeAgentImpl = static_cast<V8RuntimeAgentImpl*>(runtimeAgent);
     return adoptPtr(new V8DebuggerAgentImpl(runtimeAgentImpl->getInjectedScriptManager(), runtimeAgentImpl->debugger(), runtimeAgentImpl->contextGroupId()));
 }
 
 V8DebuggerAgentImpl::V8DebuggerAgentImpl(InjectedScriptManager* injectedScriptManager, V8DebuggerImpl* debugger, int contextGroupId)
     : m_injectedScriptManager(injectedScriptManager)
     , m_debugger(debugger)
     , m_contextGroupId(contextGroupId)
@@ skipping 545 matching lines @@
     }
     v8::Local<v8::Function> function = value.As<v8::Function>();
 
     v8::Local<v8::Value> scopesValue;
     v8::Local<v8::Array> scopes;
     String16 objectGroupName = injectedScript->objectGroupName(*remoteId);
     if (m_debugger->functionScopes(function).ToLocal(&scopesValue) && scopesValue->IsArray()) {
         scopes = scopesValue.As<v8::Array>();
         for (size_t i = 0; i < scopes->Length(); ++i) {
             v8::Local<v8::Value> scope;
-            if (!scopes->Get(injectedScript->context(), i).ToLocal(&scope) || !scope->IsObject()) {
-                *errorString = "Internal error";
+            if (!checkInternalError(errorString, scopes->Get(injectedScript->context(), i).ToLocal(&scope) && scope->IsObject()))
                 return;
-            }
             if (!injectedScript->wrapObjectProperty(errorString, scope.As<v8::Object>(), toV8String(injectedScript->isolate(), "object"), objectGroupName))
                 return;
         }
     }
 
     OwnPtr<protocol::Debugger::Location> location = protocol::Debugger::Location::create()
         .setScriptId(String16::number(function->ScriptId()))
         .setLineNumber(function->GetScriptLineNumber())
         .setColumnNumber(function->GetScriptColumnNumber()).build();
 
     OwnPtr<FunctionDetails> functionDetails = FunctionDetails::create()
         .setLocation(location.release())
         .setFunctionName(toProtocolStringWithTypeCheck(function->GetDebugName()))
         .setIsGenerator(function->IsGeneratorFunction()).build();
 
     if (!scopes.IsEmpty()) {
         protocol::ErrorSupport errorSupport;
         OwnPtr<protocol::Array<protocol::Debugger::Scope>> scopeChain = protocol::Array<protocol::Debugger::Scope>::parse(toProtocolValue(injectedScript->context(), scopes).get(), &errorSupport);
-        if (errorSupport.hasErrors()) {
-            *errorString = "Internal error";
+        if (!checkInternalError(errorString, !errorSupport.hasErrors()))

[dgozman, 2016/03/19 05:39:21]

Perhaps we should rename this to hasInternalError to avoid negating in every
call.

[kozy, 2016/03/21 05:18:28]

Done.

             return;
-        }
         functionDetails->setScopeChain(scopeChain.release());
     }
 
     *details = functionDetails.release();
 }
 
 void V8DebuggerAgentImpl::getGeneratorObjectDetails(ErrorString* errorString, const String16& objectId, OwnPtr<GeneratorObjectDetails>* outDetails)
 {
     if (!checkEnabled(errorString))
         return;
@@ skipping 12 matching lines @@
     v8::Local<v8::Value> value;
     if (!injectedScript->findObject(errorString, *remoteId, &value))
         return;
     if (!value->IsObject() || !value->ToObject(context).ToLocal(&object)) {
         *errorString = "Value with given id is not an Object";
         return;
     }
 
     v8::Local<v8::Object> detailsObject;
     v8::Local<v8::Value> detailsValue = debugger().generatorObjectDetails(object);
-    if (!detailsValue->IsObject() || !detailsValue->ToObject(context).ToLocal(&detailsObject)) {
-        *errorString = "Internal error";
+    if (!checkInternalError(errorString, detailsValue->IsObject() && detailsValue->ToObject(context).ToLocal(&detailsObject)))
         return;
-    }
 
     String16 groupName = injectedScript->objectGroupName(*remoteId);
     if (!injectedScript->wrapObjectProperty(errorString, detailsObject, toV8String(m_isolate, "function"), groupName))
         return;
 
     protocol::ErrorSupport errors;
     OwnPtr<GeneratorObjectDetails> protocolDetails = GeneratorObjectDetails::parse(toProtocolValue(context, detailsObject).get(), &errors);
-    if (!protocolDetails) {
-        *errorString = "Internal error";
+    if (!checkInternalError(errorString, protocolDetails))
         return;
-    }
     *outDetails = protocolDetails.release();
 }
 
-void V8DebuggerAgentImpl::getCollectionEntries(ErrorString* errorString, const String16& objectId, OwnPtr<protocol::Array<CollectionEntry>>* entries)
+void V8DebuggerAgentImpl::getCollectionEntries(ErrorString* errorString, const String16& objectId, OwnPtr<protocol::Array<CollectionEntry>>* outEntries)
 {
     if (!checkEnabled(errorString))
         return;
     OwnPtr<RemoteObjectId> remoteId = RemoteObjectId::parse(errorString, objectId);
     if (!remoteId)
         return;
     InjectedScript* injectedScript = m_injectedScriptManager->findInjectedScript(errorString, remoteId.get());
     if (!injectedScript)
         return;
-    injectedScript->getCollectionEntries(errorString, objectId, entries);
+
+    v8::HandleScope scope(m_isolate);
+    v8::Local<v8::Context> context = injectedScript->context();
+    v8::Context::Scope contextScope(context);
+
+    v8::Local<v8::Object> object;
+    v8::Local<v8::Value> value;
+    if (!injectedScript->findObject(errorString, *remoteId, &value))
+        return;
+    if (!value->IsObject() || !value->ToObject(context).ToLocal(&object)) {

[dgozman, 2016/03/19 05:39:21]

Use value.As<v8::Object>

[kozy, 2016/03/21 05:18:28]

Done.

+        *errorString = "Object with given id is not a collection";
+        return;
+    }
+    v8::Local<v8::Value> entriesValue = m_debugger->collectionEntries(object);
+    if (!checkInternalError(errorString, !entriesValue.IsEmpty()))
+        return;
+    if (entriesValue->IsUndefined()) {
+        *errorString = "Object with given id is not a collection";
+        return;
+    }
+    if (!checkInternalError(errorString, entriesValue->IsArray()))
+        return;
+    String16 groupName = injectedScript->objectGroupName(*remoteId);
+    v8::Local<v8::Array> entriesArray = entriesValue.As<v8::Array>();
+    for (size_t i = 0; i < entriesArray->Length(); ++i) {
+        v8::Local<v8::Value> entry;
+        if (!checkInternalError(errorString, entriesArray->Get(injectedScript->context(), i).ToLocal(&entry)) && entry->IsObject())

[dgozman, 2016/03/19 05:39:21]

entry->IsObject() should be inside checkInternalError

[kozy, 2016/03/21 05:18:28]

Removed.

+            return;
+        v8::Local<v8::Object> entryObject = entry.As<v8::Object>();
+        v8::Local<v8::String> keyString = toV8String(injectedScript->isolate(), "key");

[dgozman, 2016/03/19 05:39:21]

toV8StringInternalized

I think there are more literals in our code which should be internalized.

[kozy, 2016/03/21 05:18:28]

Done.

+        if (entryObject->Has(injectedScript->context(), keyString).FromMaybe(false)) {
+            if (!injectedScript->wrapObjectProperty(errorString, entryObject, keyString, groupName))
+                return;
+        }
+        if (!injectedScript->wrapObjectProperty(errorString, entryObject, toV8String(injectedScript->isolate(), "value"), groupName))

[dgozman, 2016/03/19 05:39:21]

I'm worried that many calls into V8 like this could introduce a performance
regression. Previously, there were O(1) calls. Can we measure?

[kozy, 2016/03/21 05:18:28]

I compare perfomance of different options and introduced helper method for
wrapping all objects in array by one call:
https://docs.google.com/spreadsheets/d/1BytWeVgEunDLW5fO63l8xkHr8q6iWu4on2tuS...
Compare between old implementations and implementations from second patch set is
highlighted. With small amount of collection entries new implementation is
faster. In case of 10 elements is twice more faster. In case of collections with
1000 and more entries new implementation is slower 3-7%. It doesn't look
critical.

+            return;
+    }
+    protocol::ErrorSupport errors;
+    OwnPtr<protocol::Array<CollectionEntry>> entries = protocol::Array<CollectionEntry>::parse(toProtocolValue(injectedScript->context(), entriesArray).get(), &errors);
+    if (!checkInternalError(errorString, entries))

[dgozman, 2016/03/19 05:39:21]

I'd rather use !errors.hasErrors()

[kozy, 2016/03/21 05:18:28]

I'd like to check pointer because null pointer will produce crash while follow
answer processing.

+        return;
+    *outEntries = entries.release();
 }
 
 void V8DebuggerAgentImpl::schedulePauseOnNextStatement(const String16& breakReason, PassOwnPtr<protocol::DictionaryValue> data)
 {
     ASSERT(enabled());
     if (m_scheduledDebuggerStep == StepInto || m_javaScriptPauseScheduled || isPaused())
         return;
     m_breakReason = breakReason;
     m_breakAuxData = data;
     m_pausingOnNativeEvent = true;
@@ skipping 816 matching lines @@
     m_scripts.clear();
     m_blackboxedPositions.clear();
     m_breakpointIdToDebuggerBreakpointIds.clear();
     resetAsyncCallTracker();
     m_promiseTracker->clear();
     if (m_frontend)
         m_frontend->globalObjectCleared();
 }
 
 } // namespace blink