[DevTools] Move collectionEntries to internalProperties in protocol

[DevTools] Move collectionEntries to internalProperties in protocol

BUG=623763
R=dgozman@chromium.org

Committed: https://crrev.com/986d0c88cf902981788863f72ea0c542767e580d
Cr-Commit-Position: refs/heads/master@{#402671}

[kozy, 2016-06-27 23:58:03]

Patchset #2 (id:20001) has been deleted

[kozy, 2016-06-27 23:58:13]

Patchset #1 (id:1) has been deleted

[kozy, 2016-06-27 23:59:48]

Dmitry, please take a look!

[dgozman, 2016-06-28 01:20:47]

I like where this is going.
Can we remove special-handling on frontend right now? We already check for
[[Entries]] when generating preview. Let's move { x => y } there?

https://codereview.chromium.org/2102453003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2102453003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:766:
if (String(name >>> 0) !== name || name >>> 0 === 0xffffffff)
Why do we have this check?

https://codereview.chromium.org/2102453003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js (right):

https://codereview.chromium.org/2102453003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:514: * @param
{boolean=} generatePreview
Let's make it non-optional. Do we have a lot of call sites?

https://codereview.chromium.org/2102453003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:1097: var
properties = this.preview.properties.reduce((map, object) =>
map.set(object.name, this._formatValue(object)), new Map());
Turn it into for-loop please.

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

https://codereview.chromium.org/2102453003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:832: if
(rawInternalProperties[i] !== "[[Entries]]") {
if (.... === entries) {
  ...
  continue;
}

[kozy, 2016-06-28 18:41:50]

All done, please take a look!

https://codereview.chromium.org/2102453003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2102453003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:766:
if (String(name >>> 0) !== name || name >>> 0 === 0xffffffff)
On 2016/06/28 01:20:46, dgozman wrote:
> Why do we have this check?

getProperties return __proto__ and length properties for array of entries, we
need to filter only entries.

https://codereview.chromium.org/2102453003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js (right):

https://codereview.chromium.org/2102453003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:514: * @param
{boolean=} generatePreview
On 2016/06/28 01:20:47, dgozman wrote:
> Let's make it non-optional. Do we have a lot of call sites?

Removed.

https://codereview.chromium.org/2102453003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:1097: var
properties = this.preview.properties.reduce((map, object) =>
map.set(object.name, this._formatValue(object)), new Map());
On 2016/06/28 01:20:47, dgozman wrote:
> Turn it into for-loop please.

Removed.

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

https://codereview.chromium.org/2102453003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:832: if
(rawInternalProperties[i] !== "[[Entries]]") {
On 2016/06/28 01:20:47, dgozman wrote:
> if (.... === entries) {
>   ...
>   continue;
> }

Done.

[dgozman, 2016-06-28 18:58:09]

Let's add a subtype for [[entries]] and clean up more things!

https://codereview.chromium.org/2102453003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2102453003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:766:
if (String(name >>> 0) !== name || name >>> 0 === 0xffffffff)
On 2016/06/28 18:41:49, kozyatinskiy wrote:
> On 2016/06/28 01:20:46, dgozman wrote:
> > Why do we have this check?
> 
> getProperties return __proto__ and length properties for array of entries, we
> need to filter only entries.

Let's have a variable |var isArrayIndex| and check |if (!isArrayIndex)|.

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

https://codereview.chromium.org/2102453003/diff/30017/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:651:
_describeEverything: function(value)
_describeIncludingPrimitives

https://codereview.chromium.org/2102453003/diff/30017/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp (right):

https://codereview.chromium.org/2102453003/diff/30017/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:675: if
(value->IsMap() || value->IsWeakMap() || value->IsSet() || value->IsWeakSet() ||
value->IsSetIterator() || value->IsMapIterator()) {
Let's move if body into collectionEntries.

https://codereview.chromium.org/2102453003/diff/30017/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8InjectedScriptHost.cpp
(right):

https://codereview.chromium.org/2102453003/diff/30017/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8InjectedScriptHost.cpp:139: if
(value->IsObject() &&
value.As<v8::Object>()->HasPrivate(isolate->GetCurrentContext(),
internalEntryPrivate(isolate)).FromMaybe(false)) {
Let's do this before consulting debugger client.

[kozy, 2016-06-28 20:55:21]

Patchset #3 (id:70001) has been deleted

[kozy, 2016-06-28 20:55:46]

All done, please take a look!

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

https://codereview.chromium.org/2102453003/diff/30017/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:651:
_describeEverything: function(value)
On 2016/06/28 18:58:08, dgozman wrote:
> _describeIncludingPrimitives

Done.

https://codereview.chromium.org/2102453003/diff/30017/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp (right):

https://codereview.chromium.org/2102453003/diff/30017/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:675: if
(value->IsMap() || value->IsWeakMap() || value->IsSet() || value->IsWeakSet() ||
value->IsSetIterator() || value->IsMapIterator()) {
On 2016/06/28 18:58:09, dgozman wrote:
> Let's move if body into collectionEntries.

Done.

https://codereview.chromium.org/2102453003/diff/30017/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8InjectedScriptHost.cpp
(right):

https://codereview.chromium.org/2102453003/diff/30017/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8InjectedScriptHost.cpp:139: if
(value->IsObject() &&
value.As<v8::Object>()->HasPrivate(isolate->GetCurrentContext(),
internalEntryPrivate(isolate)).FromMaybe(false)) {
On 2016/06/28 18:58:09, dgozman wrote:
> Let's do this before consulting debugger client.

Done.

[dgozman, 2016-06-28 21:09:11]

lgtm modulo extra proto

https://codereview.chromium.org/2102453003/diff/90001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/console/console-format-es6-expected.txt
(right):

https://codereview.chromium.org/2102453003/diff/90001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-format-es6-expected.txt:79:
__proto__: Array[0]
Can we skip proto for entries? Would be ideal.

[kozy, 2016-06-28 23:57:31]

All done! Woohoo!

https://codereview.chromium.org/2102453003/diff/90001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/console/console-format-es6-expected.txt
(right):

https://codereview.chromium.org/2102453003/diff/90001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-format-es6-expected.txt:79:
__proto__: Array[0]
On 2016/06/28 21:09:11, dgozman wrote:
> Can we skip proto for entries? Would be ideal.

I nullified it before passing.

[kozy, 2016-06-28 23:57:37]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-06-28 23:57:37]

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/2102453003/#ps110001
(title: "")

[commit-bot: I haz the power, 2016-06-28 23:59:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 02:25:37]

Committed patchset #4 (id:110001)

[commit-bot: I haz the power, 2016-06-29 02:28:13]

Description was changed from

==========
[DevTools] Move collectionEntries to internalProperties in protocol

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

to

==========
[DevTools] Move collectionEntries to internalProperties in protocol

BUG=623763
R=dgozman@chromium.org

Committed: https://crrev.com/986d0c88cf902981788863f72ea0c542767e580d
Cr-Commit-Position: refs/heads/master@{#402671}
==========

[commit-bot: I haz the power, 2016-06-29 02:28:14]

Patchset 4 (id:??) landed as
https://crrev.com/986d0c88cf902981788863f72ea0c542767e580d
Cr-Commit-Position: refs/heads/master@{#402671}