[webkit-reviews] review denied: [Bug 44875] Web Inspector: it'd be better to introduce inspector API related tests. : [Attachment 65930] [patch] initial version.

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 44875: Web Inspector: it'd be better to introduce inspector API related
tests.
https://bugs.webkit.org/show_bug.cgi?id=44875

Attachment 65930: [patch] initial version.
https://bugs.webkit.org/attachment.cgi?id=65930&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
> diff --git LayoutTests/ChangeLog
> +2010-08-30  Ilya Tikhonovsky  <loislo at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   WebInspector: As far as we have some kind of API for Inspector
> +	   it'd be better to have API related tests. This is the test
> +	   for API wrappers. These wrappers are tracking the types of arguments

> +	   of API functions.

Missing  bug title + link. (Maybe this was expected if this was
webkit-patch upload?) Applies to all ChangeLogs in this patch.


> diff --git LayoutTests/inspector/report-API-errors.html
> +var test = function()
> +{
> +    var callback = function(message)
> +    {

This both use the "var x = function()" syntax. I find that unusual.
I think most of the other tests use the "function x()" syntax, and
I think that is easier to read. Tests typically have all kinds of
styles, so this isn't something to worry about, but the uncommon
syntax caught my eye.


> +    InspectorTest._addSniffer(WebInspector, "reportProtocolError", callback,
true);
> +
> +    InspectorBackend.enableResourceTracking(1, callback);
> +    InspectorBackend.enableResourceTracking();
> +    InspectorBackend.enableResourceTracking(true, "not a function");


This covers "nice" messages with unusual arguments. This is very good.
Is there a plan for testing "bad" messages? Such as:

  InspectorFrontendHost.sendMessageToBackend("bad message")

I guess this shouldn't happen in practice, but if this really is
an over the wire protocol than those messages could be modified.
Testing that interface would be awesome too.


> diff --git WebCore/inspector/front-end/inspector.js
> -	       console.error("Attempted to dispatch unimplemented WebInspector
method: %s", messageObject.event);
> +	       this.reportProtocolError("Attempted to dispatch unimplemented
WebInspector method: %s", messageObject.event);

This changed an "sprintf" style console.error to a reportProtocolError function

call which does not respect sprintf style, and thus the 2nd argument is
ignored.
I seem to recall an easy way to convert a function to the sprintf style, just
wrapping and replacing a function. Either way, could testing this error message

be added to your test?


> diff --git WebCore/inspector/front-end/inspector.js
> -WebInspector.reportProtocolError = function(messageObject)
> +WebInspector.reportProtocolError = function(message)
>  {
> -    console.error("Error: InspectorBackend request with seq = " +
messageObject.seq + " failed.");
> +    if (typeof message === "string") {
> +	   console.error("Protocol Error: " + message);
> +	   return;
> +    }
> +
> +    console.error("Protocol Error: InspectorBackend request with seq = " +
messageObject.seq + " failed.");
>      for (var error in messageObject.errors)
>	   console.error("    " + error);
>      WebInspector.removeResponseCallbackEntry(messageObject.seq);

These last few lines are using "messageObject". However, there is no more
any "messageObject" in scope? are they needed any more? r- for this.