[DevTools] Move Console to v8_inspector

[DevTools] Move Console to v8_inspector

This is a first patch in the effort of moving console to v8_inspector:
- move Console object;
- move ConsoleMessage;
- move ConsoleMessageStorage.

The benefits are:
- moves the last v8-related inspector aspect to v8_inspector;
- simplifies bindings between console, profiler and timeline;
- reduces console API for the embedder, which mostly uses it as a log storage.

As a side effect, this patch makes Console object functions automatically bound to Console:
var log = console.log; log(42);

BUG=167911, 580337
R=dgozman@chromium.org,jochen@chromium.org

Committed: https://crrev.com/807ec9550e8a31517966636e6a5b506474ab4ea9
Cr-Commit-Position: refs/heads/master@{#388788}

[kozy, 2016-04-06 01:23:38]

Description was changed from

==========
[DevTools] var log = console.log; log(42);

Console functions works without console object as receiver.

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

to

==========
[DevTools] console functions can be called without console

Console functions works without console object as receiver.
var log = console.log; log(42);

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

[kozy, 2016-04-06 01:24:24]

Dmitry, please take a look!

[dgozman, 2016-04-06 15:51:33]

https://codereview.chromium.org/1859293002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector/console/console-log-without-console-expected.txt
(right):

https://codereview.chromium.org/1859293002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector/console/console-log-without-console-expected.txt:9:
console-log-without-console.html:11 3(anonymous function) @
console-log-without-console.html:11
Why two locations here?

https://codereview.chromium.org/1859293002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/custom/V8ConsoleBaseCustom.cpp
(right):

https://codereview.chromium.org/1859293002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/custom/V8ConsoleBaseCustom.cpp:56:
info.GetReturnValue().Set(v8::Function::New(info.GetIsolate(), debugFunction,
info.Holder()));
Is it fine to create new function for each call? What about performance?

https://codereview.chromium.org/1859293002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ConsoleBase.idl (right):

https://codereview.chromium.org/1859293002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ConsoleBase.idl:37: [Custom, Replaceable]
readonly attribute object debug;
Should not be readonly.

https://codereview.chromium.org/1859293002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ConsoleBase.idl:40: [Custom, Replaceable]
readonly attribute object log;
Why object?

[kozy, 2016-04-06 17:45:21]

Addressed comments.

https://codereview.chromium.org/1859293002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector/console/console-log-without-console-expected.txt
(right):

https://codereview.chromium.org/1859293002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector/console/console-log-without-console-expected.txt:9:
console-log-without-console.html:11 3(anonymous function) @
console-log-without-console.html:11
On 2016/04/06 15:51:33, dgozman wrote:
> Why two locations here?

It's stack trace for error message.

https://codereview.chromium.org/1859293002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/custom/V8ConsoleBaseCustom.cpp
(right):

https://codereview.chromium.org/1859293002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/custom/V8ConsoleBaseCustom.cpp:56:
info.GetReturnValue().Set(v8::Function::New(info.GetIsolate(), debugFunction,
info.Holder()));
On 2016/04/06 15:51:33, dgozman wrote:
> Is it fine to create new function for each call? What about performance?

I've compared perfomance:
https://docs.google.com/a/google.com/spreadsheets/d/1gsjs5ql7HpAAQtCsMLMkBjbF...
New implementation is slower for ~12% when console.log is called directly and
faster for 12% when console.log is stored to variable and called then.

https://codereview.chromium.org/1859293002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ConsoleBase.idl (right):

https://codereview.chromium.org/1859293002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ConsoleBase.idl:37: [Custom, Replaceable]
readonly attribute object debug;
On 2016/04/06 15:51:33, dgozman wrote:
> Should not be readonly.

It's replaceable - users can replace this property and I don't want to describe
custom setter for this.

https://codereview.chromium.org/1859293002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ConsoleBase.idl:40: [Custom, Replaceable]
readonly attribute object log;
On 2016/04/06 15:51:33, dgozman wrote:
> Why object?

There is no function type in WebIDL spec so function is object.

[kozy, 2016-04-07 00:55:47]

Description was changed from

==========
[DevTools] console functions can be called without console

Console functions works without console object as receiver.
var log = console.log; log(42);

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

to

==========
[DevTools] Move Console to v8_inspector

Console functions works without console object as receiver.
var log = console.log; log(42);

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

[kozy, 2016-04-07 00:56:39]

I've uploaded proof of concept. All tests are passed. Performance is the same as
previous implementation.

[kozy, 2016-04-07 23:39:34]

Ready for review. Please take a look!

[kozy, 2016-04-08 00:05:02]

Patchset #3 (id:40001) has been deleted

[dgozman, 2016-04-08 18:18:46]

General direction is good.
Let's make sure we have prototype with methods, similar to Firefox.
What about intent to implement and ship for moving Console away from
ConsoleBase? It's user-observed change in prototype chain.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/Window/window-lookup-precedence-expected.txt
(right):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/window-lookup-precedence-expected.txt:307:
PASS win['console'] == '[object Object]' is true
Can we set a constructor name?

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/tracing/console-timeline-expected.txt
(left):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/console-timeline-expected.txt:1:
CONSOLE WARNING: 'console.timeline' is deprecated. Please use 'console.time'
instead.
Let's add a test which shows "the method was removed" console warning.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/tracing/timeline-mark-timeline-expected.txt
(left):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/timeline-mark-timeline-expected.txt:1:
CONSOLE WARNING: 'console.markTimeline' is deprecated. Please use
'console.timeStamp' instead.
ditto

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/tracing/timeline-timer-fired-from-eval-call-site-expected.txt
(right):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/timeline-timer-fired-from-eval-call-site-expected.txt:1:
CONSOLE WARNING: line 1: console.markTimeline method was removed.
Please revert the warning message.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/MainThreadDebugger.h (right):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/MainThreadDebugger.h:71: bool
hasMemoryOnConsole(v8::Local<v8::Context>) override;
This bit should be a part of V8ContextInfo, which embedded reports on context
creation.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/MainThreadDebugger.h:72:
v8::MaybeLocal<v8::Value> memoryInfo(v8::Isolate*, v8::Local<v8::Context>,
v8::Local<v8::Object> creationContext) override;
We can probably do this directly by asking V8.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/MainThreadDebugger.h:74: void
reportMessageToConsole(v8::Local<v8::Context>, ConsoleMessage*);
override?

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp (right):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:73: ScriptState*
scriptState = ScriptState::current(context->GetIsolate());
ScriptState::from(context) or something.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:114:
InspectorInstrumentation::consoleProfile(toExecutionContext(context), title);
This could be done entirely in v8_inspector. Remove instrumentation and methods
from InspectorProfilerAgent.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/ThreadDebugger.h (right):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/ThreadDebugger.h:17: class
ConsoleMessage;
Remove.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/ThreadDebugger.h:39: void
time(v8::Isolate*, const String16& title) override;
consoleTime

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/ThreadDebugger.h:40: void
timeEnd(v8::Isolate*, const String16& title) override;
consoleTimeEnd

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/ThreadDebugger.h:41: void
timeStamp(v8::Isolate*, const String16& title) override;
consoleTimeStamp

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/ConsoleMemory.idl (left):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/ConsoleMemory.idl:9: // We can't make the
attribute readonly as it breaks existing code that relies on being able to
assign to console.memory in strict mode. Instead, the setter just ignores the
passed value.  http://crbug.com/468611
Let's keep this behavior and comment. Can we add a test as well?

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/inspector_protocol/String16WTF.h
(right):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/inspector_protocol/String16WTF.h:42: static
String16 fromDouble(double number, int precision = -1) { return precision == -1
? Decimal::fromDouble(number).toString() :
String::numberToStringFixedWidth(number, precision); }
Let's do a separate method fromDoubleFixedWidth.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp (right):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:35: if
(!value->IsExternal())
ASSERT

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:41: void
internalAddMessage(MessageType type, MessageLevel level, const
v8::FunctionCallbackInfo<v8::Value>& info, bool allowEmptyArguments, int
firstArg)
skipArgumentCount

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:62: return
obj->CreateDataProperty(context, funcName, func).FromMaybe(false);
ASSERT

[kozy, 2016-04-08 23:56:51]

All done. Please take a look!

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/Window/window-lookup-precedence-expected.txt
(right):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/window-lookup-precedence-expected.txt:307:
PASS win['console'] == '[object Object]' is true
On 2016/04/08 18:18:46, dgozman wrote:
> Can we set a constructor name?

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/tracing/console-timeline-expected.txt
(left):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/console-timeline-expected.txt:1:
CONSOLE WARNING: 'console.timeline' is deprecated. Please use 'console.time'
instead.
On 2016/04/08 18:18:45, dgozman wrote:
> Let's add a test which shows "the method was removed" console warning.

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/tracing/timeline-mark-timeline-expected.txt
(left):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/timeline-mark-timeline-expected.txt:1:
CONSOLE WARNING: 'console.markTimeline' is deprecated. Please use
'console.timeStamp' instead.
On 2016/04/08 18:18:45, dgozman wrote:
> ditto

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/tracing/timeline-timer-fired-from-eval-call-site-expected.txt
(right):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/timeline-timer-fired-from-eval-call-site-expected.txt:1:
CONSOLE WARNING: line 1: console.markTimeline method was removed.
On 2016/04/08 18:18:46, dgozman wrote:
> Please revert the warning message.

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/MainThreadDebugger.h (right):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/MainThreadDebugger.h:71: bool
hasMemoryOnConsole(v8::Local<v8::Context>) override;
On 2016/04/08 18:18:46, dgozman wrote:
> This bit should be a part of V8ContextInfo, which embedded reports on context
> creation.

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/MainThreadDebugger.h:72:
v8::MaybeLocal<v8::Value> memoryInfo(v8::Isolate*, v8::Local<v8::Context>,
v8::Local<v8::Object> creationContext) override;
On 2016/04/08 18:18:46, dgozman wrote:
> We can probably do this directly by asking V8.

blink implementation is using a lot of core related stuff like UserMetrics,
probably we can move MemoryInfo to v8_inspector later.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/MainThreadDebugger.h:74: void
reportMessageToConsole(v8::Local<v8::Context>, ConsoleMessage*);
On 2016/04/08 18:18:46, dgozman wrote:
> override?

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp (right):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:73: ScriptState*
scriptState = ScriptState::current(context->GetIsolate());
On 2016/04/08 18:18:46, dgozman wrote:
> ScriptState::from(context) or something.

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:114:
InspectorInstrumentation::consoleProfile(toExecutionContext(context), title);
On 2016/04/08 18:18:46, dgozman wrote:
> This could be done entirely in v8_inspector. Remove instrumentation and
methods
> from InspectorProfilerAgent.

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/ThreadDebugger.h (right):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/ThreadDebugger.h:17: class
ConsoleMessage;
On 2016/04/08 18:18:46, dgozman wrote:
> Remove.

It is used by reportMessageToConsole method.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/ThreadDebugger.h:39: void
time(v8::Isolate*, const String16& title) override;
On 2016/04/08 18:18:46, dgozman wrote:
> consoleTime

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/ThreadDebugger.h:40: void
timeEnd(v8::Isolate*, const String16& title) override;
On 2016/04/08 18:18:46, dgozman wrote:
> consoleTimeEnd

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/ThreadDebugger.h:41: void
timeStamp(v8::Isolate*, const String16& title) override;
On 2016/04/08 18:18:46, dgozman wrote:
> consoleTimeStamp

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/ConsoleMemory.idl (left):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/ConsoleMemory.idl:9: // We can't make the
attribute readonly as it breaks existing code that relies on being able to
assign to console.memory in strict mode. Instead, the setter just ignores the
passed value.  http://crbug.com/468611
On 2016/04/08 18:18:46, dgozman wrote:
> Let's keep this behavior and comment. Can we add a test as well?

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/inspector_protocol/String16WTF.h
(right):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/inspector_protocol/String16WTF.h:42: static
String16 fromDouble(double number, int precision = -1) { return precision == -1
? Decimal::fromDouble(number).toString() :
String::numberToStringFixedWidth(number, precision); }
On 2016/04/08 18:18:46, dgozman wrote:
> Let's do a separate method fromDoubleFixedWidth.

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp (right):

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:35: if
(!value->IsExternal())
On 2016/04/08 18:18:46, dgozman wrote:
> ASSERT

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:41: void
internalAddMessage(MessageType type, MessageLevel level, const
v8::FunctionCallbackInfo<v8::Value>& info, bool allowEmptyArguments, int
firstArg)
On 2016/04/08 18:18:46, dgozman wrote:
> skipArgumentCount

Done.

https://codereview.chromium.org/1859293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:62: return
obj->CreateDataProperty(context, funcName, func).FromMaybe(false);
On 2016/04/08 18:18:46, dgozman wrote:
> ASSERT

Done.

[dgozman, 2016-04-12 03:32:16]

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/tracing/timeline-mark-timeline-expected.txt
(left):

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/tracing/timeline-mark-timeline-expected.txt:4:
TimeStamp Properties:
Let's not introduce behavior changes in this patch except for planned ones.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right):

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:108:
debugger()->contextCreated(V8ContextInfo(scriptState->context(),
contextGroupId(frame), world.isMainWorld(), origin ? origin->toRawString() : "",
world.isIsolatedWorld() ? world.isolatedWorldHumanReadableName() : "",
IdentifiersFactory::frameId(frame), executionContext->isDocument()));
Inline it!

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:234: return
executionContext->isDocument() ? toV8(MemoryInfo::create(), creationContext,
isolate) : v8::MaybeLocal<v8::Value>();
ASSERT(executionContext->isDocument())

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp (right):

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:92:
consoleMessage->setCallStack(ScriptCallStack::capture(1));
consoleMessage->setScriptState(scriptState);

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:98:
TRACE_EVENT_COPY_ASYNC_BEGIN0("blink.console", ((String)title).utf8().data(),
this);
String(title).utf8().data()

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:103:
TRACE_EVENT_COPY_ASYNC_END0("blink.console", ((String)title).utf8().data(),
this);
ditto

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/ThreadDebugger.h (right):

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/ThreadDebugger.h:36: void
reportMessageToConsole(v8::Local<v8::Context>, MessageType, MessageLevel, const
v8::FunctionCallbackInfo<v8::Value>& arguments, unsigned skipArgumentCount)
final;
Move these to "V8DebuggerClient implementation" block.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.h (right):

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.h:62:
v8::MaybeLocal<v8::Value> memoryInfo(v8::Isolate*, v8::Local<v8::Context>,
v8::Local<v8::Object> creationContext) override { return
v8::MaybeLocal<v8::Value>(); }
Move to cpp. Add ASSERT_NOT_REACHED.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/ConsoleMemory.idl (left):

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/ConsoleMemory.idl:6: //
https://groups.google.com/a/chromium.org/d/msg/blink-dev/g5YRCGpC9vs/b4OJz71N...
Let's preserve this comment and link.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/worklet/WorkletGlobalScope.idl (left):

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/worklet/WorkletGlobalScope.idl:12:
[Replaceable] readonly attribute WorkletConsole console;
Let's add a comment to all idl files about where did console go.

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

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:91:
v8::Local<v8::FunctionTemplate> functionTemplate =
v8::FunctionTemplate::New(isolate);
Store it per-isolate in V8DebuggerImpl instance. Follow-up with
V8InjectedScriptHost please.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:218:
client->reportMessageToConsole(context, CountMessageType, DebugMessageLevel,
message);
Use internalAddMessage instead?

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:243:
client->reportMessageToConsole(info.GetIsolate()->GetCurrentContext(),
LogMessageType, WarningMessageLevel, "'console.markTimeline' is deprecated.
Please use 'console.timeStamp' instead.");
Use internalAddMessage instead?

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:273:
profileAgent->consoleProfileEnd(title);
I'd move as much code to helper methods as possible:

String16 title = ....;
if (V8ProfilerAgentImpl* profiler = enabledProfilerFromInfo(info))
    profiler->consoleProfileEnd(title);

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:284:
client->reportMessageToConsole(info.GetIsolate()->GetCurrentContext(),
LogMessageType, WarningMessageLevel, "'console.timeline' is deprecated. Please
use 'console.time' instead.");
Use internalAddMessage instead?

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:295:
client->reportMessageToConsole(info.GetIsolate()->GetCurrentContext(),
LogMessageType, WarningMessageLevel, "'console.timelineEnd' is deprecated.
Please use 'console.timeEnd' instead.");
Use internalAddMessage instead?

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:298: void
V8Console::timeCallback(const v8::FunctionCallbackInfo<v8::Value>& info)
I'd go for more helpers here as well:

v8::Local<v8::String> title;
if (!info[0]->ToString(context).ToLocal(&title))
    return;

if (V8DebuggerClient* client = clientFromInfo(info)) {
    client->consoleTime(...)
    setInPrivateMap(info, "V8Console#tempMap", title, v8::Number::New(isolate,
client->currentTimeMS());
}

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:328:
V8DebuggerClient* client = clientFromConsoleObject(isolate, consoleObject);
I think we can replace many places with more readable:

if (V8DebuggerClient* client = clientFromInfo(info))
    client->doSomething();

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:421:
console->SetPrivate(context, debuggerPrivateKey(isolate),
v8::External::New(isolate, debugger));
I think it's better to store InspectedContext* pointer instead.

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

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:782:
installConsole(info.context, info.hasMemoryOnConsole);
Let's move this to InspectedContext constructor.

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

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.h:123:
V8ProfilerAgentImpl* findEnabledProfilerAgent(v8::Local<v8::Context>);
Let's move this method to call site.

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

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/public/V8DebuggerClient.h:38:
virtual void consoleTime(v8::Isolate*, const String16& title) = 0;
Why do we pass isolate? Client knows about it.

[kozy, 2016-04-12 21:58:12]

Patchset #7 (id:140001) has been deleted

[kozy, 2016-04-12 21:58:32]

All done. Dmitry, please take a look!

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/tracing/timeline-mark-timeline-expected.txt
(left):

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/tracing/timeline-mark-timeline-expected.txt:4:
TimeStamp Properties:
On 2016/04/12 03:32:15, dgozman wrote:
> Let's not introduce behavior changes in this patch except for planned ones.

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right):

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:108:
debugger()->contextCreated(V8ContextInfo(scriptState->context(),
contextGroupId(frame), world.isMainWorld(), origin ? origin->toRawString() : "",
world.isIsolatedWorld() ? world.isolatedWorldHumanReadableName() : "",
IdentifiersFactory::frameId(frame), executionContext->isDocument()));
On 2016/04/12 03:32:15, dgozman wrote:
> Inline it!

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:234: return
executionContext->isDocument() ? toV8(MemoryInfo::create(), creationContext,
isolate) : v8::MaybeLocal<v8::Value>();
On 2016/04/12 03:32:15, dgozman wrote:
> ASSERT(executionContext->isDocument())

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp (right):

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:92:
consoleMessage->setCallStack(ScriptCallStack::capture(1));
On 2016/04/12 03:32:15, dgozman wrote:
> consoleMessage->setScriptState(scriptState);

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:98:
TRACE_EVENT_COPY_ASYNC_BEGIN0("blink.console", ((String)title).utf8().data(),
this);
On 2016/04/12 03:32:15, dgozman wrote:
> String(title).utf8().data()

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:103:
TRACE_EVENT_COPY_ASYNC_END0("blink.console", ((String)title).utf8().data(),
this);
On 2016/04/12 03:32:15, dgozman wrote:
> ditto

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/ThreadDebugger.h (right):

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/ThreadDebugger.h:36: void
reportMessageToConsole(v8::Local<v8::Context>, MessageType, MessageLevel, const
v8::FunctionCallbackInfo<v8::Value>& arguments, unsigned skipArgumentCount)
final;
On 2016/04/12 03:32:15, dgozman wrote:
> Move these to "V8DebuggerClient implementation" block.

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.h (right):

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.h:62:
v8::MaybeLocal<v8::Value> memoryInfo(v8::Isolate*, v8::Local<v8::Context>,
v8::Local<v8::Object> creationContext) override { return
v8::MaybeLocal<v8::Value>(); }
On 2016/04/12 03:32:15, dgozman wrote:
> Move to cpp. Add ASSERT_NOT_REACHED.

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/ConsoleMemory.idl (left):

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/ConsoleMemory.idl:6: //
https://groups.google.com/a/chromium.org/d/msg/blink-dev/g5YRCGpC9vs/b4OJz71N...
On 2016/04/12 03:32:15, dgozman wrote:
> Let's preserve this comment and link.

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/worklet/WorkletGlobalScope.idl (left):

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/worklet/WorkletGlobalScope.idl:12:
[Replaceable] readonly attribute WorkletConsole console;
On 2016/04/12 03:32:15, dgozman wrote:
> Let's add a comment to all idl files about where did console go.

Done.

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

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:91:
v8::Local<v8::FunctionTemplate> functionTemplate =
v8::FunctionTemplate::New(isolate);
On 2016/04/12 03:32:16, dgozman wrote:
> Store it per-isolate in V8DebuggerImpl instance. Follow-up with
> V8InjectedScriptHost please.

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:218:
client->reportMessageToConsole(context, CountMessageType, DebugMessageLevel,
message);
On 2016/04/12 03:32:15, dgozman wrote:
> Use internalAddMessage instead?

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:243:
client->reportMessageToConsole(info.GetIsolate()->GetCurrentContext(),
LogMessageType, WarningMessageLevel, "'console.markTimeline' is deprecated.
Please use 'console.timeStamp' instead.");
On 2016/04/12 03:32:15, dgozman wrote:
> Use internalAddMessage instead?

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:273:
profileAgent->consoleProfileEnd(title);
On 2016/04/12 03:32:16, dgozman wrote:
> I'd move as much code to helper methods as possible:
> 
> String16 title = ....;
> if (V8ProfilerAgentImpl* profiler = enabledProfilerFromInfo(info))
>     profiler->consoleProfileEnd(title);

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:284:
client->reportMessageToConsole(info.GetIsolate()->GetCurrentContext(),
LogMessageType, WarningMessageLevel, "'console.timeline' is deprecated. Please
use 'console.time' instead.");
On 2016/04/12 03:32:16, dgozman wrote:
> Use internalAddMessage instead?

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:295:
client->reportMessageToConsole(info.GetIsolate()->GetCurrentContext(),
LogMessageType, WarningMessageLevel, "'console.timelineEnd' is deprecated.
Please use 'console.timeEnd' instead.");
On 2016/04/12 03:32:15, dgozman wrote:
> Use internalAddMessage instead?

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:298: void
V8Console::timeCallback(const v8::FunctionCallbackInfo<v8::Value>& info)
On 2016/04/12 03:32:15, dgozman wrote:
> I'd go for more helpers here as well:
> 
> v8::Local<v8::String> title;
> if (!info[0]->ToString(context).ToLocal(&title))
>     return;
> 
> if (V8DebuggerClient* client = clientFromInfo(info)) {
>     client->consoleTime(...)
>     setInPrivateMap(info, "V8Console#tempMap", title, v8::Number::New(isolate,
> client->currentTimeMS());
> }

Moved partially.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:328:
V8DebuggerClient* client = clientFromConsoleObject(isolate, consoleObject);
On 2016/04/12 03:32:15, dgozman wrote:
> I think we can replace many places with more readable:
> 
> if (V8DebuggerClient* client = clientFromInfo(info))
>     client->doSomething();

Done.

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:421:
console->SetPrivate(context, debuggerPrivateKey(isolate),
v8::External::New(isolate, debugger));
On 2016/04/12 03:32:16, dgozman wrote:
> I think it's better to store InspectedContext* pointer instead.

Done.

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

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:782:
installConsole(info.context, info.hasMemoryOnConsole);
On 2016/04/12 03:32:16, dgozman wrote:
> Let's move this to InspectedContext constructor.

Done.

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

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.h:123:
V8ProfilerAgentImpl* findEnabledProfilerAgent(v8::Local<v8::Context>);
On 2016/04/12 03:32:16, dgozman wrote:
> Let's move this method to call site.

Done.

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

https://codereview.chromium.org/1859293002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/public/V8DebuggerClient.h:38:
virtual void consoleTime(v8::Isolate*, const String16& title) = 0;
On 2016/04/12 03:32:16, dgozman wrote:
> Why do we pass isolate? Client knows about it.

Done.

[dgozman, 2016-04-12 23:21:58]

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ConsoleBase.idl (left):

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ConsoleBase.idl:49: void profile(optional
DOMString title = null);
Let's make sure these methods work if you pass non-string, and add a test when
possible.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Window.idl (right):

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Window.idl:143: // attribute console is
installed by InspectedContext in v8_inspector.
// [Replaceable] readonly attribute Console console;
// Console is installed by v8 inspector when context is created
// and is left commented here just for documentation.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/InspectedContext.h (right):

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/InspectedContext.h:45:
InspectedContext(V8DebuggerImpl*, const V8ContextInfo&, int contextId, bool
hasMemoryAttribute);
hasMemoryAttribute is already a part of V8ContextInfo.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp (right):

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:27: class
ConsoleScope {
ConsoleHelper

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:80:
debuggerClient->reportMessageToConsole(m_info.GetIsolate()->GetCurrentContext(),
type, level, String16(), &m_info, skipArgumentCount, -1);
m_context

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:86:
debuggerClient->reportMessageToConsole(m_info.GetIsolate()->GetCurrentContext(),
type, level, message, nullptr, 0, 1);
ditto

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:86:
debuggerClient->reportMessageToConsole(m_info.GetIsolate()->GetCurrentContext(),
type, level, message, nullptr, 0, 1);
0 /* skipArgumentsCount */, 1 /* maxStackSize */

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:94:
debuggerClient->reportMessageToConsole(m_info.GetIsolate()->GetCurrentContext(),
LogMessageType, WarningMessageLevel, message, nullptr, false, 0);
0 /* skipArgumentsCount */, 0 /* maxStackSize */

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:97: bool
firstArgAsBoolean(bool defaultValue)
firstArgToBoolean

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:113: if
(m_info.Length() > 0 && (!m_info[0]->IsNull() || !m_info[0]->IsUndefined())) {
Let's treat null and undefined as everything else.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:124: return
defaultValue;
nit: let's do early bailout

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:127:
v8::MaybeLocal<v8::Map> privateMapFromConsole(const char* name)
nit: privateMap

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:313: // Follow
Firebug's behavior of counting with null and undefined title in
This is not true in stable. Let's abandon.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:357: if
(V8ProfilerAgentImpl* profileAgent = scope.profileAgent())
nit: profilerAgent

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp (right):

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:40: #include
"platform/v8_inspector/V8ProfilerAgentImpl.h"
unused?

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:843: return
v8::Local<v8::FunctionTemplate>::New(m_isolate, *m_templates.get(name));
global has .Get(isolate)

[kozy, 2016-04-12 23:34:26]

All done. ptal!

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ConsoleBase.idl (left):

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ConsoleBase.idl:49: void profile(optional
DOMString title = null);
On 2016/04/12 23:21:57, dgozman wrote:
> Let's make sure these methods work if you pass non-string, and add a test when
> possible.

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Window.idl (right):

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Window.idl:143: // attribute console is
installed by InspectedContext in v8_inspector.
On 2016/04/12 23:21:57, dgozman wrote:
> // [Replaceable] readonly attribute Console console;
> // Console is installed by v8 inspector when context is created
> // and is left commented here just for documentation.

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/InspectedContext.h (right):

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/InspectedContext.h:45:
InspectedContext(V8DebuggerImpl*, const V8ContextInfo&, int contextId, bool
hasMemoryAttribute);
On 2016/04/12 23:21:57, dgozman wrote:
> hasMemoryAttribute is already a part of V8ContextInfo.

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp (right):

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:27: class
ConsoleScope {
On 2016/04/12 23:21:57, dgozman wrote:
> ConsoleHelper

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:80:
debuggerClient->reportMessageToConsole(m_info.GetIsolate()->GetCurrentContext(),
type, level, String16(), &m_info, skipArgumentCount, -1);
On 2016/04/12 23:21:57, dgozman wrote:
> m_context

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:86:
debuggerClient->reportMessageToConsole(m_info.GetIsolate()->GetCurrentContext(),
type, level, message, nullptr, 0, 1);
On 2016/04/12 23:21:57, dgozman wrote:
> 0 /* skipArgumentsCount */, 1 /* maxStackSize */

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:86:
debuggerClient->reportMessageToConsole(m_info.GetIsolate()->GetCurrentContext(),
type, level, message, nullptr, 0, 1);
On 2016/04/12 23:21:58, dgozman wrote:
> ditto

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:94:
debuggerClient->reportMessageToConsole(m_info.GetIsolate()->GetCurrentContext(),
LogMessageType, WarningMessageLevel, message, nullptr, false, 0);
On 2016/04/12 23:21:57, dgozman wrote:
> 0 /* skipArgumentsCount */, 0 /* maxStackSize */

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:97: bool
firstArgAsBoolean(bool defaultValue)
On 2016/04/12 23:21:57, dgozman wrote:
> firstArgToBoolean

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:113: if
(m_info.Length() > 0 && (!m_info[0]->IsNull() || !m_info[0]->IsUndefined())) {
On 2016/04/12 23:21:57, dgozman wrote:
> Let's treat null and undefined as everything else.

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:124: return
defaultValue;
On 2016/04/12 23:21:57, dgozman wrote:
> nit: let's do early bailout

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:127:
v8::MaybeLocal<v8::Map> privateMapFromConsole(const char* name)
On 2016/04/12 23:21:57, dgozman wrote:
> nit: privateMap

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:313: // Follow
Firebug's behavior of counting with null and undefined title in
On 2016/04/12 23:21:57, dgozman wrote:
> This is not true in stable. Let's abandon.

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8Console.cpp:357: if
(V8ProfilerAgentImpl* profileAgent = scope.profileAgent())
On 2016/04/12 23:21:57, dgozman wrote:
> nit: profilerAgent

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp (right):

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:40: #include
"platform/v8_inspector/V8ProfilerAgentImpl.h"
On 2016/04/12 23:21:58, dgozman wrote:
> unused?

Done.

https://codereview.chromium.org/1859293002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp:843: return
v8::Local<v8::FunctionTemplate>::New(m_isolate, *m_templates.get(name));
On 2016/04/12 23:21:58, dgozman wrote:
> global has .Get(isolate)

Done.

[dgozman, 2016-04-12 23:58:57]

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

[dgozman, 2016-04-12 23:58:58]

This looks good to me. Pavel, please take a look as well.

[kozy, 2016-04-18 23:44:23]

Rebased, please take a look.

[kozy, 2016-04-20 19:18:12]

ping?

[dgozman, 2016-04-20 21:40:52]

lgtm

[dgozman, 2016-04-20 21:41:20]

Description was changed from

==========
[DevTools] Move Console to v8_inspector

Console functions works without console object as receiver.
var log = console.log; log(42);

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

to

==========
[DevTools] Move Console to v8_inspector

Console functions works without console object as receiver.
var log = console.log; log(42);

BUG=167911,580337
R=dgozman@chromium.org
==========

[dgozman, 2016-04-20 21:45:52]

Description was changed from

==========
[DevTools] Move Console to v8_inspector

Console functions works without console object as receiver.
var log = console.log; log(42);

BUG=167911,580337
R=dgozman@chromium.org
==========

to

==========
[DevTools] Move Console to v8_inspector

This is a first patch in the effort of moving console to v8_inspector:
- move Console object;
- move ConsoleMessage;
- move ConsoleMessageStorage.

The benefits are:
- moves the last v8-related inspector aspect to v8_inspector;
- simplifies bindings between console, profiler and timeline;
- reduces console API for the embedder, which mostly uses it as a log storage.

As a side effect, this patch makes Console object functions automatically bound
to Console:
var log = console.log; log(42);

BUG=167911,580337
R=dgozman@chromium.org
==========

[pfeldman, 2016-04-20 23:43:19]

lgtm

[kozy, 2016-04-21 00:04:54]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-04-21 00:04:55]

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

[commit-bot: I haz the power, 2016-04-21 00:05:45]

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

[commit-bot: I haz the power, 2016-04-21 00:18:41]

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

[commit-bot: I haz the power, 2016-04-21 00:18:43]

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

[kozy, 2016-04-21 00:20:17]

Description was changed from

==========
[DevTools] Move Console to v8_inspector

This is a first patch in the effort of moving console to v8_inspector:
- move Console object;
- move ConsoleMessage;
- move ConsoleMessageStorage.

The benefits are:
- moves the last v8-related inspector aspect to v8_inspector;
- simplifies bindings between console, profiler and timeline;
- reduces console API for the embedder, which mostly uses it as a log storage.

As a side effect, this patch makes Console object functions automatically bound
to Console:
var log = console.log; log(42);

BUG=167911,580337
R=dgozman@chromium.org
==========

to

==========
[DevTools] Move Console to v8_inspector

This is a first patch in the effort of moving console to v8_inspector:
- move Console object;
- move ConsoleMessage;
- move ConsoleMessageStorage.

The benefits are:
- moves the last v8-related inspector aspect to v8_inspector;
- simplifies bindings between console, profiler and timeline;
- reduces console API for the embedder, which mostly uses it as a log storage.

As a side effect, this patch makes Console object functions automatically bound
to Console:
var log = console.log; log(42);

BUG=167911,580337
R=dgozman@chromium.org,jochen@chromium.org
==========

[kozy, 2016-04-21 00:20:18]

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

[kozy, 2016-04-21 00:21:42]

@jochen, please take a look.
I need owner lgtm for following webexposed tests:
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-dedicated-worker-expected.txt
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-shared-worker-expected.txt

[jochen (gone - plz use gerrit), 2016-04-21 14:45:37]

lgtm

[kozy, 2016-04-21 15:26:53]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-04-21 15:27:12]

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

[commit-bot: I haz the power, 2016-04-21 16:44:14]

Description was changed from

==========
[DevTools] Move Console to v8_inspector

This is a first patch in the effort of moving console to v8_inspector:
- move Console object;
- move ConsoleMessage;
- move ConsoleMessageStorage.

The benefits are:
- moves the last v8-related inspector aspect to v8_inspector;
- simplifies bindings between console, profiler and timeline;
- reduces console API for the embedder, which mostly uses it as a log storage.

As a side effect, this patch makes Console object functions automatically bound
to Console:
var log = console.log; log(42);

BUG=167911,580337
R=dgozman@chromium.org,jochen@chromium.org
==========

to

==========
[DevTools] Move Console to v8_inspector

This is a first patch in the effort of moving console to v8_inspector:
- move Console object;
- move ConsoleMessage;
- move ConsoleMessageStorage.

The benefits are:
- moves the last v8-related inspector aspect to v8_inspector;
- simplifies bindings between console, profiler and timeline;
- reduces console API for the embedder, which mostly uses it as a log storage.

As a side effect, this patch makes Console object functions automatically bound
to Console:
var log = console.log; log(42);

BUG=167911,580337
R=dgozman@chromium.org,jochen@chromium.org
==========

[commit-bot: I haz the power, 2016-04-21 16:44:16]

Committed patchset #12 (id:250001)

[commit-bot: I haz the power, 2016-04-22 19:35:14]

Description was changed from

==========
[DevTools] Move Console to v8_inspector

This is a first patch in the effort of moving console to v8_inspector:
- move Console object;
- move ConsoleMessage;
- move ConsoleMessageStorage.

The benefits are:
- moves the last v8-related inspector aspect to v8_inspector;
- simplifies bindings between console, profiler and timeline;
- reduces console API for the embedder, which mostly uses it as a log storage.

As a side effect, this patch makes Console object functions automatically bound
to Console:
var log = console.log; log(42);

BUG=167911,580337
R=dgozman@chromium.org,jochen@chromium.org
==========

to

==========
[DevTools] Move Console to v8_inspector

This is a first patch in the effort of moving console to v8_inspector:
- move Console object;
- move ConsoleMessage;
- move ConsoleMessageStorage.

The benefits are:
- moves the last v8-related inspector aspect to v8_inspector;
- simplifies bindings between console, profiler and timeline;
- reduces console API for the embedder, which mostly uses it as a log storage.

As a side effect, this patch makes Console object functions automatically bound
to Console:
var log = console.log; log(42);

BUG=167911,580337
R=dgozman@chromium.org,jochen@chromium.org

Committed: https://crrev.com/807ec9550e8a31517966636e6a5b506474ab4ea9
Cr-Commit-Position: refs/heads/master@{#388788}
==========

[commit-bot: I haz the power, 2016-04-22 19:35:16]

Patchset 12 (id:??) landed as
https://crrev.com/807ec9550e8a31517966636e6a5b506474ab4ea9
Cr-Commit-Position: refs/heads/master@{#388788}