[webkit-reviews] review granted: [Bug 129190] Web Inspector: model tests should use a special Test.html inspector page : [Attachment 225391] patch, now with even less fail

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 129190: Web Inspector: model tests should use a special Test.html inspector
page
https://bugs.webkit.org/show_bug.cgi?id=129190

Attachment 225391: patch, now with even less fail
https://bugs.webkit.org/attachment.cgi?id=225391&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225391&action=review


> Source/WebInspectorUI/UserInterface/Base/Test.js:28
> +WebInspector.loaded = function()
> +{
> +    // Register observers for events from the InspectorBackend.

A comment about matching Main.js order?

> Source/WebInspectorUI/UserInterface/Protocol/InspectorObserver.js:36
> +    // FIXME: remove plumbing for callIdentifier parameter from backend, as
it's not used.

Nit: remove should be Remove

> Source/WebInspectorUI/UserInterface/Test.html:33
> +    <!--
> +    Each subsection is alphabetical, with the exception that base classes
must
> +    be loaded before any classes that inherit from the base class are
loaded.
> +    -->

This is correct. But just say 'match Main.html order/grouping'? Note: I left a
similar comment out of Main.html to prevent it from shipping with a comment.

> Source/WebInspectorUI/UserInterface/Test.html:109
> +<div id="docked-resizer"></div>
> +<div id="toolbar"></div>
> +<div id="main">
> +    <div id="navigation-sidebar"></div>
> +    <div id="content">
> +	   <div id="content-browser"></div>
> +	   <div id="split-content-browser" class="hidden"></div>
> +	   <div id="quick-console"></div>
> +    </div>
> +    <div id="details-sidebar"></div>
> +</div>

None of this should be needed. Right?

> LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js:161
> +// FIXME: move model tests off of the stub inspector page, and delete this
function

Nit: move should be Move