[webkit-reviews] review denied: [Bug 38664] Web Inspector: add a "table" method to console, to allow output of tabular data : [Attachment 58946] proposed patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Patrick Mueller
<pmuellr at yahoo.com>'s request for review:
Bug 38664: Web Inspector: add a "table" method to console, to allow output of
tabular data
https://bugs.webkit.org/show_bug.cgi?id=38664

Attachment 58946: proposed patch
https://bugs.webkit.org/attachment.cgi?id=58946&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
I just took a brief pass over this. Sorry that I only really
point out style issues. But, since many will need to be fixed
better to catch them early. I have to re-read the comments
and your test case to catch up on how console.table is used
to do a proper review. And since you are waiting for feedback
on webkit-dev, I think this brief review is okay for now =)


> +    if (this.headersProxy != null) {

I see you using this != null idiom quite a bit. So, are you
trying to handle cases where this is either "undefined" or "null"
only, and not 0, "", NaN, false? Typically we would just do
"if (this.headersProxy)", or if we really want null then !==.
Is there a reason this style check might not work here?


> +    // this method kicks off retrieving all the data needed to render the
table

Comments should be sentences. Start with a capital, end with a period.


> +	   if (proxyType === "object") this.headersProxy = null;

Use two lines for these single line ifs. There are a bunch of these.


> +		   return this._errorOccurred("invalid columns parameter");

These error strings would need a WebInspector.UIString(...).


> +    // handle an error message
> +    _errorOccurred: function(message)
> +    {
> +	   // for now, do nothing - don't print anything
> +	   // should we print an error message?
> +    },

For comments line these, its nice to add "FIXME: " at
the start. Even if its just a question for review, it
makes them easier to spot and search for =).

I think a red error message in the console would be
nice. Like a "console.error(message)" red message.


> +//	     dataGrid.autoSizeColumns(5);

Commented out line can be removed. Probably experimenting with a
non 100% width table?


> +	       for (columnName in this.columns)
> +		   if (row[columnName] == null) row[columnName] = "";

This leaks "columnName" out to the global scope. Missing `var`:

  for (var columnName in this.columns)

Also, another "== null" that can be "=== null" in this case.


> +	   }
> +	   else {

Same line.

  } else {


> +	   var callback = function(proxyProperties) {

Typically we use "function callback(...)"


> +	       self.requests--;
> +	   this.requests++;

Private/internal variables like this we normally put an underscore
on. So "this._requests". There are a number of other variables
that might be able to change as well. But its not too important.

> +	   return string.substr(1,string.length-2);

Missing a space after the comma.



> +	       case WebInspector.ConsoleMessage.MessageType.Table:
> +		   typeString = "Table";
> +		   break;
> +		   
>	   }

Remove the blank line. (There are a few of these sprinkled in the patch).