[webkit-reviews] review denied: [Bug 31538] Web Inspector: Wrong console output for Regexp escape sequence : [Attachment 43826] [PATCH] Desired Behavior

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 31538: Web Inspector: Wrong console output for Regexp escape sequence
https://bugs.webkit.org/show_bug.cgi?id=31538

Attachment 43826: [PATCH] Desired Behavior
https://bugs.webkit.org/attachment.cgi?id=43826&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
> +CONSOLE MESSAGE: line 49: Sat Nov 21 2009 14:25:45 GMT-0500 (EST)
> +CONSOLE MESSAGE: line 50: Sat Nov 21 2009 14:25:45 GMT-0500 (EST)

Not everybody is on EST, bots will probably fail on this one. (r- due to this).


> +console-format.html:49Sat Nov 21 2009 14:25:45 GMT-0500 (EST)
> +console-format.html:50[Sat Nov 21 2009 14:25:45 GMT-0500 (EST)]
> +"Sat Nov 21 2009 14:25:45 GMT-0500 (EST)"

Ditto.

> +var node = null; // NOTE: This is filled in after the page has loaded in
doit();

Why not to push everything in doit, leaving only var globals = [] here?

> +function loopOverGlobals(current)

Ok, so you want to wait for both logs and eval to be formatted before you dump
the next value.

> +function frontend_evaluateStringInConsole()
> +{
> +    var expression = "\"this is a test of a raw string in the console\"";

Not sure why you need to special case this - we already check it for "test".
Should you change "test" to "raw string should be quoted on eval and array, not
in log." or something?

> +function frontend_evalExpression(expression)

Nit: We could refactor ConsoleView.prototype._enterKeyPressed for better
testability (extract what you need into public "evaluateExpressionInConsole" or
something. We already have a bunch of evalIn and doEvalIn though.

> +    this.knownFormatters = {

This does not need to be public.

> +    this.undecoratedTypes = {

Ditto.

> -	   if (isProxy && type !== "object" && type !== "function" && type !==
"array" && type !== "node") {
> +	   if (isProxy && !(type in this.knownFormatters)) {

So the idea here was that everything comes as a proxy. I think !isProxy means
null only (worth checking).
Now for primitive values, we expected description to be good, so we were
unwrapping them and dumping descriptions to console (via marking them as
undecorated).
It is just that we did not delete primitive formatters when migrated to proxies
for primitive values (i.e. formatdate, formatstring are unused on ToT).
I think what you should do is rename your knownFormatters to customFormatters
and leave only object, function, array, node, string and regex there.
Everything else should fall under undecorated types - not even need to check
and create enum for them - just output descriptions there.

>      _formatdate: function(date, elem)

Should not exist.

>      },

>      _formatstring: function(str, elem)
>      {
> -	   elem.appendChild(document.createTextNode("\"" + str + "\""));

>      _formatregexp: function(re, elem)
>      {
> -	   var formatted = String(re.description).replace(/([\\\/])/g,
"\\$1").replace(/\\(\/[gim]*)$/, "$1").substring(1);
> +	   var formatted = String(re.description).replace(/\\(.)/g, "$1");

It is not clear to me why this is needed. Is the one in Object.describe bad?
Should it be fixed instead? That way regex won't need custom formatter.

>	   for (var i = 0; i < parameters.length; ++i) {
> -	       if (Object.proxyType(parameters[i]) === "string")
> -		  
formattedResult.appendChild(WebInspector.linkifyStringAsFragment(parameters[i].
description));
> -	       else
> -		  
formattedResult.appendChild(formatForConsole(parameters[i]));
> +	       var param = parameters[i];
> +	       if (Object.proxyType(param) === "string") {
> +		   if (this.originatingCommand && this.level ===
WebInspector.ConsoleMessage.MessageLevel.Log) {
> +		       var quotedString = "\"" +
param.description.replace(/"/g, "\\\"") + "\"";
> +		      
formattedResult.appendChild(WebInspector.linkifyStringAsFragment(quotedString))
;

I guess you expect this to be evaluation result only. My question is how can
something from console raw eval have parameters?
In other words, should we special case this earlier on entering _format? Does
it make sense?