1433139 - Hook up WebRender capture triggers to the browser console/hotkey

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Dzmitry Malyshau [:kvark]]

See the post about WR capturing: http://kvark.github.io/webrender/debug/ron/2018/01/23/wr-capture-infra.html

We need to have this integrated into FF nightly.

Comment 1

[Dzmitry Malyshau [:kvark]]

Attachment: WebRender Capture hookup

Te hookup targets the "wr-capture" local folder by default and captures the FRAME level on "Ctrl+Shift+3". There is a third piece missing from here - enabling "capture" feature on WR - to be provided as a separate patch.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d431c4a3776fbc4a68a3d67e608e08f8b52dd20e

Comment 2

[Markus Stange [:mstange]]

Comment on attachment 8945547 [details] [diff] [review]
WebRender Capture hookup

Review of attachment 8945547 [details] [diff] [review]:
-----------------------------------------------------------------

The C++ and rust parts look good to me. You'll need another reviewer for the front-end piece, though.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +995,5 @@
>       used as a textual label for the indicator used by assistive technology
>       users. -->
>  <!ENTITY accessibilityIndicator.tooltip "Accessibility Features Enabled">
> +
> +<!ENTITY wrCaptureCmd.commandKey "3">

This is not used. And it would probably better to not make this localizable at all, since it's nightly-only and only targeted at Gecko developers. So I recommend just removing this hunk from the patch.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1994,5 @@
>     */
>    void removeFromStyloBlocklist(in ACString aBlockedDomain);
>  
> +  /**
> +   * Capture the contents of the current WebRender frame.

maybe add "and save the capture to a file in the current working directory"?

::: gfx/webrender_bindings/WebRenderAPI.cpp
@@ +505,5 @@
> +WebRenderAPI::Capture()
> +{
> +  uint8_t bits = 3; //TODO: get from JavaScript
> +  const char *path = "wr-capture"; //TODO: get from JavaScript
> +  const char *border = "--------------------------\n";

Gecko style is to put the * hugging the type, not the variable name

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 8945547 [details] [diff] [review]
WebRender Capture hookup

Review of attachment 8945547 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/wr/WebRenderBridgeChild.cpp
@@ +691,5 @@
>  
> +void
> +WebRenderBridgeChild::Capture()
> +{
> +  this->SendCapture();

let's remove the "this->" here

Comment 4

[Dzmitry Malyshau [:kvark]]

Attachment: WR capture feature

Enabling "capture" feature on WR, which drags "RON" as an extra 3rd party dependency.

Comment 5

[Dzmitry Malyshau [:kvark]]

try push for the combined patch with 3rd party: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2be20d5a533603e3527613b9a12368e87e14f7f5

Comment 6

[Dzmitry Malyshau [:kvark]]

Attachment: WR capture hookup

Review comments addressed.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8945563 [details] [diff] [review]
WR capture feature

Review of attachment 8945563 [details] [diff] [review]:
-----------------------------------------------------------------

Please add the bug number and reviewer to the commit message. Patch looks good otherwise, thanks!

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8945565 [details] [diff] [review]
WR capture hookup

Review of attachment 8945565 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the browser parts! I'm not too worried about the keyboard shortcut conflicting on one of the platforms we support, but it's something we can always adjust and localize when it proves to be an issue in practice.

The idea here is to _only_ have a keyboard shortcut available, right? A menu item is also possible, but would require a little bit more work.

::: browser/base/content/browser-webrender.js
@@ +5,5 @@
> +// This file is loaded into the browser window scope.
> +/* eslint-env mozilla/browser-window */
> +
> +/**
> + * Communicates with WebRender debug triggers.

nit: maybe 'Global browser interface with the WebRender backend.' would be more generic and apt in this location?

@@ +9,5 @@
> + * Communicates with WebRender debug triggers.
> + */
> +var gWebRender = {
> +  capture() {
> +    window.QueryInterface(Ci.nsIInterfaceRequestor)

nit: let's start this file off 'perfect' and add a docblock comment for this method. Hoping all subsequent copy-paste operations will be done nicely ;-)

@@ +12,5 @@
> +  capture() {
> +    window.QueryInterface(Ci.nsIInterfaceRequestor)
> +          .getInterface(Ci.nsIDOMWindowUtils)
> +          .wrCapture();
> +  },

nit: If ESLint requires it nowadays, you can leave it of course, but I'm not a fan of dangling comma's...

Comment 9

[Dzmitry Malyshau [:kvark]]

Attachment: WebRender Capture hookup

Thanks for your comments! Notes are now addressed.

We will want to expand the frontend support in the following directions later:
  - have UI to trigger, presumably via a plugin like Gecko Profiler
  - configure the path and parameters from UI

But for now, just having the key is super important.

Comment 10

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8945776 [details] [diff] [review]
WebRender Capture hookup

Marking as reviewed - the minor concerns are addressed, no functional changes.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

I'll land this

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/274c63c0f54e
Enable the WR capture feature in webrender_bindings. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/75b5af791006
WebRender capture integration on Nightly. r=mikedeboer

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Backed out the second patch for failing mochitest-chrome-3 on OS X debug (maybe also other places). Failure log:

https://treeherder.mozilla.org/logviewer.html#?job_id=158693435&repo=mozilla-inbound&lineNumber=19139

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/617df9c9618b2e754029585556f3150766922d16

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Same failure happened on windows. See https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=75b5af7910068a6fe04c61b267d985c80362e49d&filter-searchStr=c3&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable

Comment 15

[Dzmitry Malyshau [:kvark]]

Attachment: WebRender Capture hookup

Added a bit of code to disable Wr capture key during chrome tests.
Pending try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55c5d17a676bd5fd869fb79124e2a47d4219ae24

Comment 16

[Dzmitry Malyshau [:kvark]]

Attachment: WR capture hookup

Updated try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cf2185a29b12df83f77ee7a6be0289723a1c6ce

Comment 17

[Mike de Boer [:mikedeboer]]

Dzimitry, all you need to do is change the key definition to:

```xul
<key id="key_wrCaptureCmd" key="3" command="wrCaptureCmd" modifiers="control,shift"/>
```

...and the test will pass as well, whilst keeping functionality the same.

Comment 18

[Mike de Boer [:mikedeboer]]

@kats Please feel free to reland with the change I mentioned in comment 17.

Comment 19

[Dzmitry Malyshau [:kvark]]

Mike, this key combination (that you propose) doesn't work on Linux.

Comment 20

[Mike de Boer [:mikedeboer]]

Oh? But AFAICT this should be equivalent... so how come it doesn't work on Linux?

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Here's a try push with the change from comment 17: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f22b7dadadbf25f3be353bc1bf27fc8ca4bd0d65

Comment 22

[Dzmitry Malyshau [:kvark]]

>Oh? But AFAICT this should be equivalent... so how come it doesn't work on Linux?

I wish I knew. It just doesn't trigger. When we were trying this, we figured that "#" key works on Linux (being Shift + "3" technically), but it fails on OSX. So `VK_3` variant seems to solve that and works on both Linux and OSX.

Comment 23

[Mike de Boer [:mikedeboer]]

In that case, let's go with:

```xul
#ifdef NIGHTLY_BUILD
    <key id="key_wrCaptureCmd"
#ifdef XP_LINUX
    keycode="VK_3" event="keydown"
#else
    key="3"
#endif
    command="wrCaptureCmd" modifiers="control,shift"/>
#endif
```

...since we didn't see the test failure on Linux either. There's just something about 'VK_3' that makes OSX and Windows go *burp*. Welcome to the wonderful world of frontend :-P

Comment 24

[Dzmitry Malyshau [:kvark]]

Attachment: WR capture hookup

Removed the test changes and instead made the key configuration different on Linux vs non-Linux, as advised.
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0c13d10a0325f547684c74857fa77a30500be72

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cc5da48de86
WebRender capture integration on Nightly. r=mikedeboer

Comment 26

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/274c63c0f54e
https://hg.mozilla.org/mozilla-central/rev/2cc5da48de86

Comment 27

[Dzmitry Malyshau [:kvark]]

It appears that Windows key didn't work out. Need extra patches.

Comment 28

[Dzmitry Malyshau [:kvark]]

Attachment: WR capture hookup - part 2

A different version of key bindings. Appears to work on all platforms and passes the tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0241b237a28530d4be3d98d0fc4da5fb3c49c4c&selectedJob=159264337

Comment 29

[Mike de Boer [:mikedeboer]]

Comment on attachment 8946646 [details] [diff] [review]
WR capture hookup - part 2

Review of attachment 8946646 [details] [diff] [review]:
-----------------------------------------------------------------

Tested locally, widget/tests/test_keycodes.xul passes as well. r=me!

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/031ccad355e4
Follow-up to adjust keybindings to work on Windows. r=mikedeboer

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/031ccad355e4

Comment 32

[Markus Stange [:mstange]]

Comment on attachment 8946646 [details] [diff] [review]
WR capture hookup - part 2

Review of attachment 8946646 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-sets.inc
@@ +386,5 @@
>  
>  #ifdef NIGHTLY_BUILD
>      <key id="key_wrCaptureCmd"
> +#ifdef XP_MACOSX
> +    keycode="3" modifiers="control,shift"

keycode="3" should have been keycode="VK_3"

This now catches Ctrl+Shift+[any key], so I can't use the profiler any more without crashing the browser due to bug 1433932.

Comment 33

[Markus Stange [:mstange]]

Er, that was wrong, it should instead be key="3"

Comment 34

[Dzmitry Malyshau [:kvark]]

Well this is not great.

Comment 35

[Dzmitry Malyshau [:kvark]]

Attachment: Fixed binding on OSX

Note: I'll not be able to test this today, so take this at your own risk. Fortunately, only affects OSX.

Comment 36

[Markus Stange [:mstange]]

Comment on attachment 8947542 [details] [diff] [review]
Fixed binding on OSX

Review of attachment 8947542 [details] [diff] [review]:
-----------------------------------------------------------------

I've confirmed that this patch works as expected on macOS.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b593176e9b0b
fix OSX key binding for WebRender capture. r=mstange

Comment 38

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b593176e9b0b