Summary: | Refactor ConsoleMessage to use ScriptFunctionCall | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dimitri Glazkov (Google) <dglazkov> | ||||
Component: | Web Inspector (Deprecated) | Assignee: | Dimitri Glazkov (Google) <dglazkov> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | timothy | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Bug Depends on: | 24524 | ||||||
Bug Blocks: | 25063 | ||||||
Attachments: |
|
Description
Dimitri Glazkov (Google)
2009-03-16 09:51:22 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 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.
(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 on attachment 28662 [details]
Use ScriptFuncitonCall with ConsoleMessage, v1
r+ with the changes we talked about.
Landed as http://trac.webkit.org/changeset/41765. |