Bug 128150 – Web Replay: route through UserInputBridge when delivering user inputs to WebCore

• Component: WebKit2
• Status: RESOLVED FIXED

[Blaze Burg]

This is one of the few parts of web replay that cannot be easily put behind a guard. For now, the methods inside InputProxy don't have any replay code paths, as that requires machinery that hasn't landed yet.

[Blaze Burg]

Created attachment 223059 [details]
patch

[Blaze Burg]

RFC: should this patch forward user inputs for WK2 and PLATFORM(MAC) only, or should I go dig for all uses of the user input entry points? I could see the use of replaying Mac WK1, but for other ports it seems like a waste of time unless web replay is intended to support those platforms (or unless InputProxy becomes useful to non-replay code for refactoring reasons).

[Blaze Burg]

Created attachment 223137 [details]
patch2

[Blaze Burg]

Created attachment 223721 [details]
rebased patch

[Andreas Kling]

I'm very excited about this feature, but we need to talk about the abstraction layer added here (CC'ing people.)

[Blaze Burg]

(In reply to comment #5)
> I'm very excited about this feature, but we need to talk about the abstraction layer added here (CC'ing people.)

Sure thing. (For reference, an older version of InputProxy.cpp with the replay parts added is viewable here: https://github.com/burg/timelapse/blob/timelapse/Source/WebCore/replay/ReplayProxy.cpp)

[Blaze Burg]

Update:

After much bikeshedding over the last few weeks, I think the name UserInputBridge or UserInputSwitch best communicate what this abstraction does: It lets inputs selectively flow from WK/WK2 to WebCore. 

When capturing, the inputs are tee'd into the replay log and EventHandler/FocusController/wherever they used to go. When replaying, the 'bridge' is up (or, circuit broken) and user inputs don't come from WK/WK2, only from the replay log and internally.

These names avoid the following overloaded/vague terms: Shim, Proxy, Mediator, Connection, Channel. I think some others have suggested Switchboard and InputTap, but those are less evocative IMO.


As for the necessity of adding yet another abstraction, the alternatives are less appealing because they distribute 'bridge' logic across many callsites in WebKit2 and/or many callees in WebCore.

Alternative 1: inside every relevant input-receiving callee in WebCore, add:

 * an optional argument to distinguish the source of the input (WC or WK2)
 * check whether we are capturing or replaying.
   - If capturing, save the user input if it's external (from WK2).
   - If replaying, ignore the user input if it's external (from WK2), and handle normally otherwise (from recording or WebCore).

Alternative 2: at every relevant input-delivering callsite in WebProcess, implement the above, but only for the external case.

Either alternative requires duplicating the delivery code (e.g., corePage()->focusController()->focusedOrMainFrame()->eventHandler()->handleMouseMove(...)) between the WK2 callsite and the replay input's dispatch() method, which could lead to subtle bitrot.

[Timothy Hatcher]

I like UserInputBridge.

[Blaze Burg]

Created attachment 224984 [details]
rebased, renamed to UserInputBridge

[Timothy Hatcher]

Comment on attachment 224984 [details]
rebased, renamed to UserInputBridge

Looks good to me. Please fix the other platform builds before landing though.

[Blaze Burg]

(In reply to comment #10)
> (From update of attachment 224984 [details])
> Looks good to me. Please fix the other platform builds before landing though.

OK. I haven't been able to get a useful GTK or EFL build attempt in a few weeks though.

[Blaze Burg]

Created attachment 225075 [details]
try EWS again

[Joseph Pecoraro]

Comment on attachment 225075 [details]
try EWS again

View in context: https://bugs.webkit.org/attachment.cgi?id=225075&action=review

Also looks good to me.

> Source/WebCore/replay/UserInputBridge.h:54
> +    bool handleContextMenuEvent(const PlatformMouseEvent&, const Frame*, bool fromReplay = false);
> +    bool handleMousePressEvent(const PlatformMouseEvent&, bool fromReplay = false);
> +    bool handleMouseReleaseEvent(const PlatformMouseEvent&, bool fromReplay = false);
> +    bool handleMouseMoveEvent(const PlatformMouseEvent&, bool fromReplay = false);
> +    bool handleMouseMoveOnScrollbarEvent(const PlatformMouseEvent&, bool fromReplay = false);

When the fromReplay parameter is actually used, it would read better if it was an enum instead of a bool. WebKit style is typically to use an enum instead of booleans where possible to avoid confusing parameter values at call sites. For example: foo(true, false, true);

[Blaze Burg]

Committed r164745: <http://trac.webkit.org/changeset/164745>