Bug 135249 - Web Inspector: convert ReplayManager to a promise-based API
Summary: Web Inspector: convert ReplayManager to a promise-based API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks: 135608
  Show dependency treegraph
 
Reported: 2014-07-24 12:37 PDT by Brian Burg
Modified: 2014-08-06 11:30 PDT (History)
4 users (show)

See Also:


Attachments
Patch (24.30 KB, patch)
2014-07-24 13:03 PDT, Brian Burg
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-07-24 12:37:50 PDT
This allows the client code to handle fewer replay state cases because the replay manager can asynchronously "make it so". It also makes capture/replay more robust, because the manager can asynchronously disable breakpoints, continue the debugger, etc. before carrying out commands.

For example, if a startCapturing() command is issued during replay, the manager can construct a promise chain to stop replaying, unload the current segment, and start capturing. Without an async promise API, each client would have to manually get the replay engine to an acceptable state.
Comment 1 Radar WebKit Bug Importer 2014-07-24 12:38:27 PDT
<rdar://problem/17798570>
Comment 2 Brian Burg 2014-07-24 13:03:49 PDT
Created attachment 235448 [details]
Patch
Comment 3 Timothy Hatcher 2014-08-05 11:06:40 PDT
Comment on attachment 235448 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/ReplayManager.js:231
> +    switchSession: function(sessionId) { // --> ()

What is the "// --> ()" for?

> Source/WebInspectorUI/UserInterface/Controllers/ReplayManager.js:253
> +            })
> +            .then(function ensureSessionDataIsLoaded(session) {

I like these on the same line.

> Source/WebInspectorUI/UserInterface/Views/ReplayDashboardView.js:124
> +        }).then(function() {

Yay! Same line.
Comment 4 Brian Burg 2014-08-05 12:47:22 PDT
Comment on attachment 235448 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/ReplayManager.js:231
>> +    switchSession: function(sessionId) { // --> ()
> 
> What is the "// --> ()" for?

This is my (crappy) syntax for documenting which functions return a promise, and what the fulfilled promise value is (if any). Contrast to the getSegment() function.

>> Source/WebInspectorUI/UserInterface/Controllers/ReplayManager.js:253
>> +            .then(function ensureSessionDataIsLoaded(session) {
> 
> I like these on the same line.

OK
Comment 5 Brian Burg 2014-08-06 11:03:29 PDT
If it's okay with you, I'm going to land this without the UI changes, since those aren't reviewed yet.
Comment 6 Brian Burg 2014-08-06 11:30:17 PDT
Committed r172161: <http://trac.webkit.org/changeset/172161>