Bug 14953 - Implement window.console in WebCore
Summary: Implement window.console in WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 14263 (view as bug list)
Depends on:
Blocks: 14354
  Show dependency treegraph
 
Reported: 2007-08-12 16:28 PDT by David Kilzer (:ddkilzer)
Modified: 2007-12-05 14:12 PST (History)
3 users (show)

See Also:


Attachments
Patch v1 (20.97 KB, patch)
2007-08-12 16:42 PDT, David Kilzer (:ddkilzer)
ddkilzer: review-
Details | Formatted Diff | Diff
Updated patch [WIP] (22.32 KB, patch)
2007-08-12 17:58 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Even more updated [Still WIP] (23.79 KB, patch)
2007-08-12 18:56 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
patch (30.37 KB, patch)
2007-10-27 19:05 PDT, Sam Weinig
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2007-08-12 16:28:07 PDT
The first step to supporting the Firebug Console API (see Bug 14354) is to implement window.console in WebCore.  (Currently, both Safari for Windows and Safari for Mac OS X implement it within the browser.)

Will attach a patch shortly.
Comment 1 David Kilzer (:ddkilzer) 2007-08-12 16:42:40 PDT
Created attachment 15943 [details]
Patch v1

Initial implementation of window.console in WebCore.
Comment 2 David Kilzer (:ddkilzer) 2007-08-12 17:08:10 PDT
Comment on attachment 15943 [details]
Patch v1

Maciej says:

We've used shadow/shadowing to refer specifically to overriding a built-in Window property with a var declaration, I think a better name for this concept would be "Replaceable".
Comment 3 David Kilzer (:ddkilzer) 2007-08-12 17:31:29 PDT
Comment on attachment 15943 [details]
Patch v1

Wrong license applied to source files as well.  Should be BSD-style, not LGPL.
Comment 4 Sam Weinig 2007-08-12 17:58:37 PDT
Created attachment 15944 [details]
Updated patch [WIP]

Adding an updated patch that is not quite ready for review.  This moves Console.{h,cpp,idl} to WebCore/page/ with the rest of the window objects, fixes the licenses, fixes some style issues and changes the extended attribute to Replaceable.
Comment 5 David Kilzer (:ddkilzer) 2007-08-12 18:43:45 PDT
(In reply to comment #4)
> Created an attachment (id=15944) [edit]
> Updated patch [WIP]

WebCore.pro and WebCore.vcproj changes were lost from previous patch.

Comment 6 Sam Weinig 2007-08-12 18:56:36 PDT
Created attachment 15946 [details]
Even more updated [Still WIP]

Adds WebCore.pro and WebCore.vcproj back
Comment 7 David Kilzer (:ddkilzer) 2007-10-13 02:50:50 PDT
<rdar://problem/5539122>
Comment 8 Timothy Hatcher 2007-10-27 12:39:50 PDT
+    // FIXME: this should use LogMessageLevel, but must use ErrorMessageLevel for now to forward message to the console.

etc.

We should pass the right message level to Crome and let Crome's addMessageToConsole do the right thing when calling the client. Passing the wrong message level is gimping the new Web Inspector console.
Comment 9 Timothy Hatcher 2007-10-27 12:53:19 PDT
If you make this change:

Index: page/Chrome.cpp
===================================================================
--- page/Chrome.cpp	(revision 27149)
+++ page/Chrome.cpp	(working copy)
@@ -197,7 +197,7 @@
 
 void Chrome::addMessageToConsole(MessageSource source, MessageLevel level, const String& message, unsigned lineNumber, const String& sourceID)
 {
-    if (source == JSMessageSource && level == ErrorMessageLevel)
+    if (source == JSMessageSource)
         m_client->addMessageToConsole(message, lineNumber, sourceID);
 
     if (InspectorController* inspector = m_page->inspectorController())

Then you can pass all the correct message levels.
Comment 10 Adam Roben (:aroben) 2007-10-27 16:00:29 PDT
(In reply to comment #8)
> +    // FIXME: this should use LogMessageLevel, but must use ErrorMessageLevel
> for now to forward message to the console.
> 
> etc.
> 
> We should pass the right message level to Crome and let Crome's
> addMessageToConsole do the right thing when calling the client. Passing the
> wrong message level is gimping the new Web Inspector console.

When I added the message level/source stuff I was being pretty paranoid about not changing the set of messages passed up to the ChromeClient. I think we can be less paranoid now and do as you suggest.
Comment 11 Sam Weinig 2007-10-27 19:05:04 PDT
Created attachment 16904 [details]
patch
Comment 12 Adam Roben (:aroben) 2007-10-27 19:19:54 PDT
Comment on attachment 16904 [details]
patch

It might be nice to add a test that actually called methods on the Console object.

r=me
Comment 13 Sam Weinig 2007-10-27 19:25:36 PDT
Landed in r27161
Comment 14 Timothy Hatcher 2007-12-05 14:12:47 PST
*** Bug 14263 has been marked as a duplicate of this bug. ***