Bug 24623 - Refactor ConsoleMessage to use ScriptFunctionCall
Summary: Refactor ConsoleMessage to use ScriptFunctionCall
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on: 24524
Blocks: 25063
  Show dependency treegraph
 
Reported: 2009-03-16 09:51 PDT by Dimitri Glazkov (Google)
Modified: 2009-03-17 09:18 PDT (History)
1 user (show)

See Also:


Attachments
Use ScriptFuncitonCall with ConsoleMessage, v1 (18.47 KB, patch)
2009-03-16 16:09 PDT, Dimitri Glazkov (Google)
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2009-03-16 09:51:22 PDT
Following up on bug 24524, do the same for ConsoleMessage.
Comment 1 Dimitri Glazkov (Google) 2009-03-16 16:09:01 PDT
Created attachment 28662 [details]
Use ScriptFuncitonCall with ConsoleMessage, v1

 WebCore/ChangeLog                              |   28 ++++++
 WebCore/bindings/js/ScriptFunctionCall.cpp     |   12 +++
 WebCore/bindings/js/ScriptFunctionCall.h       |    3 +
 WebCore/bindings/js/ScriptObjectQuarantine.cpp |    7 ++
 WebCore/bindings/js/ScriptObjectQuarantine.h   |    5 +
 WebCore/bindings/js/ScriptValue.cpp            |   11 ++
 WebCore/bindings/js/ScriptValue.h              |    1 +
 WebCore/inspector/ConsoleMessage.cpp           |  119 ++++++++++++++----------
 WebCore/inspector/ConsoleMessage.h             |   29 +++---
 WebCore/inspector/InspectorController.cpp      |   59 +-----------
 WebCore/inspector/InspectorController.h        |    1 -
 11 files changed, 154 insertions(+), 121 deletions(-)
Comment 2 Timothy Hatcher 2009-03-16 20:32:47 PDT
Comment on attachment 28662 [details]
Use ScriptFuncitonCall with ConsoleMessage, v1

Why did you pick "quarantineValue" when you picked "getQuarantinedScriptObject" for the ScriptObject version? I prefer the "quarantineValue", maybe rename "getQuarantinedScriptObject" to "quarantineObject". Or maybe "quarantine" is a better name for all of these functions, and let C++ do it's magic?

Why does "getQuarantinedScriptObject" have a bool return type and "quarantineValue" not?

The ConsoleMessage::addToScript name bother(ed|es) me, maybe addToConsole is better? What do you think?

 87     if (!m_frames.isEmpty())
 88         for (unsigned i = 0; i < m_frames.size(); ++i)
 89             messageConstructor.appendArgument(m_frames[i]);
 90     else if (!m_wrappedArguments.isEmpty())
 91         for (unsigned i = 0; i < m_wrappedArguments.size(); ++i)
 92             messageConstructor.appendArgument(m_wrappedArguments[i]);
 93     else
 94         messageConstructor.appendArgument(m_message);

The first two if statements bodies need braces since they are multi-line.
Comment 3 Dimitri Glazkov (Google) 2009-03-16 20:55:43 PDT
(In reply to comment #2)

Thank you for reviewing so promptly! :)

> (From update of attachment 28662 [details] [review])
> Why did you pick "quarantineValue" when you picked "getQuarantinedScriptObject"
> for the ScriptObject version? I prefer the "quarantineValue", maybe rename
> "getQuarantinedScriptObject" to "quarantineObject". Or maybe "quarantine" is a
> better name for all of these functions, and let C++ do it's magic?

The getXXX methods take a WebCore instance and turn it into a ScriptObject, as opposed to this simple helper, which takes a ScriptValue and returns a ScriptValue. So, I though the difference justified a name change. Also, I can't use the quarantine name for all of them, because the return types differ (ScriptObject/ScriptValue).

> Why does "getQuarantinedScriptObject" have a bool return type and
> "quarantineValue" not?

Since at least one of the getQuarantinedScriptObject methods can fail, I did the bool getName(Type& ref) convention. The quarantineValue doesn't fail, so it doesn't need to be in this convention.

> The ConsoleMessage::addToScript name bother(ed|es) me, maybe addToConsole is
> better? What do you think?

I ain't too hot about addToScript myself, and wrecked my brain trying to find a better name. For now, I'll go with addToConsole.

Ideally we need a name that says "create a mirror object on JS side of the Inspector", but in one word :)

>  87     if (!m_frames.isEmpty())
>  88         for (unsigned i = 0; i < m_frames.size(); ++i)
>  89             messageConstructor.appendArgument(m_frames[i]);
>  90     else if (!m_wrappedArguments.isEmpty())
>  91         for (unsigned i = 0; i < m_wrappedArguments.size(); ++i)
>  92             messageConstructor.appendArgument(m_wrappedArguments[i]);
>  93     else
>  94         messageConstructor.appendArgument(m_message);
> 
> The first two if statements bodies need braces since they are multi-line.

Ooh, I didn't know that. Will fix.
Comment 4 Timothy Hatcher 2009-03-16 21:02:34 PDT
Comment on attachment 28662 [details]
Use ScriptFuncitonCall with ConsoleMessage, v1

r+ with the changes we talked about.
Comment 5 Dimitri Glazkov (Google) 2009-03-17 09:18:13 PDT
Landed as http://trac.webkit.org/changeset/41765.