[WIP] DevToolsClientBackend with tracing API.

[WIP] DevToolsClientBackend with tracing support (almost).

BUG=443897

[chrishenry, 2014-12-19 10:28:12]

chrishenry@google.com changed reviewers:
+ ariblue@google.com, dtu@chromium.org, nednguyen@google.com,
slamm@chromium.org, tonyg@chromium.org

[chrishenry, 2014-12-19 10:28:12]

Hey guys,
This is an initial (very rough) prototype of how we can refactor tracing so that
we can support it for app as well. In short, I'm putting it behind
DevToolsClientBackend class.

The goal of DevToolsClientBackend also includes adding InspectorBackendList as
its members and things like SystemInfoBackend... Essentially, it is _the_
devtools client API.

Benchmark actually runs with this patch, but there are duplicated codes and
broken unit tests still. Also, StopTracing logic isn't as it should be yet, at
least until we get access to InspectorBackendList in DevToolsClientBackend,
allowing moving the logic from ChromeBrowserBackend.

Thoughts?

[nednguyen, 2014-12-19 14:43:39]

nednguyen@google.com changed reviewers:
+ sullivan@chromium.org

[nednguyen, 2014-12-19 14:43:40]

+Annie since this patch will create a rebase conflict for her huge patch :-(.
She already had to rebase twice for that huge patch.

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py
(right):

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:36:
raise exceptions.BrowserConnectionGoneException(self.browser, e)
These Exception all need to be fixed. 
maybe just raised a single exception here: DevToolsClientBackendConnectionError.
Then the caller related to browser can catch this and raise Browser* exceptions.

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:38:
def IsAvailable(self):
IsAlive(self)

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/tracing_controller_backend.py
(right):

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/tracing_controller_backend.py:45: result
= browser_backend.StopTracing()
TracingController doesn't need the handle to browser_backend. It should only
grab handle to tracing_backend

[slamm, 2014-12-19 19:08:10]

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend.py
(right):

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/tracing_backend.py:48:
#TODO(nednguyen): remove this when the stable branch pass 2118.
FYI stable passed 2118, in September/October timeframe.

[ariblue, 2014-12-19 19:26:55]

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:82:
self._port)
There are implicit assumptions in this function and in GetSystemInfo() below
that classes that inherit from ChromeBrowserBackend will define self._port. Do
we need any sort of assertions or other protections that self._port is defined
at this point?

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:238:
def Request(self, path, timeout=30, throw_network_exception=False):
This should be killed or updated to use the devtools_client.

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py
(right):

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:17:
class DevToolsClientBackend(object):
Does it make sense to split this into two classes?

* One which handles the underlying connection logic for devtools HTTP requests
(tracks the port and handles the _RequestJson).
* One that provides the API (GetChromeBranchNumber, StartTracing,
ListInspectableContexts, etc).

[chrishenry, 2014-12-19 19:27:13]

Heh, it's pretty unlikely that I'll get to submit this before going on vacation
tho, so don't worry, Annie!

It's probably worth splitting into smaller patches too (already did with some of
the collateral cleanups).

[ariblue, 2014-12-19 19:30:12]

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py
(right):

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:17:
class DevToolsClientBackend(object):
On 2014/12/19 19:26:55, ariblue wrote:
> Does it make sense to split this into two classes?
> 
> * One which handles the underlying connection logic for devtools HTTP requests
> (tracks the port and handles the _RequestJson).
> * One that provides the API (GetChromeBranchNumber, StartTracing,
> ListInspectableContexts, etc).

Just saw your comment here:
https://code.google.com/p/chromium/issues/detail?id=423954#c29

[chrishenry, 2014-12-19 20:51:21]

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:82:
self._port)
On 2014/12/19 19:26:55, ariblue wrote:
> There are implicit assumptions in this function and in GetSystemInfo() below
> that classes that inherit from ChromeBrowserBackend will define self._port. Do
> we need any sort of assertions or other protections that self._port is defined
> at this point?

Yes. (: Done (assert). I don't quite like the handling of self._port, but don't
want to expand scope as well...

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:238:
def Request(self, path, timeout=30, throw_network_exception=False):
On 2014/12/19 19:26:55, ariblue wrote:
> This should be killed or updated to use the devtools_client.

Agreed. Will split to InspectorJson patch.

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py
(right):

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:17:
class DevToolsClientBackend(object):
On 2014/12/19 19:30:12, ariblue wrote:
> On 2014/12/19 19:26:55, ariblue wrote:
> > Does it make sense to split this into two classes?
> > 
> > * One which handles the underlying connection logic for devtools HTTP
requests
> > (tracks the port and handles the _RequestJson).
> > * One that provides the API (GetChromeBranchNumber, StartTracing,
> > ListInspectableContexts, etc).
> 
> Just saw your comment here:
> https://code.google.com/p/chromium/issues/detail?id=423954#c29

I think you and Dave are right here. Having separate InspectorJson seems
helpful, especially given that TabListBackend will also need it.

I'll look into getting this checked in separately.

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:36:
raise exceptions.BrowserConnectionGoneException(self.browser, e)
On 2014/12/19 14:43:40, nednguyen wrote:
> These Exception all need to be fixed. 
> maybe just raised a single exception here:
DevToolsClientBackendConnectionError.
> Then the caller related to browser can catch this and raise Browser*
exceptions.

Agreed. I'm going to split InspectorJson to a separate patch. Expect this
cleanup there.

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:38:
def IsAvailable(self):
On 2014/12/19 14:43:40, nednguyen wrote:
> IsAlive(self)

Done.

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/tracing_controller_backend.py
(right):

https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/tracing_controller_backend.py:45: result
= browser_backend.StopTracing()
On 2014/12/19 14:43:40, nednguyen wrote:
> TracingController doesn't need the handle to browser_backend. It should only
> grab handle to tracing_backend

This logic should eventually look like
app_backend.devtools_client.StopChromeTracing() when we port most of the logic
there. I think this is a reasonably sane architecture for now, until we figure
out how to deal with the inherent platform vs app mismatch wrt tracing.

[slamm, 2014-12-22 20:35:38]

On 2014/12/19 20:51:21, chrishenry (OOO until Jan 5) wrote:
>
https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
> (right):
> 
>
https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:82:
> self._port)
> On 2014/12/19 19:26:55, ariblue wrote:
> > There are implicit assumptions in this function and in GetSystemInfo() below
> > that classes that inherit from ChromeBrowserBackend will define self._port.
Do
> > we need any sort of assertions or other protections that self._port is
defined
> > at this point?
> 
> Yes. (: Done (assert). I don't quite like the handling of self._port, but
don't
> want to expand scope as well...
> 
>
https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:238:
> def Request(self, path, timeout=30, throw_network_exception=False):
> On 2014/12/19 19:26:55, ariblue wrote:
> > This should be killed or updated to use the devtools_client.
> 
> Agreed. Will split to InspectorJson patch.
> 
>
https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
> File
>
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py
> (right):
> 
>
https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
>
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:17:
> class DevToolsClientBackend(object):
> On 2014/12/19 19:30:12, ariblue wrote:
> > On 2014/12/19 19:26:55, ariblue wrote:
> > > Does it make sense to split this into two classes?
> > > 
> > > * One which handles the underlying connection logic for devtools HTTP
> requests
> > > (tracks the port and handles the _RequestJson).
> > > * One that provides the API (GetChromeBranchNumber, StartTracing,
> > > ListInspectableContexts, etc).
> > 
> > Just saw your comment here:
> > https://code.google.com/p/chromium/issues/detail?id=423954#c29
> 
> I think you and Dave are right here. Having separate InspectorJson seems
> helpful, especially given that TabListBackend will also need it.
> 
> I'll look into getting this checked in separately.
> 
>
https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
>
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:36:
> raise exceptions.BrowserConnectionGoneException(self.browser, e)
> On 2014/12/19 14:43:40, nednguyen wrote:
> > These Exception all need to be fixed. 
> > maybe just raised a single exception here:
> DevToolsClientBackendConnectionError.
> > Then the caller related to browser can catch this and raise Browser*
> exceptions.
> 
> Agreed. I'm going to split InspectorJson to a separate patch. Expect this
> cleanup there.
> 
>
https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
>
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:38:
> def IsAvailable(self):
> On 2014/12/19 14:43:40, nednguyen wrote:
> > IsAlive(self)
> 
> Done.
> 
>
https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/platform/tracing_controller_backend.py
> (right):
> 
>
https://codereview.chromium.org/813283002/diff/40001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/platform/tracing_controller_backend.py:45:
result
> = browser_backend.StopTracing()
> On 2014/12/19 14:43:40, nednguyen wrote:
> > TracingController doesn't need the handle to browser_backend. It should only
> > grab handle to tracing_backend
> 
> This logic should eventually look like
> app_backend.devtools_client.StopChromeTracing() when we port most of the logic
> there. I think this is a reasonably sane architecture for now, until we figure
> out how to deal with the inherent platform vs app mismatch wrt tracing.

I chatted with Ned regarding the Chrome version check and he said it is not
needed after all.
I started a CL to remove the check and remove the chrome_browser_backend from
the TracingBackend arguments.

    [Telemetry] Drop check for devtools support for record-as-much-as-possible
(chrome >2118)
    https://codereview.chromium.org/808563005

[chromium-reviews, 2014-12-22 23:23:48]

As long as you've check that L webview is OK (either past the branch number
or doesn't require the check).

(from phone)
On Dec 19, 2014 12:51 PM, <chrishenry@google.com> wrote:

>
> https://codereview.chromium.org/813283002/diff/40001/
> tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
> File
> tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
> (right):
>
> https://codereview.chromium.org/813283002/diff/40001/
> tools/telemetry/telemetry/core/backends/chrome/chrome_
> browser_backend.py#newcode82
> tools/telemetry/telemetry/core/backends/chrome/chrome_
> browser_backend.py:82:
> self._port)
> On 2014/12/19 19:26:55, ariblue wrote:
>
>> There are implicit assumptions in this function and in GetSystemInfo()
>>
> below
>
>> that classes that inherit from ChromeBrowserBackend will define
>>
> self._port. Do
>
>> we need any sort of assertions or other protections that self._port is
>>
> defined
>
>> at this point?
>>
>
> Yes. (: Done (assert). I don't quite like the handling of self._port,
> but don't want to expand scope as well...
>
> https://codereview.chromium.org/813283002/diff/40001/
> tools/telemetry/telemetry/core/backends/chrome/chrome_
> browser_backend.py#newcode238
> tools/telemetry/telemetry/core/backends/chrome/chrome_
> browser_backend.py:238:
> def Request(self, path, timeout=30, throw_network_exception=False):
> On 2014/12/19 19:26:55, ariblue wrote:
>
>> This should be killed or updated to use the devtools_client.
>>
>
> Agreed. Will split to InspectorJson patch.
>
> https://codereview.chromium.org/813283002/diff/40001/
> tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_
> backend.py
> File
> tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_
> backend.py
> (right):
>
> https://codereview.chromium.org/813283002/diff/40001/
> tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_
> backend.py#newcode17
> tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_
> backend.py:17:
> class DevToolsClientBackend(object):
> On 2014/12/19 19:30:12, ariblue wrote:
>
>> On 2014/12/19 19:26:55, ariblue wrote:
>> > Does it make sense to split this into two classes?
>> >
>> > * One which handles the underlying connection logic for devtools
>>
> HTTP requests
>
>> > (tracks the port and handles the _RequestJson).
>> > * One that provides the API (GetChromeBranchNumber, StartTracing,
>> > ListInspectableContexts, etc).
>>
>
>  Just saw your comment here:
>> https://code.google.com/p/chromium/issues/detail?id=423954#c29
>>
>
> I think you and Dave are right here. Having separate InspectorJson seems
> helpful, especially given that TabListBackend will also need it.
>
> I'll look into getting this checked in separately.
>
> https://codereview.chromium.org/813283002/diff/40001/
> tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_
> backend.py#newcode36
> tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_
> backend.py:36:
> raise exceptions.BrowserConnectionGoneException(self.browser, e)
> On 2014/12/19 14:43:40, nednguyen wrote:
>
>> These Exception all need to be fixed.
>> maybe just raised a single exception here:
>>
> DevToolsClientBackendConnectionError.
>
>> Then the caller related to browser can catch this and raise Browser*
>>
> exceptions.
>
> Agreed. I'm going to split InspectorJson to a separate patch. Expect
> this cleanup there.
>
> https://codereview.chromium.org/813283002/diff/40001/
> tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_
> backend.py#newcode38
> tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_
> backend.py:38:
> def IsAvailable(self):
> On 2014/12/19 14:43:40, nednguyen wrote:
>
>> IsAlive(self)
>>
>
> Done.
>
> https://codereview.chromium.org/813283002/diff/40001/
> tools/telemetry/telemetry/core/platform/tracing_controller_backend.py
> File
> tools/telemetry/telemetry/core/platform/tracing_controller_backend.py
> (right):
>
> https://codereview.chromium.org/813283002/diff/40001/
> tools/telemetry/telemetry/core/platform/tracing_controller_backend.py#
> newcode45
> tools/telemetry/telemetry/core/platform/tracing_controller_backend.py:45:
> result = browser_backend.StopTracing()
> On 2014/12/19 14:43:40, nednguyen wrote:
>
>> TracingController doesn't need the handle to browser_backend. It
>>
> should only
>
>> grab handle to tracing_backend
>>
>
> This logic should eventually look like
> app_backend.devtools_client.StopChromeTracing() when we port most of the
> logic there. I think this is a reasonably sane architecture for now,
> until we figure out how to deal with the inherent platform vs app
> mismatch wrt tracing.
>
> https://codereview.chromium.org/813283002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chrishenry, 2015-01-10 01:25:03]

I'm closing this patch. Most of the changes here are already checked in (with
slightly different form) as of today, with another change in review. Thanks all!