[DevTools] Remove functionDetails from protocol.json

[DevTools] Remove functionDetails from protocol.json

BUG=623763
R=dgozman@chromium.org

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

[kozy, 2016-07-07 18:39:04]

Dmitry, please take a look!

[dgozman, 2016-07-07 19:59:24]

Looks promising.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector-protocol/debugger/setScriptSource.html
(right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/debugger/setScriptSource.html:105:
function callbackFunctionDetails(result) {
style: { on next line

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/console/console-dir-expected.txt
(right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-dir-expected.txt:43:
[[FunctionLocation]]: console-dir.html:19
Screenshot of before and after?

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-dir-expected.txt:45:
[[Scopes]]: Scopes[1]
Can there be multiple scopes?

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-dir-expected.txt:61:
[[FunctionLocation]]: (program):97
(program)? Is that like a native function or what? Let's not show internal ones,
since we can't show the location anyway.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/console/console-format-collections-expected.txt
(right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-format-collections-expected.txt:60:
console-format-collections.html:27 [html, head, script, script, script, body, p,
div.c1.c2.c3, form#f, select#sel, option, option, input, input, f: form#f, sel:
select#sel, x: NodeList[2], [[IsGenerator]]: false]
Why this change?

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/function-details-expected.txt
(right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/function-details-expected.txt:74:
scopeChain #1: With Block; __proto__: <no string representation>,e: 7,f: 5,u:
<no value>,v: <no value>
Why did all these change? Is it important?

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/function-details.html
(right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/function-details.html:136:
var properties = internalProperties.reduce((map, object) => map.set(object.name,
object), new Map());
For-loop.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/function-details.html:137:
// InspectorTest.addResult(properties);
Commented code.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/watch-expressions-preserve-expansion-expected.txt
(right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/watch-expressions-preserve-expansion-expected.txt:7:
expanded [[Scopes]] [object Object]
Isn't it an array?

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(left):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:602:
if (internalProperties[i].name === "[[TargetFunction]]") {
Just to check - where did this logic go?

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js (right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:665:
functionDetails: function(remoteObject, callback)
Should we migrate to promise while we are here?

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:692: return;
useless return

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

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:366:
object = object.object;
This needs a justification.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:671: if
(subtype === "internal#scopes")
scopeList

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:675:
return (InjectedScript.closureTypes.get(obj.type) || "Unknown") + (obj.name ?
"(" + obj.name + ")" : "");
maybe space before "(" ?

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

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:672: if
(!scopes->SetPrivate(context,
V8InjectedScriptHost::internalScopesPrivate(m_isolate),
v8::True(m_isolate)).FromMaybe(false))
I think we need an enum now, and helper functions:
class V8InternalValue { kScope, kEntry, kLocation....}
bool markAsInternal(v8::Local<v8::Value>, V8InternalValue)
v8::Maybe<V8InternalValue> internalValueType(v8::Local<v8::Value>)

Or maybe char* constants instead of enum to interoperate with InjectedScript.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:719: if
(!boundFunction->IsFunction() && functionScopes(function).ToLocal(&scopes)) {
Why do we check boundFunction _not_ being a function?

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:724:
properties->Set(properties->Length(), v8::Boolean::New(m_isolate,
function->IsGeneratorFunction()));
This one we can do when disabled.

[kozy, 2016-07-07 22:59:08]

please take another look

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector-protocol/debugger/setScriptSource.html
(right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/debugger/setScriptSource.html:105:
function callbackFunctionDetails(result) {
On 2016/07/07 19:59:23, dgozman wrote:
> style: { on next line

Done.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/console/console-dir-expected.txt
(right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-dir-expected.txt:43:
[[FunctionLocation]]: console-dir.html:19
On 2016/07/07 19:59:23, dgozman wrote:
> Screenshot of before and after?

after:
https://drive.google.com/file/d/0B7RLEApsOKBLY0lnZV9sVnZ3ajQ/view?usp=sharing
before:
https://drive.google.com/file/d/0B7RLEApsOKBLZzlLWWxhSmhuV2s/view?usp=sharing

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-dir-expected.txt:45:
[[Scopes]]: Scopes[1]
On 2016/07/07 19:59:23, dgozman wrote:
> Can there be multiple scopes?

Yes, it can. It will be Scopes[N] array with each scope.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-dir-expected.txt:61:
[[FunctionLocation]]: (program):97
On 2016/07/07 19:59:23, dgozman wrote:
> (program)? Is that like a native function or what? Let's not show internal
ones,
> since we can't show the location anyway.

For some reason V8 reports scriptId, line and column for function that looks
like internal one. Added additional check to ObjectPropertySection.js.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/console/console-format-collections-expected.txt
(right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-format-collections-expected.txt:60:
console-format-collections.html:27 [html, head, script, script, script, body, p,
div.c1.c2.c3, form#f, select#sel, option, option, input, input, f: form#f, sel:
select#sel, x: NodeList[2], [[IsGenerator]]: false]
On 2016/07/07 19:59:23, dgozman wrote:
> Why this change?

because document.all is a function and collection.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/function-details-expected.txt
(right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/function-details-expected.txt:74:
scopeChain #1: With Block; __proto__: <no string representation>,e: 7,f: 5,u:
<no value>,v: <no value>
On 2016/07/07 19:59:23, dgozman wrote:
> Why did all these change? Is it important?

Before we report scope with internal type that starts with lower-case letter,
with this CL we report scope with already formatted scope type.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/function-details.html
(right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/function-details.html:136:
var properties = internalProperties.reduce((map, object) => map.set(object.name,
object), new Map());
On 2016/07/07 19:59:23, dgozman wrote:
> For-loop.

Done.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/function-details.html:137:
// InspectorTest.addResult(properties);
On 2016/07/07 19:59:23, dgozman wrote:
> Commented code.

Done.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/watch-expressions-preserve-expansion-expected.txt
(right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/watch-expressions-preserve-expansion-expected.txt:7:
expanded [[Scopes]] [object Object]
On 2016/07/07 19:59:23, dgozman wrote:
> Isn't it an array?

Array is object and it's part of test addResult output.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(left):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:602:
if (internalProperties[i].name === "[[TargetFunction]]") {
On 2016/07/07 19:59:23, dgozman wrote:
> Just to check - where did this logic go?

scopeCount for bound function is zero in DebuggerScript.js and then we won't
return scopes in internal properties.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js (right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:665:
functionDetails: function(remoteObject, callback)
On 2016/07/07 19:59:23, dgozman wrote:
> Should we migrate to promise while we are here?

Done.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:692: return;
On 2016/07/07 19:59:23, dgozman wrote:
> useless return

Done.

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

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:366:
object = object.object;
On 2016/07/07 19:59:23, dgozman wrote:
> This needs a justification.

Added comment.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:671: if
(subtype === "internal#scopes")
On 2016/07/07 19:59:23, dgozman wrote:
> scopeList

Done.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:675:
return (InjectedScript.closureTypes.get(obj.type) || "Unknown") + (obj.name ?
"(" + obj.name + ")" : "");
On 2016/07/07 19:59:23, dgozman wrote:
> maybe space before "(" ?

Done.

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

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:672: if
(!scopes->SetPrivate(context,
V8InjectedScriptHost::internalScopesPrivate(m_isolate),
v8::True(m_isolate)).FromMaybe(false))
On 2016/07/07 19:59:24, dgozman wrote:
> I think we need an enum now, and helper functions:
> class V8InternalValue { kScope, kEntry, kLocation....}
> bool markAsInternal(v8::Local<v8::Value>, V8InternalValue)
> v8::Maybe<V8InternalValue> internalValueType(v8::Local<v8::Value>)
> 
> Or maybe char* constants instead of enum to interoperate with InjectedScript.

I'm not sure. What are advantages over current approach?

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:719: if
(!boundFunction->IsFunction() && functionScopes(function).ToLocal(&scopes)) {
On 2016/07/07 19:59:24, dgozman wrote:
> Why do we check boundFunction _not_ being a function?

Changed to IsUndefined check.

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:724:
properties->Set(properties->Length(), v8::Boolean::New(m_isolate,
function->IsGeneratorFunction()));
On 2016/07/07 19:59:24, dgozman wrote:
> This one we can do when disabled.

Done.

[dgozman, 2016-07-08 20:37:29]

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/console/console-dir-expected.txt
(right):

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-dir-expected.txt:43:
[[FunctionLocation]]: console-dir.html:19
On 2016/07/07 22:59:07, kozyatinskiy wrote:
> On 2016/07/07 19:59:23, dgozman wrote:
> > Screenshot of before and after?
> 
> after:
> https://drive.google.com/file/d/0B7RLEApsOKBLY0lnZV9sVnZ3ajQ/view?usp=sharing
> before:
> https://drive.google.com/file/d/0B7RLEApsOKBLZzlLWWxhSmhuV2s/view?usp=sharing

- Let's omit IsGenerator when it's false - just noise.
- Let's put IsGenerator before Scopes.
- Can we avoid |length| of Scopes?
- What's the difference between name and [[FunctionName]] ?

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

https://codereview.chromium.org/2122423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:672: if
(!scopes->SetPrivate(context,
V8InjectedScriptHost::internalScopesPrivate(m_isolate),
v8::True(m_isolate)).FromMaybe(false))
On 2016/07/07 22:59:08, kozyatinskiy wrote:
> On 2016/07/07 19:59:24, dgozman wrote:
> > I think we need an enum now, and helper functions:
> > class V8InternalValue { kScope, kEntry, kLocation....}
> > bool markAsInternal(v8::Local<v8::Value>, V8InternalValue)
> > v8::Maybe<V8InternalValue> internalValueType(v8::Local<v8::Value>)
> > 
> > Or maybe char* constants instead of enum to interoperate with
InjectedScript.
> 
> I'm not sure. What are advantages over current approach?

It brings some order to this system.

[kozy, 2016-07-08 23:55:54]

ptal!

[dgozman, 2016-07-09 01:47:29]

https://codereview.chromium.org/2122423002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/console/console-dir-expected.txt
(right):

https://codereview.chromium.org/2122423002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-dir-expected.txt:59:
[[FunctionLocation]]: Object
<unknown>

https://codereview.chromium.org/2122423002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js (right):

https://codereview.chromium.org/2122423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:684: if
(!functionName && response.properties) {
functionName is always null here.

https://codereview.chromium.org/2122423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:697: return {
location: debuggerLocation, sourceURL: null, functionName: functionName ?
functionName.value : "" };
Why null sourceURL? Either remove it or get it from the script.

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

https://codereview.chromium.org/2122423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:682: if
(value->IsFunction()) {
Merge these if block with previous.

[kozy, 2016-07-11 17:48:19]

Patchset #5 (id:80001) has been deleted

[kozy, 2016-07-11 17:50:20]

Patchset #5 (id:100001) has been deleted

[kozy, 2016-07-11 17:50:39]

All done. Please take a look!

https://codereview.chromium.org/2122423002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/console/console-dir-expected.txt
(right):

https://codereview.chromium.org/2122423002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-dir-expected.txt:59:
[[FunctionLocation]]: Object
On 2016/07/09 01:47:29, dgozman wrote:
> <unknown>

Done.

https://codereview.chromium.org/2122423002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js (right):

https://codereview.chromium.org/2122423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:684: if
(!functionName && response.properties) {
On 2016/07/09 01:47:29, dgozman wrote:
> functionName is always null here.

Done.

https://codereview.chromium.org/2122423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:697: return {
location: debuggerLocation, sourceURL: null, functionName: functionName ?
functionName.value : "" };
On 2016/07/09 01:47:29, dgozman wrote:
> Why null sourceURL? Either remove it or get it from the script.

Done.

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

https://codereview.chromium.org/2122423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:682: if
(value->IsFunction()) {
On 2016/07/09 01:47:29, dgozman wrote:
> Merge these if block with previous.

Done.

[dgozman, 2016-07-11 19:18:08]

lgtm

https://codereview.chromium.org/2122423002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2122423002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:1071:
} else {
No need for else and {} because of return.

https://codereview.chromium.org/2122423002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8InternalValueType.cpp
(right):

https://codereview.chromium.org/2122423002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8InternalValueType.cpp:22:
return toV8String(isolate, "internal#entry");
internalized, as we use it many times.

https://codereview.chromium.org/2122423002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8InternalValueType.h
(right):

https://codereview.chromium.org/2122423002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8InternalValueType.h:12: enum
class V8InternalValueType { kNotInternal, kEntry, kLocation, kScope, kScopeList
};
No need for kNotInternal anymore.

https://codereview.chromium.org/2122423002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8InternalValueType.h:14: bool
markAsInternal(v8::Local<v8::Context>, v8::Local<v8::Object>,
V8InternalValueType);
It would be ideal if we can make these V8InternalValueType::{mark, markArray,
fromObject}

[kozy, 2016-07-11 21:20:08]

all done

https://codereview.chromium.org/2122423002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2122423002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:1071:
} else {
On 2016/07/11 19:18:07, dgozman wrote:
> No need for else and {} because of return.

Done.

https://codereview.chromium.org/2122423002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8InternalValueType.cpp
(right):

https://codereview.chromium.org/2122423002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8InternalValueType.cpp:22:
return toV8String(isolate, "internal#entry");
On 2016/07/11 19:18:08, dgozman wrote:
> internalized, as we use it many times.

Done.

https://codereview.chromium.org/2122423002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8InternalValueType.h
(right):

https://codereview.chromium.org/2122423002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8InternalValueType.h:12: enum
class V8InternalValueType { kNotInternal, kEntry, kLocation, kScope, kScopeList
};
On 2016/07/11 19:18:08, dgozman wrote:
> No need for kNotInternal anymore.

Done.

https://codereview.chromium.org/2122423002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8InternalValueType.h:14: bool
markAsInternal(v8::Local<v8::Context>, v8::Local<v8::Object>,
V8InternalValueType);
On 2016/07/11 19:18:08, dgozman wrote:
> It would be ideal if we can make these V8InternalValueType::{mark, markArray,
> fromObject}

Renamed internalValueType to v8InternalValueTypeFrom.

[kozy, 2016-07-11 21:20:39]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-07-11 21:20:40]

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/2122423002/#ps140001
(title: "a")

[commit-bot: I haz the power, 2016-07-11 21:21:34]

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-07-11 21:33:08]

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

[commit-bot: I haz the power, 2016-07-11 21:33:09]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kozy, 2016-07-11 23:26:24]

kozyatinskiy@chromium.org changed reviewers:
+ pfeldman@chromium.org

[kozy, 2016-07-11 23:26:25]

Pavel, I need owner lgtm for core.gyp. Please take a look!

[pfeldman, 2016-07-11 23:37:35]

lgtm

[kozy, 2016-07-12 00:16:07]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-07-12 00:17:24]

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-07-12 03:17:05]

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

[commit-bot: I haz the power, 2016-07-12 03:17:06]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kozy, 2016-07-12 03:27:24]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-07-12 03:27:58]

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-07-12 05:45:42]

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

[commit-bot: I haz the power, 2016-07-12 05:45:43]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kozy, 2016-07-12 06:15:44]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-07-12 06:16:25]

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-07-12 07:56:03]

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

[commit-bot: I haz the power, 2016-07-12 07:56:04]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kozy, 2016-07-12 15:08:34]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-07-12 15:09:23]

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-07-12 19:18:30]

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

[commit-bot: I haz the power, 2016-07-12 19:18:31]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kozy, 2016-07-12 19:24:14]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-07-12 19:26:18]

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-07-12 23:27:26]

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

[commit-bot: I haz the power, 2016-07-12 23:27:27]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kozy, 2016-07-12 23:44:21]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-07-12 23:47:20]

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-07-13 02:45:38]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-07-13 02:45:53]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 02:48:04]

Description was changed from

==========
[DevTools] Remove functionDetails from protocol.json

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

to

==========
[DevTools] Remove functionDetails from protocol.json

BUG=623763
R=dgozman@chromium.org

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

[commit-bot: I haz the power, 2016-07-13 02:48:05]

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

[pfeldman, 2016-08-10 16:02:09]

https://codereview.chromium.org/2122423002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js
(right):

https://codereview.chromium.org/2122423002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:206:
InjectedScript.closureTypes = new Map([
You redefine Object and push for safety, but introduce Map here...

[dgozman, 2016-08-10 16:17:51]

https://codereview.chromium.org/2122423002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js
(right):

https://codereview.chromium.org/2122423002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:206:
InjectedScript.closureTypes = new Map([
On 2016/08/10 16:02:09, pfeldman wrote:
> You redefine Object and push for safety, but introduce Map here...

Good catch!

[pfeldman, 2016-08-10 16:19:46]

I wish it was a catch :-(

On Wed, Aug 10, 2016, 09:17 <dgozman@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2122423002/diff/140001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js
> (right):
>
>
>
https://codereview.chromium.org/2122423002/diff/140001/third_party/WebKit/Sou...
>
> third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:206:
> InjectedScript.closureTypes = new Map([
> On 2016/08/10 16:02:09, pfeldman wrote:
> > You redefine Object and push for safety, but introduce Map here...
>
> Good catch!
>
> https://codereview.chromium.org/2122423002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[pfeldman, 2016-08-10 16:19:47]

I wish it was a catch :-(

On Wed, Aug 10, 2016, 09:17 <dgozman@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2122423002/diff/140001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js
> (right):
>
>
>
https://codereview.chromium.org/2122423002/diff/140001/third_party/WebKit/Sou...
>
> third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:206:
> InjectedScript.closureTypes = new Map([
> On 2016/08/10 16:02:09, pfeldman wrote:
> > You redefine Object and push for safety, but introduce Map here...
>
> Good catch!
>
> https://codereview.chromium.org/2122423002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.