[webkit-reviews] review granted: [Bug 17228] console.{log, warn, info, error} should support format strings, variable arguments : [Attachment 19935] Step 2: Route all Console-related activities through Console

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Adam Roben (aroben)
<aroben at apple.com>'s request for review:
Bug 17228: console.{log,warn,info,error} should support format strings,
variable arguments
http://bugs.webkit.org/show_bug.cgi?id=17228

Attachment 19935: Step 2: Route all Console-related activities through Console
http://bugs.webkit.org/attachment.cgi?id=19935&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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