Bug 130145 - Web Inspector: add frontend controller and models for replay sessions
Summary: Web Inspector: add frontend controller and models for replay sessions
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: BJ Burg
URL:
Keywords: InRadar
Depends on: 129217
Blocks: 129692
  Show dependency treegraph
 
Reported: 2014-03-12 11:24 PDT by BJ Burg
Modified: 2014-03-21 05:33 PDT (History)
5 users (show)

See Also:


Attachments
the patch (56.64 KB, patch)
2014-03-19 16:45 PDT, BJ Burg
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2014-03-12 11:24:46 PDT
Prerequisite to further UI work.
Comment 1 BJ Burg 2014-03-19 16:45:07 PDT
Created attachment 227234 [details]
the patch
Comment 2 WebKit Commit Bot 2014-03-19 16:47:43 PDT
Attachment 227234 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/CodeGeneratorInspector.py:48:  whitespace before '}'  [pep8/E202] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Radar WebKit Bug Importer 2014-03-19 16:48:28 PDT
<rdar://problem/16373707>
Comment 4 Radar WebKit Bug Importer 2014-03-19 16:50:54 PDT
<rdar://problem/16373726>
Comment 5 Joseph Pecoraro 2014-03-20 16:39:24 PDT
Comment on attachment 227234 [details]
the patch

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

r=me

> Source/WebInspectorUI/ChangeLog:34
> +        (WebInspector.ReplayManager.prototype.getSession.get var):
> +        (WebInspector.ReplayManager.prototype.getSegment.get var):

I wonder what prepare-ChangeLog is doing creating these lines. There are lots of unnecessary lines in this ChangeLog =(.

> Source/JavaScriptCore/inspector/scripts/CodeGeneratorInspector.py:48
> +    "Replay": "WEB_REPLAY"

Nit: Following comma! It makes future patches suave, without a - line =)

> Source/WebInspectorUI/UserInterface/Base/Test.js:39
> +    if (InspectorBackend.registerReplayDispatcher)
> +        InspectorBackend.registerReplayDispatcher(new WebInspector.ReplayObserver);

No need for the if check in Test.js right? That is tied to tip of tree.

> Source/WebInspectorUI/UserInterface/Base/Test.js:129
> +    this.evaluateInPage("InspectorTestProxy.debugLog(unescape('" + escape(JSON.stringify(message)) + "'))");

What is going on here? With the single quotes this will be a literal string. Did this intend to be '"' + ... + '"'?

> Source/WebInspectorUI/UserInterface/Base/Test.js:163
> +        this.evaluateInPage("InspectorTestProxy.addResult(unescape('" + escape(text) + "'))");

Same.

> Source/WebInspectorUI/UserInterface/Base/Test.js:199
> +    return PageAgent.reload.promise(!!shouldIgnoreCache)
> +    .then(function() {
> +        this._shouldResendResults = true;
> +        this._testPageIsReloading = true;
> +
> +        return Promise.resolve(null);
> +    });

Style: I like the style you used later with promises. Indent the .then(function() { ... }).

> Source/WebInspectorUI/UserInterface/Controllers/ReplayManager.js:41
> +    // THese hold promises that resolve when the instance data is recieved.

Typo: "THese" => "These"

> Source/WebInspectorUI/UserInterface/Controllers/ReplayManager.js:257
> +                var removedSession = manager._sessions.take(sessionId);

Nit: console.assert(removedSession);

> Source/WebInspectorUI/UserInterface/Controllers/ReplayManager.js:280
> +        this._segmentPromises.delete(segmentId);

Can this._segmentPromises(segmentId) potentially have unresolved/unrejected promises at this point? If that is the case, can we carry over the promises to the new segment promise? Maybe the segmentId can change here, so that might not be possible.

> Source/WebInspectorUI/UserInterface/Models/ReplaySession.js:60
> +        ReplayAgent.getSerializedSession.promise(this.identifier)
> +            .then(this._updateFromPayload.bind(this));

I would expect this to use WebInspector.ReplayManager getSession to share the same getSerializedSession calls instead of repeating the backend work.

> Source/WebInspectorUI/UserInterface/Models/ReplaySession.js:83
> +

Nit: unnecessary blank line

> Source/WebInspectorUI/UserInterface/Models/ReplaySessionSegment.js:68
> +

Nit: Unnecessary blank line.

> LayoutTests/inspector/replay/replay-test.js:37
> +    .then(function() {
> +        InspectorTest.log("Capturing has started.");
> +        return new Promise(function waitToCaptureInitialNavigation(resolve, reject) {
> +            InspectorTest.log("Waiting to capture initial navigation...");
> +            InspectorTest.eventDispatcher.addEventListener(InspectorTest.EventDispatcher.Event.TestPageDidLoad, resolve);
> +        });
> +    })
> +    .then(function() {
> +        InspectorTest.log("Initial navigation captured. Dumping state....");
> +        return RuntimeAgent.evaluate.promise("dumpNondeterministicState()");
> +    })
> +    .then(function(payload) {

I hate to admit it, but this test is awesome. Very slick.
Comment 6 BJ Burg 2014-03-20 21:45:40 PDT
Committed r166040: <http://trac.webkit.org/changeset/166040>
Comment 7 Timothy Hatcher 2014-03-21 05:33:54 PDT
Comment on attachment 227234 [details]
the patch

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

> Source/WebInspectorUI/UserInterface/Base/Test.js:193
> -    PageAgent.reload.invoke({ignoreCache: !!shouldIgnoreCache});
> +    return PageAgent.reload.promise(!!shouldIgnoreCache)

You could make invoke still work with promises. See my comment in InspectorBackend.js.

>> Source/WebInspectorUI/UserInterface/Base/Test.js:199
>> +    });
> 
> Style: I like the style you used later with promises. Indent the .then(function() { ... }).

I would put ".then(function() {" on previous line. I think the extra indentation of the body is too much in the later examples.

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

I'd like to see these on the same line.

Analogous to } else if(...) { being one line in our style guide.

> Source/WebInspectorUI/UserInterface/Models/ReplaySession.js:73
> +        var session = this;

Why not bind?

> Source/WebInspectorUI/UserInterface/Models/ReplaySession.js:74
> +        Promise.all(pendingSegments).then(

Same line here looks good to me.

> Source/WebInspectorUI/UserInterface/Models/ReplaySessionSegment.js:57
> +    // XXX: make objects for the queues and inputs?

FIXME?

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:79
>          agent[domainAndMethod[1]] = this._sendMessageToBackend.bind(this, method, signature);
>          agent[domainAndMethod[1]]["invoke"] = this._invoke.bind(this, method, signature);
> +        agent[domainAndMethod[1]]["promise"] = this._promise.bind(this, method, signature);

I am not sold on "promise" as the API for this (maybe another name?). But I think we should fold promises deeper and make the normal method and invoke return a promise if the callback argument is null/undefined. This way you can use invoke and still use a promise result. (In the rare case where we call a backend method with no callback on purpose, the caller would just ignore the Promise result. I assume the Promise will still fulfill even if then() isn't called? Nut I guess that is as simple as adding .then() in those spots — or maybe an alias we add to Promise.prototype like .fulfill()?)

>> LayoutTests/inspector/replay/replay-test.js:37
>> +    .then(function(payload) {
> 
> I hate to admit it, but this test is awesome. Very slick.

Very!