Bug 24623 – Refactor ConsoleMessage to use ScriptFunctionCall

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Dimitri Glazkov (Google)]

Following up on bug 24524, do the same for ConsoleMessage.

[Dimitri Glazkov (Google)]

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(-)

[Timothy Hatcher]

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.

[Dimitri Glazkov (Google)]

(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.

[Timothy Hatcher]

Comment on attachment 28662 [details]
Use ScriptFuncitonCall with ConsoleMessage, v1

r+ with the changes we talked about.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/41765.