Bug 130145 – Web Inspector: add frontend controller and models for replay sessions

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

Prerequisite to further UI work.

[Blaze Burg]

Created attachment 227234 [details]
the patch

[WebKit Commit Bot]

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.

[Radar WebKit Bug Importer]

<rdar://problem/16373707>

[Radar WebKit Bug Importer]

<rdar://problem/16373726>

[Joseph Pecoraro]

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.

[Blaze Burg]

Committed r166040: <http://trac.webkit.org/changeset/166040>

[Timothy Hatcher]

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!