[webkit-reviews] review denied: [Bug 88707] Web Inspector: Implement snippets evaluation. : [Attachment 146697] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Vsevolod Vlasov
<vsevik at chromium.org>'s request for review:
Bug 88707: Web Inspector: Implement snippets evaluation.
https://bugs.webkit.org/show_bug.cgi?id=88707

Attachment 146697: Patch
https://bugs.webkit.org/attachment.cgi?id=146697&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=146697&action=review


> Source/WebCore/inspector/front-end/BreakpointManager.js:302
> +	       var oldFakeUILocation = /** @type {WebInspector.UILocation} */
this._uiLocations.get(this._fakeRawLocation);

Isn't this the same as this._primaryUILocation ?

> Source/WebCore/inspector/front-end/BreakpointManager.js:303
> +	       if (oldFakeUILocation) {

Can it be 0?

> Source/WebCore/inspector/front-end/BreakpointManager.js:360
> +	   if (this._enabled &&
this._primaryUILocation.uiLocationToRawLocation()) {

This little twist changes the way breakpoint manager interacts with the
debugger. I would add a comment. And I don't think I like it anyways.

> Source/WebCore/inspector/front-end/DebuggerModel.js:138
> +	   if (rawLocation.scriptId) {

I hate to say it, but I feel like debugging capabilities in the snippet editor
are not worth making the underlying models so much more complex.