[webkit-reviews] review denied: [Bug 26083] How to access formatted Java script Console message ? : [Attachment 72972] Patch against r71350, with fixes to issues raised by Lazlo. Layout tests also added.

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Gopal Raghavan
<gopal.1.raghavan at nokia.com>'s request for review:
Bug 26083: How to access formatted Java script Console message ?
https://bugs.webkit.org/show_bug.cgi?id=26083

Attachment 72972: Patch against r71350, with fixes to issues raised by Lazlo.
Layout tests also added.
https://bugs.webkit.org/attachment.cgi?id=72972&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72972&action=review

Many things to fix... I have up halfway

> WebCore/ChangeLog:5
> +	   Fixes bug: https://bugs.webkit.org/show_bug.cgi?id=26083

Please look at other commits on how to format a ChangeLog

> WebCore/page/Console.cpp:149
> +
> +}

why newline here!

> WebCore/page/Console.cpp:200
> +    if (!message.isEmpty())
> +	 page->chrome()->client()->addMessageToConsole(JSMessageSource, type,
level, message, lastCaller.lineNumber(), lastCaller.sourceURL());

wrong indentation!

> WebCore/page/Console.cpp:505
> +//  get all the arguments in message

We dont do comments like these outside the methods... and comments start with
capital letter and ends with a dot!

> WebCore/page/Console.cpp:506
> +void Console::getAllArgumentsAsString(ScriptCallStack* callStack, String&
message)

IS this string supposed to not be const?

> WebCore/page/Console.cpp:508
> +{
> + 

No newline needed here

> WebCore/page/Console.cpp:511
> +    String fStr; // formatted string

fStr... that naming is against our style... please consult out style guide and
use meaning full variables. Then you wouldn't need useless comments like this
one

> WebCore/page/Console.cpp:524
> +	   } else {
> +	       String argStr;
> +	       int idx = 0;
> +	       bool repl = false; // should you replace format pattern with
value
> +

wow, you misses a } so either your code is wrong or your indentation is very
wrong!

> WebCore/page/Console.cpp:529
> +		    // found it

Useless comment.. please remove these

> WebCore/page/Console.cpp:532
> +		    repl = true;

repl? what does that mean? write out variable names

> WebCore/page/Console.cpp:537
> +	   if (lastCaller.argumentAt(i).jsValue().isNumber()) { // handle
numbers

useless comments... the code pretty much says it

> WebCore/page/Console.cpp:555
> +	   if (repl)
> +		fStr.insert(argStr, idx);
> +	   else 
> +		fStr += argStr;
> +	   }

Look at this code... indendation is wrong or so is your logic! bad, very bad.

> WebCore/page/Console.cpp:567
> +	   JSC::ExecState* exec = callStack->globalState();
> +	   JSC::JSObject *myObj = jsVal.getObject();

coding style violation and inconsistency