Wire up the Inspector V8 Debugger

Wire up more of the DebuggerAgent

The debugger can now correctly break on exceptions
and show the corresponding line in the inspector.

It correctly understands which scripts are internal
to sky and does not pause during them.

There is still a ton to make work here
(including stacktraces which I have not tested),
but basic functionality seems to work.

The current implementation is not smart enough to
unpause the inspector when the frontend disconnects.

BUG=434510, 434513
R=abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/2de560eaad3eaaab14edd887532b105baac110e8

[eseidel, 2014-11-14 00:45:04]

eseidel@chromium.org changed reviewers:
+ abarth@chromium.org, yurys@chromium.org

[yurys, 2014-11-14 07:38:31]

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/frame/FrameH...
File sky/engine/core/frame/FrameHost.h (left):

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/frame/FrameH...
sky/engine/core/frame/FrameHost.h:77: void trace(Visitor*);
Just curious, aren't you going to oilpanize Sky?

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/inspector/As...
File sky/engine/core/inspector/AsyncCallStackTracker.cpp (right):

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/inspector/As...
sky/engine/core/inspector/AsyncCallStackTracker.cpp:245: // void
AsyncCallStackTracker::didPostExecutionContextTask(ExecutionContext* context,
ExecutionContextTask* task, const ScriptValue& callFrames)
Why not delete these methods completely if the context structure is different in
Sky?

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/inspector/As...
File sky/engine/core/inspector/AsyncCallStackTracker.h (right):

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/inspector/As...
sky/engine/core/inspector/AsyncCallStackTracker.h:51: class
AsyncCallStackTracker FINAL {
Given that you tend to write DOM in JS, Sky will likely have a different
implementation of async stacks. Anyways, I guess it is easier to just restore
all dependencies of the debugger agent now and clean up them later.

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/inspector/In...
File sky/engine/core/inspector/InstrumentingAgents.h (right):

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/inspector/In...
sky/engine/core/inspector/InstrumentingAgents.h:17: class InstrumentingAgents {
This class should be safe to remove if you drop async call stacks support.

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/inspector/in...
File sky/engine/core/inspector/inspector_backend_mojo.cc (right):

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/inspector/in...
sky/engine/core/inspector/inspector_backend_mojo.cc:45:
PageScriptDebugServer::setMainThreadIsolate(isolate);
You'll also need to supply the debug server with a delegate to run nested
message loop while script execution is paused. In blink it is done via
PageScriptDebugServer::setClientMessageLoop.

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/inspector/in...
File sky/engine/core/inspector/inspector_backend_mojo.h (right):

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/inspector/in...
sky/engine/core/inspector/inspector_backend_mojo.h:25:
InspectorBackendMojo(const FrameHost& frame_host);
nit: mark as explicit?

[abarth-chromium, 2014-11-14 08:00:57]

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/frame/FrameH...
File sky/engine/core/frame/FrameHost.h (left):

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/frame/FrameH...
sky/engine/core/frame/FrameHost.h:77: void trace(Visitor*);
On 2014/11/14 at 07:38:31, yurys wrote:
> Just curious, aren't you going to oilpanize Sky?

Nope.

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/inspector/As...
File sky/engine/core/inspector/AsyncCallStackTracker.h (right):

https://codereview.chromium.org/727593004/diff/1/sky/engine/core/inspector/As...
sky/engine/core/inspector/AsyncCallStackTracker.h:51: class
AsyncCallStackTracker FINAL {
On 2014/11/14 at 07:38:31, yurys wrote:
> Given that you tend to write DOM in JS, Sky will likely have a different
implementation of async stacks. Anyways, I guess it is easier to just restore
all dependencies of the debugger agent now and clean up them later.

It depends what you mean by DOM.  We're not planning to move Node to JS.  Just
almost all the subclasses of Element.

[eseidel, 2014-11-14 21:20:51]

The most interesting bit to review here is the inspector_backend_mojo.*

[abarth-chromium, 2014-11-14 22:29:45]

LGTM

https://codereview.chromium.org/727593004/diff/100001/sky/engine/core/inspect...
File sky/engine/core/inspector/inspector_backend_mojo.cc (right):

https://codereview.chromium.org/727593004/diff/100001/sky/engine/core/inspect...
sky/engine/core/inspector/inspector_backend_mojo.cc:28: MessageLoopAdaptor() { }
xxx
 public:
  MessageLoopAdaptor

https://codereview.chromium.org/727593004/diff/100001/sky/engine/core/inspect...
sky/engine/core/inspector/inspector_backend_mojo.cc:32: {
Merge lines

https://codereview.chromium.org/727593004/diff/100001/sky/engine/core/inspect...
sky/engine/core/inspector/inspector_backend_mojo.cc:74: debugger_agent_ =
PageDebuggerAgent::create(&PageScriptDebugServer::shared(), &frame_host_.page(),
script_manager_.get());
This line is way more than 80 cols :)

[eseidel, 2014-11-18 19:26:19]

ptal

[eseidel, 2014-11-18 19:29:26]

https://codereview.chromium.org/727593004/diff/120001/sky/engine/core/inspect...
File sky/engine/core/inspector/InspectorDebuggerAgent.cpp (right):

https://codereview.chromium.org/727593004/diff/120001/sky/engine/core/inspect...
sky/engine/core/inspector/InspectorDebuggerAgent.cpp:1294: bool
InspectorDebuggerAgent::shouldSkipInspectorInternals()
This is the code to ignore the sky js files.

[eseidel, 2014-11-18 19:38:48]

https://codereview.chromium.org/727593004/diff/120001/sky/engine/bindings/cor...
File sky/engine/bindings/core/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/727593004/diff/120001/sky/engine/bindings/cor...
sky/engine/bindings/core/v8/ScriptDebugServer.cpp:583: const blink::WebData&
debuggerScriptSourceResource =
blink::Platform::current()->loadResource("DebuggerScript.js");
I'm not sure what the Source suffix did.  Maybe there was some build processing
of the file?

https://codereview.chromium.org/727593004/diff/120001/sky/engine/bindings/cor...
File sky/engine/bindings/core/v8/ScriptRegexp.h (right):

https://codereview.chromium.org/727593004/diff/120001/sky/engine/bindings/cor...
sky/engine/bindings/core/v8/ScriptRegexp.h:28: #define ScriptRegexp_h
This definitely is not needed.  I just had too much to do in this patch to fix
the callers.

https://codereview.chromium.org/727593004/diff/120001/sky/engine/core/inspect...
File sky/engine/core/inspector/InjectedScriptHost.idl (right):

https://codereview.chromium.org/727593004/diff/120001/sky/engine/core/inspect...
sky/engine/core/inspector/InjectedScriptHost.idl:41: [NotEnumerable,
Unforgeable, Custom] boolean isHTMLAllCollection(any obj);
This should be removable. :)

https://codereview.chromium.org/727593004/diff/120001/sky/engine/core/inspect...
File sky/engine/core/inspector/InspectorState.h (right):

https://codereview.chromium.org/727593004/diff/120001/sky/engine/core/inspect...
sky/engine/core/inspector/InspectorState.h:53:
InspectorState(InspectorStateUpdateListener*, PassRefPtr<JSONObject>);
This appears to be the inspector settings object.  I don't currently wire it up
to load from the cookie, but we may need to do that in the future.

https://codereview.chromium.org/727593004/diff/120001/sky/viewer/document_vie...
File sky/viewer/document_view.cc (right):

https://codereview.chromium.org/727593004/diff/120001/sky/viewer/document_vie...
sky/viewer/document_view.cc:80: debugger_id_(s_next_debugger_id++),
This ended up not being used, will remove.

https://codereview.chromium.org/727593004/diff/120001/sky/viewer/platform/pla...
File sky/viewer/platform/platform_impl.cc (right):

https://codereview.chromium.org/727593004/diff/120001/sky/viewer/platform/pla...
sky/viewer/platform/platform_impl.cc:241: blink::WebData
PlatformImpl::loadResource(const char* name)
This is a hack for now.  Eventually we may be able to move away from using
DebuggerScript.js and InspectorScript.js at all.  Certainly InspectorScript.js
should be able to be re-written as a sky module.  DebuggerScript may need to
remain a script if it needs to be injected as a v8 internal script.

[abarth-chromium, 2014-11-18 22:37:37]

lgtm

https://codereview.chromium.org/727593004/diff/120001/sky/engine/core/inspect...
File sky/engine/core/inspector/InspectorDebuggerAgent.cpp (right):

https://codereview.chromium.org/727593004/diff/120001/sky/engine/core/inspect...
sky/engine/core/inspector/InspectorDebuggerAgent.cpp:1304: }
This is kind of goofy to hardcode in the binary...  Can you file a bug about
this?

https://codereview.chromium.org/727593004/diff/120001/sky/viewer/document_vie...
File sky/viewer/document_view.cc (right):

https://codereview.chromium.org/727593004/diff/120001/sky/viewer/document_vie...
sky/viewer/document_view.cc:80: debugger_id_(s_next_debugger_id++),
On 2014/11/18 at 19:38:48, eseidel wrote:
> This ended up not being used, will remove.

ok

https://codereview.chromium.org/727593004/diff/120001/sky/viewer/platform/pla...
File sky/viewer/platform/platform_impl.cc (right):

https://codereview.chromium.org/727593004/diff/120001/sky/viewer/platform/pla...
sky/viewer/platform/platform_impl.cc:242: {
Merge with previous line.

https://codereview.chromium.org/727593004/diff/120001/sky/viewer/platform/pla...
sky/viewer/platform/platform_impl.cc:247: base::FilePath inspector_dir =
engine_dir.AppendASCII("core").AppendASCII("inspector");
Can you file a bug about this too?  We won't have DIR_SOURCE_ROOT in
production....

[eseidel, 2014-11-18 23:05:10]

Committed patchset #7 (id:120001) manually as
2de560eaad3eaaab14edd887532b105baac110e8.