[webkit-reviews] review granted: [Bug 130145] Web Inspector: add frontend controller and models for replay sessions : [Attachment 227234] the patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Mar 20 16:39:02 PDT 2014


Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 130145: Web Inspector: add frontend controller and models for replay
sessions
https://bugs.webkit.org/show_bug.cgi?id=130145

Attachment 227234: the patch
https://bugs.webkit.org/attachment.cgi?id=227234&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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.Ev
ent.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.


More information about the webkit-reviews mailing list