[DevTools] Move getCollectionEntries to native

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

Index: third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
diff --git a/third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp b/third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
index 5f7ee2bde1564c580bd5d0eefd9fd349b2491cc5..9d459314a02907a86d823cba7f2e2d698089751e 100644
--- a/third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
+++ b/third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
@@ -143,6 +143,13 @@ static String16 calculateHash(const String16& str)
     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);
@@ -708,10 +715,8 @@ void V8DebuggerAgentImpl::getFunctionDetails(ErrorString* errorString, const Str
         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;
         }
@@ -730,10 +735,8 @@ void V8DebuggerAgentImpl::getFunctionDetails(ErrorString* errorString, const Str
     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());
     }
 
@@ -766,10 +769,8 @@ void V8DebuggerAgentImpl::getGeneratorObjectDetails(ErrorString* errorString, co
 
     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))
@@ -777,14 +778,12 @@ void V8DebuggerAgentImpl::getGeneratorObjectDetails(ErrorString* errorString, co
 
     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;
@@ -794,7 +793,48 @@ void V8DebuggerAgentImpl::getCollectionEntries(ErrorString* errorString, const S
     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)