Bug 26083 - How to access formatted Java script Console message ?
Summary: How to access formatted Java script Console message ?
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 31860 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-05-29 11:33 PDT by Gopal Raghavan
Modified: 2014-12-11 16:38 PST (History)
13 users (show)

See Also:


Attachments
Fix for formatting javascript console message and handling firebug api (11.28 KB, patch)
2009-09-17 10:38 PDT, Gopal Raghavan
no flags Details | Formatted Diff | Diff
Fix for formatted js console log message. Added some info to ChangeLog as suggested by Eric.S (12.14 KB, patch)
2009-09-24 09:20 PDT, Gopal Raghavan
abarth: review-
Details | Formatted Diff | Diff
Fix for formatted js console log message. Fixed some style issues as suggested by Adam Barth. (12.17 KB, patch)
2009-10-18 22:38 PDT, Gopal Raghavan
laszlo.gombos: review-
abarth: commit-queue-
Details | Formatted Diff | Diff
Patch against r71350, with fixes to issues raised by Lazlo. Layout tests also added. (19.29 KB, patch)
2010-11-04 12:35 PDT, Gopal Raghavan
kenneth: review-
kenneth: commit-queue-
Details | Formatted Diff | Diff
Taken care of Kenneth's comments and qt build issue (18.97 KB, patch)
2010-11-04 15:19 PDT, Gopal Raghavan
no flags Details | Formatted Diff | Diff
Fixed build issues and took care of review comments (16.07 KB, patch)
2010-11-17 09:52 PST, Gopal Raghavan
laszlo.gombos: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gopal Raghavan 2009-05-29 11:33:12 PDT
On Thu, May 14, 2009 at 11:48 AM, Gopal Raghavan <gop.rag@gmail.com> 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 <darin@apple.com> 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 <gop.rag@gmail.com> 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 <darin@apple.com> 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
Comment 1 Gopal Raghavan 2009-09-17 10:38:07 PDT
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
Comment 2 Eric Seidel (no email) 2009-09-23 17:09:33 PDT
Tab in ChangeLog will cause this to fail to land.

You've failed to explain in your ChangeLog why we want this change.
Comment 3 Gopal Raghavan 2009-09-24 09:20:32 PDT
Created attachment 40070 [details]
Fix for formatted js console log message. Added some info to ChangeLog as suggested by Eric.S
Comment 4 Eric Seidel (no email) 2009-09-28 17:29:47 PDT
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.
Comment 5 Adam Barth 2009-10-13 15:08:12 PDT
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?
Comment 6 Gopal Raghavan 2009-10-18 22:38:17 PDT
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
Comment 7 Adam Barth 2009-11-27 14:50:03 PST
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).
Comment 8 Laszlo Gombos 2010-01-27 15:38:18 PST
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.
Comment 9 Gopal Raghavan 2010-11-04 12:35:29 PDT
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
Comment 10 Kenneth Rohde Christiansen 2010-11-04 13:00:27 PDT
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
Comment 11 Early Warning System Bot 2010-11-04 13:08:14 PDT
Attachment 72972 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5198021
Comment 12 Gopal Raghavan 2010-11-04 15:19:46 PDT
Created attachment 72989 [details]
Taken care of Kenneth's comments and qt build issue

Thanks Kenneth, for your quick feedback.

BR,
--
Gopal
Comment 13 Gopal Raghavan 2010-11-04 15:32:35 PDT
This will also solve https://bugs.webkit.org/show_bug.cgi?id=31860
Comment 14 Alexey Proskuryakov 2010-11-04 23:31:59 PDT
+    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.
Comment 15 WebKit Review Bot 2010-11-05 04:15:06 PDT
Attachment 72989 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5324012
Comment 16 Gopal Raghavan 2010-11-17 09:52:10 PST
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
Comment 17 WebKit Review Bot 2010-11-17 13:21:06 PST
Attachment 74128 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6113029
Comment 18 Laszlo Gombos 2010-11-17 23:01:03 PST
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.
Comment 19 Yury Semikhatsky 2010-11-18 09:18:06 PST
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.
Comment 20 Pavel Feldman 2010-11-18 09:23:22 PST
We should also make sure we don't harm performance here.
Comment 21 Simon Hausmann 2011-12-04 21:55:27 PST
*** Bug 31860 has been marked as a duplicate of this bug. ***
Comment 22 Brian Burg 2014-12-11 16:38:53 PST
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++.