[DevTools] Move getCollectionEntries to native

[DevTools] Move getCollectionEntries to native

BUG=595206
R=dgozman@chromium.org

Committed: https://crrev.com/5ff5540ad9862e23d7783972c584772668e43adc
Cr-Commit-Position: refs/heads/master@{#382456}

[kozy, 2016-03-19 01:49:25]

Dmitry, please take a look!

[dgozman, 2016-03-19 05:39:22]

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
(right):

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:146:
static bool checkInternalError(ErrorString* errorString, bool success)
I saw this one already.

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:738: if
(!checkInternalError(errorString, !errorSupport.hasErrors()))
Perhaps we should rename this to hasInternalError to avoid negating in every
call.

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:805: if
(!value->IsObject() || !value->ToObject(context).ToLocal(&object)) {
Use value.As<v8::Object>

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:822: if
(!checkInternalError(errorString, entriesArray->Get(injectedScript->context(),
i).ToLocal(&entry)) && entry->IsObject())
entry->IsObject() should be inside checkInternalError

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:825:
v8::Local<v8::String> keyString = toV8String(injectedScript->isolate(), "key");
toV8StringInternalized

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

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:830: if
(!injectedScript->wrapObjectProperty(errorString, entryObject,
toV8String(injectedScript->isolate(), "value"), groupName))
I'm worried that many calls into V8 like this could introduce a performance
regression. Previously, there were O(1) calls. Can we measure?

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:835: if
(!checkInternalError(errorString, entries))
I'd rather use !errors.hasErrors()

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

All done. Link to perf comparison is added. Please take a look.

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
(right):

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:146:
static bool checkInternalError(ErrorString* errorString, bool success)
On 2016/03/19 05:39:21, dgozman wrote:
> I saw this one already.

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

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:738: if
(!checkInternalError(errorString, !errorSupport.hasErrors()))
On 2016/03/19 05:39:21, dgozman wrote:
> Perhaps we should rename this to hasInternalError to avoid negating in every
> call.

Done.

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:805: if
(!value->IsObject() || !value->ToObject(context).ToLocal(&object)) {
On 2016/03/19 05:39:21, dgozman wrote:
> Use value.As<v8::Object>

Done.

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:822: if
(!checkInternalError(errorString, entriesArray->Get(injectedScript->context(),
i).ToLocal(&entry)) && entry->IsObject())
On 2016/03/19 05:39:21, dgozman wrote:
> entry->IsObject() should be inside checkInternalError

Removed.

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:825:
v8::Local<v8::String> keyString = toV8String(injectedScript->isolate(), "key");
On 2016/03/19 05:39:21, dgozman wrote:
> toV8StringInternalized
> 
> I think there are more literals in our code which should be internalized.

Done.

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:830: if
(!injectedScript->wrapObjectProperty(errorString, entryObject,
toV8String(injectedScript->isolate(), "value"), groupName))
On 2016/03/19 05:39:21, dgozman wrote:
> I'm worried that many calls into V8 like this could introduce a performance
> regression. Previously, there were O(1) calls. Can we measure?

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.

https://codereview.chromium.org/1810283003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:835: if
(!checkInternalError(errorString, entries))
On 2016/03/19 05:39:21, dgozman wrote:
> I'd rather use !errors.hasErrors()

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

[kozy, 2016-03-21 16:16:31]

Patchset #2 (id:20001) has been deleted

[dgozman, 2016-03-21 17:21:02]

lgtm

https://codereview.chromium.org/1810283003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/InjectedScript.h (right):

https://codereview.chromium.org/1810283003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScript.h:63: bool
wrapObjectsInArray(ErrorString*, v8::Local<v8::Array>, v8::Local<v8::Value>
property, const String16& groupName, bool forceValueType = false, bool
generatePreview = false) const;
wrapPropertyInArray ?

https://codereview.chromium.org/1810283003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScript.h:63: bool
wrapObjectsInArray(ErrorString*, v8::Local<v8::Array>, v8::Local<v8::Value>
property, const String16& groupName, bool forceValueType = false, bool
generatePreview = false) const;
v8::Local<v8::String> property

https://codereview.chromium.org/1810283003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js
(right):

https://codereview.chromium.org/1810283003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:314: if
(property in array[i])
typeof array[i] === "object"

https://codereview.chromium.org/1810283003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
(right):

https://codereview.chromium.org/1810283003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:725: if
(V8RuntimeAgentImpl::hasInternalError(errorString, errorSupport.hasErrors()))
Let's have a copy per file.

[kozy, 2016-03-21 19:45:01]

All done!

https://codereview.chromium.org/1810283003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/InjectedScript.h (right):

https://codereview.chromium.org/1810283003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScript.h:63: bool
wrapObjectsInArray(ErrorString*, v8::Local<v8::Array>, v8::Local<v8::Value>
property, const String16& groupName, bool forceValueType = false, bool
generatePreview = false) const;
On 2016/03/21 17:21:02, dgozman wrote:
> wrapPropertyInArray ?

Done.

https://codereview.chromium.org/1810283003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScript.h:63: bool
wrapObjectsInArray(ErrorString*, v8::Local<v8::Array>, v8::Local<v8::Value>
property, const String16& groupName, bool forceValueType = false, bool
generatePreview = false) const;
On 2016/03/21 17:21:02, dgozman wrote:
> v8::Local<v8::String> property

Done.

https://codereview.chromium.org/1810283003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js
(right):

https://codereview.chromium.org/1810283003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:314: if
(property in array[i])
On 2016/03/21 17:21:02, dgozman wrote:
> typeof array[i] === "object"

Done.

https://codereview.chromium.org/1810283003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
(right):

https://codereview.chromium.org/1810283003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:725: if
(V8RuntimeAgentImpl::hasInternalError(errorString, errorSupport.hasErrors()))
On 2016/03/21 17:21:02, dgozman wrote:
> Let's have a copy per file.

Done.

[kozy, 2016-03-21 19:45:23]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-03-21 19:45:23]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1810283003/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-03-21 19:45:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1810283003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1810283003/60001

[commit-bot: I haz the power, 2016-03-21 20:32:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-21 20:32:28]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kozy, 2016-03-21 23:08:39]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-03-21 23:09:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1810283003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1810283003/60001

[commit-bot: I haz the power, 2016-03-22 00:27:20]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-03-22 00:28:53]

Description was changed from

==========
[DevTools] Move getCollectionEntries to native

BUG=595206
R=dgozman@chromium.org
==========

to

==========
[DevTools] Move getCollectionEntries to native

BUG=595206
R=dgozman@chromium.org

Committed: https://crrev.com/5ff5540ad9862e23d7783972c584772668e43adc
Cr-Commit-Position: refs/heads/master@{#382456}
==========

[commit-bot: I haz the power, 2016-03-22 00:28:54]

Patchset 3 (id:??) landed as
https://crrev.com/5ff5540ad9862e23d7783972c584772668e43adc
Cr-Commit-Position: refs/heads/master@{#382456}