[telemetry] Add support for requesting memory dumps via DevTools API

[telemetry] Add support for requesting memory dumps via DevTools API

This (together with the proposed Tracing.dumpMemory DevTools API, see
https://docs.google.com/document/d/190-URzSjfsiXNeyke86z85-5ctuqsC_RoX1J1uJTwIQ/edit?usp=sharing)
will allow telemetry memory page interactions to request memory dumps
as follows:

  action_runner.tab.browser.DumpMemory()

BUG=505826
Committed: https://crrev.com/c1110689abeb479d17c170f28c2ce04543182bdb
Cr-Commit-Position: refs/heads/master@{#340943}

[petrcermak, 2015-07-14 17:22:25]

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

[petrcermak, 2015-07-14 17:22:26]

Hi, please review this patch. I will add tests if you're happy with it.

Thanks,
Petr

https://codereview.chromium.org/1224083015/diff/1/tools/telemetry/telemetry/i...
File tools/telemetry/telemetry/internal/backends/browser_backend.py (right):

https://codereview.chromium.org/1224083015/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/backends/browser_backend.py:81: def
devtools_client(self):
All implementations of this abstract class already seem to have this property,
but let's make it explicit.

[nednguyen, 2015-07-15 17:53:00]

https://codereview.chromium.org/1224083015/diff/1/tools/telemetry/telemetry/i...
File tools/telemetry/telemetry/internal/backends/browser_backend.py (right):

https://codereview.chromium.org/1224083015/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/backends/browser_backend.py:79: 
Instead, you should add: DumpMemory & supports_memory_dumps methods.

https://codereview.chromium.org/1224083015/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/backends/browser_backend.py:81: def
devtools_client(self):
On 2015/07/14 17:22:26, petrcermak wrote:
> All implementations of this abstract class already seem to have this property,
> but let's make it explicit.

Hmh, this should go to chrome_browser_backend. The devtool is a implementation
detail of chrome-like browser.

[petrcermak, 2015-07-15 18:56:53]

Thanks for your comments. PTAL.

Petr

[nednguyen, 2015-07-15 21:23:23]

On 2015/07/15 18:56:53, petrcermak wrote:
> Thanks for your comments. PTAL.
> 
> Petr

This is landable. Can you add unittest?

[nednguyen, 2015-07-15 23:16:30]

https://codereview.chromium.org/1224083015/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/backends/browser_backend.py (right):

https://codereview.chromium.org/1224083015/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/backends/browser_backend.py:106: def
supports_memory_dumps(self):
Actually this should be "support_memory_dumping"?

[petrcermak, 2015-07-16 10:29:21]

Patchset #3 (id:40001) has been deleted

[petrcermak, 2015-07-16 10:33:01]

I added 2 unit tests and 1 browser test to tracing_backend_unittest.py.
Unfortunately, the browser test has to remain disabled until the new method is
added to the DevTools API. It seemed unnecessary to test the new methods in the
other classes because they are just plumbing (and the new browser test will
check the plumbing all the way from Browser once enabled).

Thanks,
Petr

https://codereview.chromium.org/1224083015/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/backends/browser_backend.py (right):

https://codereview.chromium.org/1224083015/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/backends/browser_backend.py:106: def
supports_memory_dumps(self):
On 2015/07/15 23:16:30, nednguyen wrote:
> Actually this should be "support_memory_dumping"?

Done. All properties in this file are prefixed with "supports_", so I renamed it
to "supports_memory_dumping" to be consistent.

[Primiano Tucci (use gerrit), 2015-07-16 10:47:53]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-07-16 10:47:54]

https://codereview.chromium.org/1224083015/diff/60001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/backends/browser_backend.py (right):

https://codereview.chromium.org/1224083015/diff/60001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/backends/browser_backend.py:109: def
DumpMemory(self, timeout=web_contents.DEFAULT_WEB_CONTENTS_TIMEOUT):
Shouldn't you pass also the dumper_config (by default="") argument through, so
when we have light/heavy dumps you can control them?

[petrcermak, 2015-07-16 13:35:33]

Thanks for your comments. PTAL.

Petr

https://codereview.chromium.org/1224083015/diff/60001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/backends/browser_backend.py (right):

https://codereview.chromium.org/1224083015/diff/60001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/backends/browser_backend.py:109: def
DumpMemory(self, timeout=web_contents.DEFAULT_WEB_CONTENTS_TIMEOUT):
On 2015/07/16 10:47:54, Primiano Tucci wrote:
> Shouldn't you pass also the dumper_config (by default="") argument through, so
> when we have light/heavy dumps you can control them?

Done.

[petrcermak, 2015-07-16 19:03:14]

https://codereview.chromium.org/1224083015/diff/60001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/backends/browser_backend.py (right):

https://codereview.chromium.org/1224083015/diff/60001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/backends/browser_backend.py:109: def
DumpMemory(self, timeout=web_contents.DEFAULT_WEB_CONTENTS_TIMEOUT):
On 2015/07/16 13:35:33, petrcermak wrote:
> On 2015/07/16 10:47:54, Primiano Tucci wrote:
> > Shouldn't you pass also the dumper_config (by default="") argument through,
so
> > when we have light/heavy dumps you can control them?
> 
> Done.

I removed the argument after today's discussion.

[nednguyen, 2015-07-16 19:54:17]

On 2015/07/16 19:03:14, petrcermak wrote:
>
https://codereview.chromium.org/1224083015/diff/60001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/internal/backends/browser_backend.py (right):
> 
>
https://codereview.chromium.org/1224083015/diff/60001/tools/telemetry/telemet...
> tools/telemetry/telemetry/internal/backends/browser_backend.py:109: def
> DumpMemory(self, timeout=web_contents.DEFAULT_WEB_CONTENTS_TIMEOUT):
> On 2015/07/16 13:35:33, petrcermak wrote:
> > On 2015/07/16 10:47:54, Primiano Tucci wrote:
> > > Shouldn't you pass also the dumper_config (by default="") argument
through,
> so
> > > when we have light/heavy dumps you can control them?
> > 
> > Done.
> 
> I removed the argument after today's discussion.

lgtm

[petrcermak, 2015-07-24 14:39:23]

This patch was superseded by https://codereview.chromium.org/1257693002/. We
decided to extend the API exposed by the existing memory benchmarking extension
instead of augmenting the DevTools API.

[petrcermak, 2015-07-29 15:33:19]

nednguyen: After some discussions we decided to go back to the DevTools
solution. I'm really sorry for the confusion and extra work (the DevTools API
has fortunately landed already, so there should be no more surprises). Could you
please have one more quick look at this patch if it's still OK to land (I made a
few changes)?

Thanks,
Petr

[nednguyen, 2015-07-29 16:07:41]

No worry, change of direction does happen :-)

lgtm

https://codereview.chromium.org/1224083015/diff/120001/tools/telemetry/teleme...
File
tools/telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend_unittest.py
(right):

https://codereview.chromium.org/1224083015/diff/120001/tools/telemetry/teleme...
tools/telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend_unittest.py:164:
# Check that dumping memory before tracing starts raises an exception.
This is interesting. Dumping memory command can only work when tracing is
running?

[petrcermak, 2015-07-29 16:15:05]

https://codereview.chromium.org/1224083015/diff/120001/tools/telemetry/teleme...
File
tools/telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend_unittest.py
(right):

https://codereview.chromium.org/1224083015/diff/120001/tools/telemetry/teleme...
tools/telemetry/telemetry/internal/backends/chrome_inspector/tracing_backend_unittest.py:164:
# Check that dumping memory before tracing starts raises an exception.
On 2015/07/29 16:07:41, nednguyen wrote:
> This is interesting. Dumping memory command can only work when tracing is
> running?

Yes. The reason is that the dump is added to the trace (so there's no place to
put it if tracing is not running). The request only returns the GUID of the
generated dump (not the whole large dump).

[petrcermak, 2015-07-29 16:15:18]

The CQ bit was checked by petrcermak@chromium.org

[commit-bot: I haz the power, 2015-07-29 16:16:04]

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

[commit-bot: I haz the power, 2015-07-29 19:09:02]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-07-29 19:10:12]

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