[Webkit-unassigned] [Bug 38664] Web Inspector: add a "table" method to console, to allow output of tabular data

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 17 11:27:00 PDT 2010


https://bugs.webkit.org/show_bug.cgi?id=38664





--- Comment #51 from Patrick Mueller <pmuellr at yahoo.com>  2010-06-17 11:27:00 PST ---
(In reply to comment #50)
> (From update of attachment 58946 [details])
> 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. 

thx!

Comments below for clarification on some of the points.

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

I took a pass through ALL of the ==/!= checks to turn them into ===/!== where I could.  There are places, and this probably not one of them, where I really need != null or == null - I want to be able to trap null or undefined but NOT 0.  Lucked into finding that one with one of the test cases.  I suppose I should probably comment on WHY the check is made that way, for those special cases.

I suspect there are plenty of cases where I can collapse to just a field check, as you suggest.

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

I first tried to throw a TypeError for these cases, but that didn't work - the exception never made it back over the other side.  Didn't investigate why, I guess no one actually needs to do this.  Generating a console message was the other option, but decided to defer doing it for now.

There are other odd cases, if you run the tests, like tables with a header but no body.  It would probably be useful to generate a message for these also, but wasn't sure.

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

There are no "API" fields in the class, they're all internal, no code outside the class should ever need to touch anything.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list