[webkit-reviews] review denied: [Bug 40980] Web Inspector: null is not properly handled when evaluated in console (V8 only) : [Attachment 59384] patch (more different types of objects added)

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrey Kosyakov
<caseq at chromium.org>'s request for review:
Bug 40980: Web Inspector: null is not properly handled when evaluated in
console (V8 only)
https://bugs.webkit.org/show_bug.cgi?id=40980

Attachment 59384: patch (more different types of objects added)
https://bugs.webkit.org/attachment.cgi?id=59384&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
WebCore/inspector/front-end/InjectedScript.js:604
 +		objectText = type;
So you always return "function" for functions that have poor toString. Old code
was smarter (before it got regressed). It was doing
Object.prototype.toString(obj) on objects like this (which was evolved into
"[object Function]" or similar). Then, className was extracting the "Function"
part. I think we should fix the regression in the InjectedScript._toString
first (see Safari 4 code for that). Note that there were some reservations wrt
Object lifetime that were making Object.prototype.toString call problematic.

Maybe we should simplify the code as you suggested, but I don't think returning
"function" for all objects like this is a good idea.


WebCore/inspector/front-end/InjectedScript.js: 
 +	    if (typeof obj !== "object")
I think this will break chromium interactive tests. Take a look at r58956. I
know that typeof null is in fact "object", so the fix in r58956 itself was
arguable. Probably this needs a more strict check such as  (obj === null ||
typeof obj !== "object")