[webkit-reviews] review requested: [Bug 44875] Web Inspector: it'd be better to introduce inspector API related tests. : [Attachment 65947] [patch] second iteration

[bugzilla-daemon at webkit.org]

Ilya Tikhonovsky <loislo at chromium.org> has asked  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 65947: [patch] second iteration
https://bugs.webkit.org/attachment.cgi?id=65947&action=review

------- Additional Comments from Ilya Tikhonovsky <loislo at chromium.org>
(In reply to comment #2)
> (From update of attachment 65930 [details])
> > 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.

done.

> > 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.

done


> > +	 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.

That will be the target of another test. In this test I'm trying to cover the
functionality of
the internal Inspector JS API implemented in InspectorBackendStub.js

> > 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?

Just realized that I can sniff console.error instead of reportProtocolError.
New step for this use case was added and an error in the code was fixed. :)


> > 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.

This part of patch just removed.