[webkit-reviews] review denied: [Bug 123223] Web Inspector: Add a way to test the Manager and model classes : [Attachment 215002] Patch V1

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Alexandru Chiculita
<achicu at adobe.com>'s request for review:
Bug 123223: Web Inspector: Add a way to test the Manager and model classes
https://bugs.webkit.org/show_bug.cgi?id=123223

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

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=215002&action=review


Clever, I like it. This looks good to me, but this will just need to fix
Main.html missing new includes.

> LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js:110
> +    if (xhr.status !== 0)
> +	   throw new Error("Invalid script URL: " + scriptName);

A status of 200 may need to be allowed if there is a
LayoutTest/http/inspector-protocol test. Though I'm not sure that will ever
happen.

> LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js:122
> +	   "InspectorFrontendAPI",

When this is included it will overwrite the "InspectorFrontendAPI" implemented
by LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js. We
will need to be careful, if a test wants to grab events we'll need to
re-incorporate a way to get back the protocol-test
InspectorTest.eventHandler["Debugger.scriptParsed"] syntax, or something like
it. But if we are indeed just testing the model, then we probably will never
care about this.

> Source/WebInspectorUI/UserInterface/MessageDispatcher.js:1
> +/*

These new files will need to be included by Main.html, in the right order, so
that the frontend itself still works! r- for this.

> Source/WebInspectorUI/UserInterface/URLUtilities.js:1
> +/*

I like that you've put all of this code in one file. I've had that itch for a
while!