Closed Bug 1499828 Opened 11 months ago Closed 11 months ago

Show overlay with recording position and progress bar

Categories

(Core :: Web Replay, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

Attachments

(7 files)

When rewinding or jumping around in a tab's recording, a lot of the time it isn't obvious where one is in the recording, or whether any progress is being made.  When playing forward to the end of the recording, it also isn't obvious when the tab resumes recording and can be interacted with.

The attached patch adds a simple overlay that is shown over a recording/replaying tab whenever the tab's child process is replaying: the process is before the end of the recording, or is paused at the end of the recording.  The overlay shows a percentage of the recording that has been reached so far, and a progress bar that is currently always empty.  The latter could be removed, but I left it in to have a more complete appearance for the overlay and because pretty soon I want to use this bar to show progress towards rewinding to the next checkpoint (at which point the progress percentage will update) or jumping to a target execution point.  The overlay can also be dragged around the screen.
Attached patch patchSplinter Review
Attached image screen shot
Add a method, RecordReplayControl.recordingPosition(), which can be used in the middleman to get the current position in the recording.
Attachment #9018000 - Flags: review?(continuation)
Add an overlay which is shown when the current child is replaying, per the description in comment 0.
Attachment #9018002 - Flags: review?(lsmyth)
Call the UpdateOverlay JS method defined in part 2 when required: the active child changes (affecting the overlay's visibility) or an active replaying child reaches a checkpoint (affecting the percentage progress shown).
Attachment #9018003 - Flags: review?(continuation)
When the active child is replaying, we currently ignore all incoming events.  In order for the overlay (which lives in the middleman's window) to be draggable, the middleman process needs to handle mouse events.  The attached patch processes mouse move and button events in the middleman when the active child is replaying.  At other times, the overlay won't be visible and the events should go to the recording child.
Attachment #9018004 - Flags: review?(continuation)
If the debugger is paused, the overlay should be draggable, but the devtools server code running in the middleman process prevents this by suppressing all events on the window which the server code is running against.  This makes sense for the normal server use case (where the server is living in the same process as the debuggee and we don't want new events to be processed by the debuggee) but is unnecessary if the server lives in the middleman process instead.
Attachment #9018006 - Flags: review?(lsmyth)
Comment on attachment 9018002 [details] [diff] [review]
Part 2 - Show overlay when the current child is replaying.

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

The drag math logic is a little hard to follow for me, but it does seem reasonable. My comments are really mostly nits, so your call.

::: devtools/server/actors/replay/graphics.js
@@ +55,5 @@
> +  progressBar.getContext("2d").fillRect(0, 0, 100, 5);
> +  progressBar.style.padding = "5px 5px 5px 5px";
> +
> +  overlay.appendChild(progressBar);
> +  window.document.body.insertBefore(overlay, window.document.body.firstChild);

FYI, there is a `prepend` in DOM4 to make this easier, e.g.

    window.document.body.prepend(overlay);

Same for the other case below.

@@ +76,5 @@
> +    let newTop = () => overlay.offsetTop + diffy;
> +    if (newTop() < 0) {
> +      diffy -= newTop();
> +    }
> +    let maxTop = canvas.height / window.devicePixelRatio;

I've personally always had trouble using `devicePizelRatio`. Do you think it would be clearer to use `parseFloat(window.getComputedStyle(canvas).height)`, which I _think_ would avoid the need for it?
Attachment #9018002 - Flags: review?(lsmyth) → review+
Attachment #9018006 - Flags: review?(lsmyth) → review+
Attachment #9018000 - Flags: review?(continuation) → review+
Comment on attachment 9018003 [details] [diff] [review]
Part 3 - Redraw the overlay when required.

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

::: toolkit/recordreplay/ipc/ParentGraphics.cpp
@@ +191,5 @@
>    args[3].setBoolean(hadFailure);
>  
>    // Call into the graphics module to update the canvas it manages.
>    RootedValue rval(cx);
> +  if (!JS_CallFunctionName(cx, *gGraphicsSandbox, "UpdateCanvas", args, &rval)) {

Was this just a bug before?
Attachment #9018003 - Flags: review?(continuation) → review+
Comment on attachment 9018004 [details] [diff] [review]
Part 4 - Handle mouse events in the middleman when the active child is replaying.

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

Maybe you could take a look at this, Olli? I don't know if there are other kinds of input events that we might care about.

::: toolkit/recordreplay/ipc/ParentForwarding.cpp
@@ +92,5 @@
>      return true;
>    }
>  
> +  // Handle messages that should be sent to the middleman when the active child
> +  // is replaying.

Maybe say why we need the messages?
Attachment #9018004 - Flags: review?(continuation)
Attachment #9018004 - Flags: review+
Attachment #9018004 - Flags: feedback?(bugs)
(In reply to Logan Smyth [:loganfsmyth] from comment #8)
> @@ +76,5 @@
> > +    let newTop = () => overlay.offsetTop + diffy;
> > +    if (newTop() < 0) {
> > +      diffy -= newTop();
> > +    }
> > +    let maxTop = canvas.height / window.devicePixelRatio;
> 
> I've personally always had trouble using `devicePizelRatio`. Do you think it
> would be clearer to use
> `parseFloat(window.getComputedStyle(canvas).height)`, which I _think_ would
> avoid the need for it?

Hmm, we already use window.devicePixelRatio when setting the style for the canvas in updateWindowCanvas, so the two should be equivalent.  I'll experiment but at least changing it here would allow this code to continue to work even if how we compute the canvas style changes.
(In reply to Andrew McCreight [:mccr8] from comment #9)
> Comment on attachment 9018003 [details] [diff] [review]
> Part 3 - Redraw the overlay when required.
> 
> Review of attachment 9018003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/recordreplay/ipc/ParentGraphics.cpp
> @@ +191,5 @@
> >    args[3].setBoolean(hadFailure);
> >  
> >    // Call into the graphics module to update the canvas it manages.
> >    RootedValue rval(cx);
> > +  if (!JS_CallFunctionName(cx, *gGraphicsSandbox, "UpdateCanvas", args, &rval)) {
> 
> Was this just a bug before?

No, part 2 renamed this function to distinguish it from the new UpdateOverlay function.
(In reply to Brian Hackett (:bhackett) from comment #11)
> (In reply to Logan Smyth [:loganfsmyth] from comment #8)
> > @@ +76,5 @@
> > > +    let newTop = () => overlay.offsetTop + diffy;
> > > +    if (newTop() < 0) {
> > > +      diffy -= newTop();
> > > +    }
> > > +    let maxTop = canvas.height / window.devicePixelRatio;
> > 
> > I've personally always had trouble using `devicePizelRatio`. Do you think it
> > would be clearer to use
> > `parseFloat(window.getComputedStyle(canvas).height)`, which I _think_ would
> > avoid the need for it?
> 
> Hmm, we already use window.devicePixelRatio when setting the style for the
> canvas in updateWindowCanvas, so the two should be equivalent.  I'll
> experiment but at least changing it here would allow this code to continue
> to work even if how we compute the canvas style changes.

Ah I see that now. Alright, no problem then. I didn't realize this was meant to pair up with existing code that was already using it.
Comment on attachment 9018004 [details] [diff] [review]
Part 4 - Handle mouse events in the middleman when the active child is replaying.

Should we send all the input events to middleman while replaying?
Attachment #9018004 - Flags: feedback?(bugs) → feedback+
(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 9018004 [details] [diff] [review]
> Part 4 - Handle mouse events in the middleman when the active child is
> replaying.
> 
> Should we send all the input events to middleman while replaying?

We could, but so far at least we only need the mouse events for the overlay to work (which is the only thing that a user can directly interact with in a middleman process).  Is there a simple way to test for whether an IPDL message is an input event?
Depends on: 1496881
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65557267db2a
Part 1 - Add API for getting the current recording position, r=mccr8.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc47411763c5
Part 2 - Show overlay when the current child is replaying, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/97475fb566ea
Part 3 - Redraw the overlay when required, r=mccr8.
https://hg.mozilla.org/integration/mozilla-inbound/rev/925e342d29d8
Part 4 - Handle input events in the middleman when the active child is replaying, r=mccr8.
https://hg.mozilla.org/integration/mozilla-inbound/rev/18f2ea8c2a19
Part 5 - Don't suppress events when pausing in the middleman, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae91c0db8005
Part 6 - Avoid hiding test graphics under overlay, r=test_only.
Depends on: 1504835
You need to log in before you can comment on or make changes to this bug.