Closed Bug 1433139 Opened 2 years ago Closed 2 years ago

Hook up WebRender capture triggers to the browser console/hotkey

Categories

(Firefox :: General, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: kvark, Assigned: kvark)

References

Details

Attachments

(4 files, 5 obsolete files)

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.
Assignee: nobody → kvark
Attached patch WebRender Capture hookup (obsolete) — Splinter Review
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
Attachment #8945547 - Flags: review?(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
Attachment #8945547 - Flags: review?(mstange) → review+
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
Enabling "capture" feature on WR, which drags "RON" as an extra 3rd party dependency.
Attachment #8945563 - Flags: review?(bugmail)
Attached patch WR capture hookup (obsolete) — Splinter Review
Review comments addressed.
Attachment #8945547 - Attachment is obsolete: true
Attachment #8945565 - Flags: review?(mstange)
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!
Attachment #8945563 - Flags: review?(bugmail) → review+
Attachment #8945565 - Flags: review?(mdeboer)
Attachment #8945565 - Flags: review?(mstange) → review+
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...
Attachment #8945565 - Flags: review?(mdeboer) → review+
Attached patch WebRender Capture hookup (obsolete) — Splinter Review
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.
Attachment #8945565 - Attachment is obsolete: true
Attachment #8945776 - Flags: review?(mdeboer)
Comment on attachment 8945776 [details] [diff] [review]
WebRender Capture hookup

Marking as reviewed - the minor concerns are addressed, no functional changes.
Attachment #8945776 - Flags: review?(mdeboer) → review+
Keywords: checkin-needed
Pushed by kgupta@mozilla.com:
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
Status: NEW → ASSIGNED
Component: Developer Tools → General
Attached patch WebRender Capture hookup (obsolete) — Splinter Review
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
Attachment #8945776 - Attachment is obsolete: true
Attachment #8945848 - Flags: review?(mconley)
Attached patch WR capture hookup (obsolete) — Splinter Review
Updated try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cf2185a29b12df83f77ee7a6be0289723a1c6ce
Attachment #8945848 - Attachment is obsolete: true
Attachment #8945848 - Flags: review?(mconley)
Attachment #8945849 - Flags: review?(mconley)
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.
Flags: needinfo?(kvark)
Attachment #8945849 - Flags: review?(mconley)
@kats Please feel free to reland with the change I mentioned in comment 17.
Mike, this key combination (that you propose) doesn't work on Linux.
Flags: needinfo?(kvark)
Oh? But AFAICT this should be equivalent... so how come it doesn't work on Linux?
>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.
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
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
Attachment #8945849 - Attachment is obsolete: true
Attachment #8945894 - Flags: review?(mdeboer)
Attachment #8945894 - Flags: review?(mdeboer) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cc5da48de86
WebRender capture integration on Nightly. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/274c63c0f54e
https://hg.mozilla.org/mozilla-central/rev/2cc5da48de86
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
It appears that Windows key didn't work out. Need extra patches.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attachment #8946646 - Flags: review?(mdeboer)
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!
Attachment #8946646 - Flags: review?(mdeboer) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/031ccad355e4
Follow-up to adjust keybindings to work on Windows. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/031ccad355e4
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
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.
Er, that was wrong, it should instead be key="3"
Well this is not great.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Note: I'll not be able to test this today, so take this at your own risk. Fortunately, only affects OSX.
Attachment #8947542 - Flags: review?(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.
Attachment #8947542 - Flags: review?(mstange) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b593176e9b0b
fix OSX key binding for WebRender capture. r=mstange
https://hg.mozilla.org/mozilla-central/rev/b593176e9b0b
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.