[DevTools] Dispatch messages to V8InspectorSession directly.

[DevTools] Dispatch messages to V8InspectorSession directly.

This patch adds dispatcher, backend and frontend channel to V8InspectorSessionImpl
and removes agent public interfaces and their blink wrappers.

Drive-by: removed sessionId from inspector_protocol since InspectorSession now
handles it.

Next step would be to separate v8 instrumentation from InspectorSession,
remove latter from worker (depends on console efforts) and merge session
into WebDevToolsAgentImpl.

BUG=580337
Committed: https://crrev.com/4698dd61590b2cba8346c5efd83428b9a3c3e970
Cr-Commit-Position: refs/heads/master@{#395269}

[dgozman, 2016-05-11 00:46:37]

Description was changed from

==========
[DevTools] Dispatch messages to V8InspectorSession directly.

This patch adds dispatcher, backend and frontend channel to
V8InspectorSessionImpl
and removes agent public interfaces and their blink wrappers.

Next step would be to separate v8 instrumentation from InspectorSession,
remove latter from worker (depends on console efforts) and merge session
into WebDevToolsAgentImpl.

BUG=580337
==========

to

==========
[DevTools] Dispatch messages to V8InspectorSession directly.

This patch adds dispatcher, backend and frontend channel to
V8InspectorSessionImpl
and removes agent public interfaces and their blink wrappers.

Drive-by: removed sessionId from inspector_protocol since InspectorSession now
handles it.

Next step would be to separate v8 instrumentation from InspectorSession,
remove latter from worker (depends on console efforts) and merge session
into WebDevToolsAgentImpl.

BUG=580337
==========

[dgozman, 2016-05-11 00:46:54]

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

[dgozman, 2016-05-11 00:46:56]

Could you please take a look?

https://codereview.chromium.org/1967933002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorRuntimeAgent.cpp (left):

https://codereview.chromium.org/1967933002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorRuntimeAgent.cpp:85:
TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("devtools.timeline"),
"UpdateCounters", TRACE_EVENT_SCOPE_THREAD, "data",
InspectorUpdateCountersEvent::data());
This one was removed, but looks like all tests pass.

[caseq, 2016-05-11 01:40:34]

lgtm

https://codereview.chromium.org/1967933002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorSession.cpp (right):

https://codereview.chromium.org/1967933002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorSession.cpp:31: ,
m_shouldRestore(savedState)
!!savedState?

[dgozman, 2016-05-11 02:28:46]

The CQ bit was checked by dgozman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-11 02:29:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1967933002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1967933002/1

[commit-bot: I haz the power, 2016-05-11 02:42:24]

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

[commit-bot: I haz the power, 2016-05-11 02:42:25]

Dry run: Try jobs failed on following builders:
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[dgozman, 2016-05-12 01:28:44]

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

[dgozman, 2016-05-12 01:28:45]

Pavel, I need an owner stamp for platform/ and and public/. Take a look please.

[pfeldman, 2016-05-12 01:31:58]

lgtm

[dgozman, 2016-05-12 22:20:07]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-05-12 22:20:08]

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

[commit-bot: I haz the power, 2016-05-12 22:20:31]

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

[commit-bot: I haz the power, 2016-05-12 22:42:44]

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

[commit-bot: I haz the power, 2016-05-12 22:42:45]

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

[dgozman, 2016-05-13 02:07:14]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-05-13 02:07:15]

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

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

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

[commit-bot: I haz the power, 2016-05-13 03:18:51]

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

[commit-bot: I haz the power, 2016-05-13 03:18:52]

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

[dgozman, 2016-05-20 21:46:06]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-05-20 21:46:07]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1967933002/#ps80001
(title: "rebased, fixed thread issue in WorkerThread")

[commit-bot: I haz the power, 2016-05-20 21:46:28]

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

[commit-bot: I haz the power, 2016-05-21 01:02:34]

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

[commit-bot: I haz the power, 2016-05-21 01:02:36]

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

[dgozman, 2016-05-21 01:05:31]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2016-05-21 01:05:54]

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

[commit-bot: I haz the power, 2016-05-21 04:17:30]

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

[commit-bot: I haz the power, 2016-05-21 04:17:31]

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

[dgozman, 2016-05-21 04:42:21]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2016-05-21 04:42:36]

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

[commit-bot: I haz the power, 2016-05-21 06:06:55]

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

[commit-bot: I haz the power, 2016-05-21 06:06:57]

Failed to apply patch for
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:
While running git apply --index -3 -p1;
  error: patch failed:
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:272
  Falling back to three-way merge...
  Applied patch to
'third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp' with
conflicts.
  U third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp

Patch:      
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
Index: third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
diff --git
a/third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
b/third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
index
bd703af00d63ff89ed8257767a19301855111ff2..7717cf51489b95deff0b88bfa07924f840010454
100644
--- a/third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
+++ b/third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
@@ -155,12 +155,12 @@ static PassOwnPtr<protocol::Debugger::Location>
buildProtocolLocation(const Stri
         .setColumnNumber(columnNumber).build();
 }
 
-V8DebuggerAgentImpl::V8DebuggerAgentImpl(V8InspectorSessionImpl* session)
+V8DebuggerAgentImpl::V8DebuggerAgentImpl(V8InspectorSessionImpl* session,
protocol::Frontend::Debugger* frontend, protocol::DictionaryValue* state)
     : m_debugger(session->debugger())
     , m_session(session)
     , m_enabled(false)
-    , m_state(nullptr)
-    , m_frontend(nullptr)
+    , m_state(state)
+    , m_frontend(frontend)
     , m_isolate(m_debugger->isolate())
     , m_breakReason(protocol::Debugger::Paused::ReasonEnum::Other)
     , m_scheduledDebuggerStep(NoStep)
@@ -272,19 +272,6 @@ void
V8DebuggerAgentImpl::internalSetAsyncCallStackDepth(int depth)
     }
 }
 
-void V8DebuggerAgentImpl::setInspectorState(protocol::DictionaryValue* state)
-{
-    m_state = state;
-}
-
-void V8DebuggerAgentImpl::clearFrontend()
-{
-    ErrorString error;
-    disable(&error);
-    ASSERT(m_frontend);
-    m_frontend = nullptr;
-}
-
 void V8DebuggerAgentImpl::restore()
 {
     ASSERT(!m_enabled);

[dgozman, 2016-05-21 23:29:14]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-05-21 23:29:14]

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

[commit-bot: I haz the power, 2016-05-21 23:29:23]

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

[commit-bot: I haz the power, 2016-05-22 01:53:04]

Description was changed from

==========
[DevTools] Dispatch messages to V8InspectorSession directly.

This patch adds dispatcher, backend and frontend channel to
V8InspectorSessionImpl
and removes agent public interfaces and their blink wrappers.

Drive-by: removed sessionId from inspector_protocol since InspectorSession now
handles it.

Next step would be to separate v8 instrumentation from InspectorSession,
remove latter from worker (depends on console efforts) and merge session
into WebDevToolsAgentImpl.

BUG=580337
==========

to

==========
[DevTools] Dispatch messages to V8InspectorSession directly.

This patch adds dispatcher, backend and frontend channel to
V8InspectorSessionImpl
and removes agent public interfaces and their blink wrappers.

Drive-by: removed sessionId from inspector_protocol since InspectorSession now
handles it.

Next step would be to separate v8 instrumentation from InspectorSession,
remove latter from worker (depends on console efforts) and merge session
into WebDevToolsAgentImpl.

BUG=580337
==========

[commit-bot: I haz the power, 2016-05-22 01:53:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-22 01:54:49]

Description was changed from

==========
[DevTools] Dispatch messages to V8InspectorSession directly.

This patch adds dispatcher, backend and frontend channel to
V8InspectorSessionImpl
and removes agent public interfaces and their blink wrappers.

Drive-by: removed sessionId from inspector_protocol since InspectorSession now
handles it.

Next step would be to separate v8 instrumentation from InspectorSession,
remove latter from worker (depends on console efforts) and merge session
into WebDevToolsAgentImpl.

BUG=580337
==========

to

==========
[DevTools] Dispatch messages to V8InspectorSession directly.

This patch adds dispatcher, backend and frontend channel to
V8InspectorSessionImpl
and removes agent public interfaces and their blink wrappers.

Drive-by: removed sessionId from inspector_protocol since InspectorSession now
handles it.

Next step would be to separate v8 instrumentation from InspectorSession,
remove latter from worker (depends on console efforts) and merge session
into WebDevToolsAgentImpl.

BUG=580337

Committed: https://crrev.com/4698dd61590b2cba8346c5efd83428b9a3c3e970
Cr-Commit-Position: refs/heads/master@{#395269}
==========

[commit-bot: I haz the power, 2016-05-22 01:54:50]

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

[sof, 2016-05-23 06:20:52]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-05-23 06:20:53]

https://codereview.chromium.org/1967933002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/CodeGeneratorInstrumentation.py
(right):

https://codereview.chromium.org/1967933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/CodeGeneratorInstrumentation.py:100:
class CORE_EXPORT InstrumentingAgents : public
GarbageCollectedFinalized<InstrumentingAgents> {
Why this change? This object doesn't need to run any finalizer (and if it did,
the clang plugin would issue an error.)

[dgozman, 2016-05-23 15:47:02]

https://codereview.chromium.org/1967933002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/CodeGeneratorInstrumentation.py
(right):

https://codereview.chromium.org/1967933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/CodeGeneratorInstrumentation.py:100:
class CORE_EXPORT InstrumentingAgents : public
GarbageCollectedFinalized<InstrumentingAgents> {
On 2016/05/23 06:20:53, sof wrote:
> Why this change? This object doesn't need to run any finalizer (and if it did,
> the clang plugin would issue an error.)

I was fixing a crash, and thought that maybe we don't destruct HeapHashMap
inside this object. That seemed to help at first, but not really.

I can revert this back in a follow-up.

[sof, 2016-05-23 18:00:30]

https://codereview.chromium.org/1967933002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/CodeGeneratorInstrumentation.py
(right):

https://codereview.chromium.org/1967933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/CodeGeneratorInstrumentation.py:100:
class CORE_EXPORT InstrumentingAgents : public
GarbageCollectedFinalized<InstrumentingAgents> {
On 2016/05/23 15:47:02, dgozman wrote:
> On 2016/05/23 06:20:53, sof wrote:
> > Why this change? This object doesn't need to run any finalizer (and if it
did,
> > the clang plugin would issue an error.)
> 
> I was fixing a crash, and thought that maybe we don't destruct HeapHashMap
> inside this object. That seemed to help at first, but not really.
> 
> I can revert this back in a follow-up.

Thanks for explaining; the HeapHashMap backing store table will independently
finalize its elements, if needed.

The reason I fuss over this :) is that we're considering enabling a plugin
warning by default (warn-unneeded-finalizer) which checks for unnecessary uses
of *Finalized<>, so keeping the codebase warning-free until then makes some
sense. I don't think such a warning will be getting in anyone's way very much,
but that's the trade-off to make.

https://codereview.chromium.org/2003033002/ switches to GarbageCollected<>.

[dgozman, 2016-05-23 18:04:33]

On 2016/05/23 18:00:30, sof wrote:
>
https://codereview.chromium.org/1967933002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/inspector/CodeGeneratorInstrumentation.py
> (right):
> 
>
https://codereview.chromium.org/1967933002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/inspector/CodeGeneratorInstrumentation.py:100:
> class CORE_EXPORT InstrumentingAgents : public
> GarbageCollectedFinalized<InstrumentingAgents> {
> On 2016/05/23 15:47:02, dgozman wrote:
> > On 2016/05/23 06:20:53, sof wrote:
> > > Why this change? This object doesn't need to run any finalizer (and if it
> did,
> > > the clang plugin would issue an error.)
> > 
> > I was fixing a crash, and thought that maybe we don't destruct HeapHashMap
> > inside this object. That seemed to help at first, but not really.
> > 
> > I can revert this back in a follow-up.
> 
> Thanks for explaining; the HeapHashMap backing store table will independently
> finalize its elements, if needed.
> 
> The reason I fuss over this :) is that we're considering enabling a plugin
> warning by default (warn-unneeded-finalizer) which checks for unnecessary uses
> of *Finalized<>, so keeping the codebase warning-free until then makes some
> sense. I don't think such a warning will be getting in anyone's way very much,
> but that's the trade-off to make.
> 
> https://codereview.chromium.org/2003033002/ switches to GarbageCollected<>.

Good to know. Thanks for the patch!