Bug 26083 – How to access formatted Java script Console message ?

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[Gopal Raghavan]

On Thu, May 14, 2009 at 11:48 AM, Gopal Raghavan <[e-mail]> wrote:

    WebKit/WebCore/page/Cosole.cpp provides Console::log(), debug(), error() etc., to support Firebug console API.
    There are two overloaded addMessage(). Console::log messages are processed through Console::addMessage(MessageLevel level, ScriptCallStack* callStack, bool acceptNoArguments)


    I am trying to access this message through the chromeClient.

    Any idea why only the first argument is passed to  client ?


    // in Console::addMessage
        if (getFirstArgumentAsString(callStack->state(), lastCaller, message))
            page->chrome()->client()->addMessageToConsole(message, lastCaller.lineNumber(), lastCaller.sourceURL().prettyURL());


    Immediately, as next step the entire message is provided to inspector.

    When inspector processes the ConsoleMessage, it uses the formatter in front-end/console.js to display it on Inspector console.

    Is there a way to access this formatted message ?

    Thanks,
    --
    Gopal


On Thu, May 14, 2009 at 12:55 PM, Darin Adler <[e-mail]> wrote:

    On May 14, 2009, at 8:48 AM, Gopal Raghavan wrote:

        Any idea why only the first argument is passed to  client ?


    It’s a bug. There’s an internal Apple report of this in the Mac OS X public API that ChromeClient is used for:

       <rdar://problem/6453834> -webView:addMessageToConsole: passes console.log's first argument rather than the composed string

    but I don’t know of a report of it in bugs.webkit.org. Feel free to file a bug report, or to try your hand at fixing it. It could be challenging to fix.

       -- Darin


On Fri, May 29, 2009 at 1:02 PM, Gopal Raghavan <[e-mail]> wrote:

    Darin,
    Under which component should I file this bug?
    WebKit Misc
    WebCore JavaScript
    WebCore Misc
    ...

    I have also coded a patch for this.
    Thanks,
    --
    Gopal



On Fri, May 29, 2009 at 1:07 PM, Darin Adler <[e-mail]> wrote:

    On May 29, 2009, at 10:02 AM, Gopal Raghavan wrote:

        Under which component should I file this bug?
        WebKit Misc
        WebCore JavaScript
        WebCore Misc


    I think WebCore Misc would be fine. See related https://bugs.webkit.org/show_bug.cgi?id=24496

       -- Darin

[Gopal Raghavan]

Created attachment 39707 [details]
Fix for formatting javascript console message and handling firebug api

Current implementation of firebug api support for console.log is not complete.
Only the first argument is returned.
So if we have console.log("This is person info: %o",person) and pass person object, we do not get the complete formatted output.

This fix formats the string appropriately and passes the proper string to chrome client through addMessageToConsole
Supports firebug api outlined in http://getfirebug.com/console.html

[Eric Seidel (no email)]

Tab in ChangeLog will cause this to fail to land.

You've failed to explain in your ChangeLog why we want this change.

[Gopal Raghavan]

Created attachment 40070 [details]
Fix for formatted js console log message. Added some info to ChangeLog as suggested by Eric.S

[Eric Seidel (no email)]

Comment on attachment 39707 [details]
Fix for formatting javascript console message and handling firebug api

I think he meant to obsolete this patch.  "bugzilla-tool post-diff 26083" will take care of automatically uplaoding patches and obsoleting old ones for you btw.

[Adam Barth]

Comment on attachment 40070 [details]
Fix for formatted js console log message. Added some info to ChangeLog as suggested by Eric.S

+ getMessageSourceAndLevelPrefix(source,level,sourceString,levelString);

We need spaces after the commas here.

+ getAllArgumentsAsString(callStack,message);

Here too.

+ void Console::getAllArgumentsAsString(ScriptCallStack* callStack, String& message)

Please remove leading blank line from this function.

+ if (i==0 && lastCaller.argumentAt(i).jsValue().isString()) {

We need spaces around the ==

There are too many style violations for me to review them all, much less understand the content of the patch.  Have you run check-webkit-style on your patch?

[Gopal Raghavan]

Created attachment 41397 [details]
Fix for formatted js console log message. Fixed some style issues as suggested by Adam Barth.

Fixed style issues pointed by Adam.
Ran check-webkit-style. Total errors found:0
Re-submitting patch for review.
Thanks,
--
Gopal

[Adam Barth]

Comment on attachment 41397 [details]
Fix for formatted js console log message. Fixed some style issues as suggested by Adam Barth.

Rejecting patch 41397 from commit-queue.

Failed to run "['/Users/abarth/git/webkit-kr/WebKitTools/Scripts/svn-apply', '--force']" exit_code: 1
Last 500 characters of output:
to file WebCore/page/Console.cpp.rej
patching file WebCore/page/Console.h
Hunk #1 succeeded at 118 (offset 2 lines).
Hunk #2 succeeded at 128 (offset 2 lines).
patching file WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKit/qt/Api/qwebpage.cpp
Hunk #1 succeeded at 1790 (offset 215 lines).
patching file WebKit/qt/Api/qwebpage.h
Hunk #1 succeeded at 341 (offset 3 lines).
patching file WebKit/qt/WebCoreSupport/ChromeClientQt.cpp
Hunk #1 succeeded at 235 (offset 6 lines).

[Laszlo Gombos]

Comment on attachment 41397 [details]
Fix for formatted js console log message. Fixed some style issues as suggested by Adam Barth.

I think this patch should have LayoutTests (fast/dom/Window/console*); this would also help to communicate the motivation behind the patch. 

> Index: WebKit/qt/Api/qwebpage.h
> ===================================================================
> --- WebKit/qt/Api/qwebpage.h	(revision 48300)
> +++ WebKit/qt/Api/qwebpage.h	(working copy)
> @@ -338,7 +338,7 @@ protected:
>      virtual void javaScriptAlert(QWebFrame *originatingFrame, const QString& msg);
>      virtual bool javaScriptConfirm(QWebFrame *originatingFrame, const QString& msg);
>      virtual bool javaScriptPrompt(QWebFrame *originatingFrame, const QString& msg, const QString& defaultValue, QString* result);
> -    virtual void javaScriptConsoleMessage(const QString& message, int lineNumber, const QString& sourceID);
> +    virtual void javaScriptConsoleMessage(const QString& messageSource, const QString& messageLevel, const QString& message, int lineNumber, const QString& sourceID);
>  
>      virtual QString userAgentForUrl(const QUrl& url) const;
>  

This change breaks backward compatibility for the QtWebKit API as it changes the type of the second argument. 

r- for lack of tests and BC break.

[Gopal Raghavan]

Created attachment 72972 [details]
Patch against r71350, with fixes to issues raised by Lazlo. Layout tests also added.

Patch based on r71350.
included console-api.html layout test.

Style check: PASS, LayoutTest:PASS

gopal@fire:~/WebKit1$ WebKitTools/Scripts/check-webkit-style 
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 0 in 11 files

gopal@fire:~/WebKit1$ WebKitTools/Scripts/run-webkit-tests --qt --debug -o /tmp/layout/ LayoutTests/fast/dom/Window/console-functions.html 
Running build-dumprendertree
Running tests from /home/gopal/WebKit1/LayoutTests
Testing 1 test cases.
fast/dom/Window .
8.36s total testing time

all 1 test cases succeeded
gopal@fire:~/WebKit1$ WebKitTools/Scripts/run-webkit-tests --qt --debug -o /tmp/layout/ LayoutTests/fast/dom/Window/console-api.html 
Running build-dumprendertree
Running tests from /home/gopal/WebKit1/LayoutTests
Testing 1 test cases.
fast/dom/Window .
11.32s total testing time

all 1 test cases succeeded

[Kenneth Rohde Christiansen]

Comment on attachment 72972 [details]
Patch against r71350, with fixes to issues raised by Lazlo. Layout tests also added.

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

[Early Warning System Bot]

Attachment 72972 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5198021

[Gopal Raghavan]

Created attachment 72989 [details]
Taken care of Kenneth's comments and qt build issue

Thanks Kenneth, for your quick feedback.

BR,
--
Gopal

[Gopal Raghavan]

This will also solve https://bugs.webkit.org/show_bug.cgi?id=31860

[Alexey Proskuryakov]

+    String formatPattern ="%s%d%i%f%o";
...
+                String pattern = formatPattern.substring(j, formatPatternLen);

What's the benefit of having a String with modifiers, given that new strings need to be created anyway?

+                if (formattedString.contains(pattern, false)) {

This will mishandle e.g. %%d, right?

I'd be extremely wary of custom formatted output implementations, these can be a big source of bugs, maybe even security ones.

[WebKit Review Bot]

Attachment 72989 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5324012

[Gopal Raghavan]

Created attachment 74128 [details]
Fixed build issues and took care of review comments

Thanks for review.
Had to redo patch after Yury's 71966 changeset.
I have now tested all positive and negative cases.
Included layout test.
Br,
--
Gopal

[WebKit Review Bot]

Attachment 74128 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6113029

[Laszlo Gombos]

Comment on attachment 74128 [details]
Fixed build issues and took care of review comments

The patch adds direct dependencies to JavaScriptCore (e.g. PropertyNameArray.h) from within port-independent WebCore without any guards. This makes the Chromium bot fail as Chromium is using V8.

There are classes available that abstract V8 and JavaScriptCore for WebCore and are available to stringify JavaScript values. Check if the ScriptValue interface would work here - you might find toString() and toInspectorValue() useful.

CCing Yury and Pavel for their help/input.

r- as I think the functionality of the makeStringFromObj() function should be implemented for V8 as well.

[Yury Semikhatsky]

As Laszlo said you shouldn't add dependencies on VM specific code to WebCore. Instead you may use things like ScriptState and ScriptValue which are essentially wrappers around JS VM specific objects providing a VM-agnostic interface to such objects. For more details take a look at the implementation of ScriptArguments::getFirstArgumentAsString. It should be quite easy to add something like 

 String ScriptArguments::allArgumentsAsString

which would serialize the arguments into a string in a JS VM independent manner.

[Pavel Feldman]

We should also make sure we don't harm performance here.

[Simon Hausmann]

*** Bug 31860 has been marked as a duplicate of this bug. ***

[Brian Burg]

Inspector supports format strings in the frontend. I don't understand what the use case is for having these on the backend. If it's for logging stderr or something, that should be bouncing through InspectorFrontendHost anyway and doesn't need to be implemented in C++.