Bug 17228

Summary: console.{log,warn,info,error} should support format strings, variable arguments
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: Web Inspector (Deprecated)Assignee: Adam Roben (:aroben) <aroben>
Status: RESOLVED FIXED    
Severity: Enhancement CC: sam, timothy
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 16837    
Bug Blocks: 14354    
Attachments:
Description Flags
Step 1: Pass JSConsole's arguments to Console unmodified
none
Step 2: Route all Console-related activities through Console
none
Step 2: Route all Console-related activities through Console
none
Step 2: Route all Console-related activities through Console
none
Step 3: Pass all arguments to console.{log,warn,info,error} down to the Inspector's JS
none
Step 3: Pass all arguments to console.{log,warn,info,error} down to the Inspector's JS
darin: review-
Step 3 (v2): Pass all arguments to console.{log,warn,info,error} down to the Inspector's JS
none
Step 4: Separate out tokenizing of format strings
none
Step 5: Make the formatting functions used for each format specifier configurable
none
Step 6: Make String.format able to produce objects other than strings
none
Step 7: Return the unused substitutions from String.format
none
testcase
none
Final step: Treat the first argument as a format string, concatenate additional arguments
none
Fix a bug in the testcase none

Description Adam Roben (:aroben) 2008-02-08 13:50:51 PST
Our implementation of console.{log,warn,info,error} should support format strings, like Firebug does.
Comment 1 Adam Roben (:aroben) 2008-02-08 14:17:23 PST
<rdar://problem/5732828>
Comment 2 Adam Roben (:aroben) 2008-03-21 11:23:43 PDT
Firebug also lets you list more variables than you have format specifiers, and the remaining variables are appended to the string separated by spaces.
Comment 3 Adam Roben (:aroben) 2008-03-21 11:26:09 PDT
Created attachment 19931 [details]
Step 1: Pass JSConsole's arguments to Console unmodified
Comment 4 Adam Roben (:aroben) 2008-03-21 12:18:27 PDT
Created attachment 19933 [details]
Step 2: Route all Console-related activities through Console
Comment 5 Adam Roben (:aroben) 2008-03-21 13:39:47 PDT
Created attachment 19934 [details]
Step 2: Route all Console-related activities through Console

Corrected a typo
Comment 6 Adam Roben (:aroben) 2008-03-21 13:40:46 PDT
Created attachment 19935 [details]
Step 2: Route all Console-related activities through Console

Uploaded the right patch this time
Comment 7 Adam Roben (:aroben) 2008-03-21 14:06:40 PDT
Created attachment 19937 [details]
Step 3: Pass all arguments to console.{log,warn,info,error} down to the Inspector's JS
Comment 8 Adam Roben (:aroben) 2008-03-21 15:47:46 PDT
Created attachment 19946 [details]
Step 3: Pass all arguments to console.{log,warn,info,error} down to the Inspector's JS

We're now more careful in how we handle the arguments passed in from the inspected page.
Comment 9 Darin Adler 2008-04-02 02:37:02 PDT
Comment on attachment 19931 [details]
Step 1: Pass JSConsole's arguments to Console unmodified

This is unconventional structure. It's not really a "binding" if we're passing the JavaScript arguments directly to the underlying DOM object.

I'm going to say r=me, but if console is really going to be a pure JavaScript object, then maybe we shouldn't use .idl to create the object.
Comment 10 Darin Adler 2008-04-02 02:41:58 PDT
Comment on attachment 19935 [details]
Step 2: Route all Console-related activities through Console

Why does the console() hang off domWindow()? Why not directly off Frame?

I think we should move the "shouldPrintExceptions" and printf stuff into the console object too.

+void FrameLoader::reportLocalLoadFailed(const Frame* frame, const String& url)

The use of const Frame* here is strange. Just Frame* will do.

-        void addMessageToConsole(MessageSource, MessageLevel, const String& message, unsigned lineNumber, const String& sourceID);
+        // This method should only be called by Console
+        void addMessageToConsole(const String& message, unsigned lineNumber, const String& sourceID);

I think Console should talk directly to the ChromeClient. I don't think we need a Chrome function in between. Then you wouldn't need that comment. Also, please don't call C++ member functions "methods".

It's extremely unconventional to use the name "state" for an ExecState* parameter. We use "exec" almost everywhere. I think "state" might be better if we did it everywhere, but I think it's not a great idea to start being original about this in the console code.

r=me
Comment 11 Darin Adler 2008-04-02 03:13:32 PDT
Comment on attachment 19946 [details]
Step 3: Pass all arguments to console.{log,warn,info,error} down to the Inspector's JS

I think it could be unsafe to pass the actual JavaScript objects to the inspector. This allows a webpage to get the inspector to execute code on its behalf in the inspector's privileged context, for example if it supplies an object with a custom toString function.

+        JSValueRef messageValue = JSValueMakeString(m_scriptContext, messageString.get());
+        arguments.append(messageValue);

This is a problem. Once the local variable messageValue goes out of scope, the string could be garbage collected. Since you're using a Vector here, the values won't be on the stack and won't be seen by the garbage collector. Depending on how the optimizer optimizes the C++ code, this could be an issue for the other things like sourceValue as well; the compiler is not obligated to keep the value around on the stack or in a register once you've made the last use of it.

Because of this, I think you need either not use a Vector or use something that calls JSValueProtect.

My suggestion is that you choose a fixed maximum number of arguments and use an array on the stack rather than a Vector.

review- because of the GC issue with the arguments Vector.
Comment 12 Adam Roben (:aroben) 2008-04-02 11:17:26 PDT
(In reply to comment #9)
> (From update of attachment 19931 [details] [edit])
> This is unconventional structure. It's not really a "binding" if we're passing
> the JavaScript arguments directly to the underlying DOM object.

I suppose that's true. Sam was saying he thought we should leave the string-only versions of the functions as well so that they we can make them callable from, e.g., Obj-C. I decided not to leave them in this patch because we aren't currently using them, and I didn't want to leave in dead code.

> I'm going to say r=me, but if console is really going to be a pure JavaScript
> object, then maybe we shouldn't use .idl to create the object.

I guess if we do add back the string-only versions of the functions then Console will be more of a "true" binding again.
Comment 13 Adam Roben (:aroben) 2008-04-15 10:51:32 PDT
Created attachment 20561 [details]
Step 3 (v2): Pass all arguments to console.{log,warn,info,error} down to the Inspector's JS

This patch addresses Darin's comments about security and garbage collection issues.
Comment 14 Adam Roben (:aroben) 2008-04-15 16:29:55 PDT
Created attachment 20566 [details]
Step 4: Separate out tokenizing of format strings

This is the first in a series of patches that refactors our string formatting code so that it can be used by the Console.
Comment 15 Adam Roben (:aroben) 2008-04-15 16:57:59 PDT
Created attachment 20569 [details]
Step 5: Make the formatting functions used for each format specifier configurable
Comment 16 Adam Roben (:aroben) 2008-04-15 16:58:25 PDT
Created attachment 20570 [details]
Step 6: Make String.format able to produce objects other than strings
Comment 17 Adam Roben (:aroben) 2008-04-15 16:58:47 PDT
Created attachment 20571 [details]
Step 7: Return the unused substitutions from String.format
Comment 18 Adam Roben (:aroben) 2008-04-15 18:21:33 PDT
Created attachment 20573 [details]
testcase

Here's a test that you can run in Firebug or in the Inspector. It actually has turned up some bizarre behaviors in Firebug that we probably don't want to emulate (e.g., trying to format an object as an integer ends up printing "[Object object]"). I'll probably end up committing some variation of this in WebCore/manual-tests.
Comment 19 Timothy Hatcher 2008-04-16 08:33:44 PDT
Comment on attachment 20571 [details]
Step 7: Return the unused substitutions from String.format

As talked about on IRC, returning a anonymous object with two named properties would be better.
Comment 20 Adam Roben (:aroben) 2008-04-16 11:17:20 PDT
Created attachment 20592 [details]
Final step: Treat the first argument as a format string, concatenate additional arguments
Comment 21 Adam Roben (:aroben) 2008-04-16 12:20:28 PDT
Comment on attachment 19931 [details]
Step 1: Pass JSConsole's arguments to Console unmodified

Committed in r31950
Comment 22 Adam Roben (:aroben) 2008-04-16 12:21:19 PDT
Comment on attachment 19935 [details]
Step 2: Route all Console-related activities through Console

Committed in r31951
Comment 23 Adam Roben (:aroben) 2008-04-16 12:21:43 PDT
Comment on attachment 20561 [details]
Step 3 (v2): Pass all arguments to console.{log,warn,info,error} down to the Inspector's JS

Committed in r31952
Comment 24 Adam Roben (:aroben) 2008-04-16 12:22:16 PDT
Comment on attachment 20566 [details]
Step 4: Separate out tokenizing of format strings

Committed in r31953
Comment 25 Adam Roben (:aroben) 2008-04-16 12:22:44 PDT
Comment on attachment 20569 [details]
Step 5: Make the formatting functions used for each format specifier configurable

Committed in r31954
Comment 26 Adam Roben (:aroben) 2008-04-16 12:36:22 PDT
Comment on attachment 20570 [details]
Step 6: Make String.format able to produce objects other than strings

Committed in r31955
Comment 27 Adam Roben (:aroben) 2008-04-16 12:36:45 PDT
Comment on attachment 20571 [details]
Step 7: Return the unused substitutions from String.format

Committed in r31956
Comment 28 Adam Roben (:aroben) 2008-04-16 12:37:07 PDT
Comment on attachment 20592 [details]
Final step: Treat the first argument as a format string, concatenate additional arguments

Committed in r31957
Comment 29 Adam Roben (:aroben) 2008-04-16 12:45:19 PDT
Created attachment 20600 [details]
Fix a bug in the testcase
Comment 30 Adam Roben (:aroben) 2008-04-16 13:25:09 PDT
Comment on attachment 20600 [details]
Fix a bug in the testcase

Committed in r31959