[webkit-reviews] review denied: [Bug 37502] Web Inspector: Removes public callLocation API from ScriptCallStack and replaces with ability to get top 10 stack frames : [Attachment 55848] Fixes style issues and updates chromium deps
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon May 17 11:10:05 PDT 2010
jaimeyap at google.com has denied review:
Bug 37502: Web Inspector: Removes public callLocation API from ScriptCallStack
and replaces with ability to get top 10 stack frames
https://bugs.webkit.org/show_bug.cgi?id=37502
Attachment 55848: Fixes style issues and updates chromium deps
https://bugs.webkit.org/attachment.cgi?id=55848&action=review
------- Additional Comments from jaimeyap at google.com
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 59177)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,28 @@
> +2010-05-11 Jaime Yap <jaimeyap at google.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Removes public callLocation API and utility context from
ScriptCallStack.
> + Adds ability to grab the top 5 JavaScript stack frames for each
timeline
> + record using new fast V8 stack trace API.
> + https://bugs.webkit.org/show_bug.cgi?id=37502
> +
> + * bindings/js/ScriptCallStack.cpp:
> + (WebCore::ScriptCallStack::stackTrace):
> + * bindings/js/ScriptCallStack.h:
> + * bindings/v8/ScriptArray.h:
> + (WebCore::ScriptArray::ScriptArray):
> + * bindings/v8/ScriptCallStack.cpp:
> + (WebCore::ScriptCallStack::callLocation):
> + (WebCore::ScriptCallStack::stackTrace):
> + * bindings/v8/ScriptCallStack.h:
> + * inspector/TimelineRecordFactory.cpp:
> + (WebCore::TimelineRecordFactory::createGenericRecord):
> + * inspector/front-end/TimelinePanel.js:
> + (WebInspector.TimelinePanel.FormattedRecord):
> +
(WebInspector.TimelinePanel.FormattedRecord.prototype._generatePopupContent):
> +
(WebInspector.TimelinePanel.FormattedRecord.prototype._getRecordDetails):
> +
> 2010-05-11 Mark Rowe <mrowe at apple.com>
>
> Fix the world.
> Index: WebCore/bindings/js/ScriptCallStack.cpp
> ===================================================================
> --- WebCore/bindings/js/ScriptCallStack.cpp (revision 59174)
> +++ WebCore/bindings/js/ScriptCallStack.cpp (working copy)
> @@ -101,7 +101,7 @@ void ScriptCallStack::initialize()
> m_initialized = true;
> }
>
> -bool ScriptCallStack::callLocation(String*, int*, String*)
> +bool ScriptCallStack::stackTrace(int, ScriptState*, ScriptArray&)
> {
> return false;
> }
> Index: WebCore/bindings/js/ScriptCallStack.h
> ===================================================================
> --- WebCore/bindings/js/ScriptCallStack.h (revision 59174)
> +++ WebCore/bindings/js/ScriptCallStack.h (working copy)
> @@ -31,6 +31,7 @@
> #ifndef ScriptCallStack_h
> #define ScriptCallStack_h
>
> +#include "ScriptArray.h"
> #include "ScriptCallFrame.h"
> #include "ScriptState.h"
> #include "ScriptString.h"
> @@ -53,7 +54,7 @@ namespace WebCore {
> // frame retrieval methods
> const ScriptCallFrame &at(unsigned);
> unsigned size();
> - static bool callLocation(String*, int*, String*);
> + static bool stackTrace(int, ScriptState*, ScriptArray&);
>
> private:
> void initialize();
> Index: WebCore/bindings/v8/ScriptArray.h
> ===================================================================
> --- WebCore/bindings/v8/ScriptArray.h (revision 59174)
> +++ WebCore/bindings/v8/ScriptArray.h (working copy)
> @@ -41,6 +41,7 @@ namespace WebCore {
> class ScriptArray : public ScriptObject {
> public:
> ScriptArray(ScriptState* scriptState, v8::Handle<v8::Array>);
> + ScriptArray() {};
> virtual ~ScriptArray() {}
>
> bool set(unsigned index, const ScriptObject&);
> Index: WebCore/bindings/v8/ScriptCallStack.cpp
> ===================================================================
> --- WebCore/bindings/v8/ScriptCallStack.cpp (revision 59174)
> +++ WebCore/bindings/v8/ScriptCallStack.cpp (working copy)
> @@ -31,6 +31,7 @@
> #include "config.h"
> #include "ScriptCallStack.h"
>
> +#include "ScriptScope.h"
> #include "ScriptController.h"
> #include "ScriptDebugServer.h"
> #include "V8Binding.h"
> @@ -41,8 +42,6 @@
>
> namespace WebCore {
>
> -v8::Persistent<v8::Context> ScriptCallStack::s_utilityContext;
> -
> ScriptCallStack* ScriptCallStack::create(const v8::Arguments& arguments,
unsigned skipArgumentCount) {
> String sourceName;
> int sourceLineNumber;
> @@ -54,9 +53,22 @@ ScriptCallStack* ScriptCallStack::create
>
> bool ScriptCallStack::callLocation(String* sourceName, int*
sourceLineNumber, String* functionName)
> {
> - if (!topStackFrame(*sourceName, *sourceLineNumber, *functionName))
> - return false;
> - *sourceLineNumber += 1;
> + v8::HandleScope scope;
> + v8::Context::Scope contextScope(v8::Context::GetCurrent());
> + v8::Handle<v8::StackTrace>
stackTrace(v8::StackTrace::CurrentStackTrace(1));
> + if (stackTrace.IsEmpty())
> + return false;
> + if (stackTrace->GetFrameCount() <= 0) {
> + // Fallback to setting lineNumber to 0, and source and function name
to "undefined".
> + *sourceName = toWebCoreString(v8::Undefined());
> + *sourceLineNumber = 0;
> + *functionName = toWebCoreString(v8::Undefined());
> + return true;
> + }
> + v8::Handle<v8::StackFrame> frame = stackTrace->GetFrame(0);
> + *sourceName = toWebCoreString(frame->GetScriptName());
> + *sourceLineNumber = frame->GetLineNumber();
> + *functionName = toWebCoreString(frame->GetFunctionName());
> return true;
> }
>
> @@ -78,66 +90,13 @@ const ScriptCallFrame& ScriptCallStack::
> return m_lastCaller;
> }
>
> -// Create the utility context for holding JavaScript functions used
internally
> -// which are not visible to JavaScript executing on the page.
> -void ScriptCallStack::createUtilityContext()
> +bool ScriptCallStack::stackTrace(int frameLimit, ScriptState* state,
ScriptArray& stackTrace)
> {
> - ASSERT(s_utilityContext.IsEmpty());
> -
> - v8::HandleScope scope;
> - v8::Handle<v8::ObjectTemplate> globalTemplate =
v8::ObjectTemplate::New();
> - s_utilityContext = v8::Context::New(0, globalTemplate);
> - v8::Context::Scope contextScope(s_utilityContext);
> -
> - // Compile JavaScript function for retrieving the source line, the
source
> - // name and the symbol name for the top JavaScript stack frame.
> - DEFINE_STATIC_LOCAL(const char*, topStackFrame,
> - ("function topStackFrame(exec_state) {"
> - " if (!exec_state.frameCount())"
> - " return undefined;"
> - " var frame = exec_state.frame(0);"
> - " var func = frame.func();"
> - " var scriptName;"
> - " if (func.resolved() && func.script())"
> - " scriptName = func.script().name();"
> - " return [scriptName, frame.sourceLine(), (func.name() ||
func.inferredName())];"
> - "}"));
> - v8::Script::Compile(v8::String::New(topStackFrame))->Run();
> -}
> -
> -bool ScriptCallStack::topStackFrame(String& sourceName, int& lineNumber,
String& functionName)
> -{
> - v8::HandleScope scope;
> - v8::Handle<v8::Context> v8UtilityContext = utilityContext();
> - if (v8UtilityContext.IsEmpty())
> - return false;
> - v8::Context::Scope contextScope(v8UtilityContext);
> - v8::Handle<v8::Function> topStackFrame;
> - topStackFrame =
v8::Local<v8::Function>::Cast(v8UtilityContext->Global()->Get(v8::String::New("
topStackFrame")));
> - if (topStackFrame.IsEmpty())
> - return false;
> - v8::Handle<v8::Value> value = v8::Debug::Call(topStackFrame);
> - if (value.IsEmpty())
> - return false;
> - // If there is no top stack frame, we still return success, but fill the
input params with defaults.
> - if (value->IsUndefined()) {
> - // Fallback to setting lineNumber to 0, and source and function name
to "undefined".
> - sourceName = toWebCoreString(value);
> - lineNumber = 0;
> - functionName = toWebCoreString(value);
> - return true;
> - }
> - if (!value->IsArray())
> - return false;
> - v8::Local<v8::Object> jsArray = value->ToObject();
> - v8::Local<v8::Value> sourceNameValue = jsArray->Get(0);
> - v8::Local<v8::Value> lineNumberValue = jsArray->Get(1);
> - v8::Local<v8::Value> functionNameValue = jsArray->Get(2);
> - if (sourceNameValue.IsEmpty() || lineNumberValue.IsEmpty() ||
functionNameValue.IsEmpty())
> + ScriptScope scope(state);
> + v8::Handle<v8::StackTrace>
trace(v8::StackTrace::CurrentStackTrace(frameLimit));
> + if (trace.IsEmpty() || !trace->GetFrameCount())
> return false;
> - sourceName = toWebCoreString(sourceNameValue);
> - lineNumber = lineNumberValue->Int32Value();
> - functionName = toWebCoreString(functionNameValue);
> + stackTrace = ScriptArray(state, trace->AsArray());
> return true;
> }
>
> Index: WebCore/bindings/v8/ScriptCallStack.h
> ===================================================================
> --- WebCore/bindings/v8/ScriptCallStack.h (revision 59174)
> +++ WebCore/bindings/v8/ScriptCallStack.h (working copy)
> @@ -31,6 +31,7 @@
> #ifndef ScriptCallStack_h
> #define ScriptCallStack_h
>
> +#include "ScriptArray.h"
> #include "ScriptCallFrame.h"
> #include "ScriptState.h"
> #include "ScriptValue.h"
> @@ -47,7 +48,16 @@ public:
> static ScriptCallStack* create(const v8::Arguments&, unsigned
skipArgumentCount = 0);
> ~ScriptCallStack();
>
> - static bool callLocation(String* sourceName, int* sourceLineNumber,
String* functionName);
> + // Returns false if there is no running JavaScript or if fetching the
stack failed.
> + // Sets stackTrace to be an array of stack frame objects.
> + // A stack frame object looks like:
> + // {
> + // scriptName: <file name for the associated script resource>
> + // functionName: <name of the JavaScript function>
> + // lineNumber: <1 based line number>
> + // column: <1 based column offset on the line>
> + // }
> + static bool stackTrace(int frameLimit, ScriptState* state, ScriptArray&
stackTrace);
>
> const ScriptCallFrame& at(unsigned) const;
> // FIXME: implement retrieving and storing call stack trace
> @@ -59,30 +69,10 @@ public:
> private:
> ScriptCallStack(const v8::Arguments& arguments, unsigned
skipArgumentCount, String sourceName, int sourceLineNumber, String funcName);
>
> - // Function for retrieving the source name, line number and function
name for the top
> - // JavaScript stack frame.
> - //
> - // It will return true if the caller information was successfully
retrieved and written
> - // into the function parameters, otherwise the function will return
false. It may
> - // fail due to a stack overflow in the underlying JavaScript
implementation, handling
> - // of such exception is up to the caller.
> - static bool topStackFrame(String& sourceName, int& lineNumber, String&
functionName);
> -
> - static void createUtilityContext();
> -
> - // Returns a local handle of the utility context.
> - static v8::Local<v8::Context> utilityContext()
> - {
> - if (s_utilityContext.IsEmpty())
> - createUtilityContext();
> - return v8::Local<v8::Context>::New(s_utilityContext);
> - }
> + static bool callLocation(String* sourceName, int* sourceLineNumber,
String* functionName);
>
> ScriptCallFrame m_lastCaller;
> ScriptState* m_scriptState;
> -
> - // Utility context holding JavaScript functions used internally.
> - static v8::Persistent<v8::Context> s_utilityContext;
> };
>
> } // namespace WebCore
> Index: WebCore/inspector/TimelineRecordFactory.cpp
> ===================================================================
> --- WebCore/inspector/TimelineRecordFactory.cpp (revision 59174)
> +++ WebCore/inspector/TimelineRecordFactory.cpp (working copy)
> @@ -49,14 +49,9 @@ ScriptObject TimelineRecordFactory::crea
> ScriptObject record = frontend->newScriptObject();
> record.set("startTime", startTime);
>
> - String sourceName;
> - int sourceLineNumber;
> - String functionName;
> - if (ScriptCallStack::callLocation(&sourceName, &sourceLineNumber,
&functionName) && sourceName != "undefined") {
> - record.set("callerScriptName", sourceName);
> - record.set("callerScriptLine", sourceLineNumber);
> - record.set("callerFunctionName", functionName);
> - }
> + ScriptArray stackTrace;
> + if (ScriptCallStack::stackTrace(5, frontend->scriptState(), stackTrace))
> + record.set("stackTrace", stackTrace);
> return record;
> }
>
> Index: WebCore/inspector/front-end/TimelinePanel.js
> ===================================================================
> --- WebCore/inspector/front-end/TimelinePanel.js (revision 59174)
> +++ WebCore/inspector/front-end/TimelinePanel.js (working copy)
> @@ -800,8 +800,7 @@ WebInspector.TimelinePanel.FormattedReco
> this._selfTime = this.endTime - this.startTime;
> this._lastChildEndTime = this.endTime;
> this.originalRecordForTests = record;
> - this.callerScriptName = record.callerScriptName;
> - this.callerScriptLine = record.callerScriptLine;
> + this.stackTrace = record.stackTrace;
> this.totalHeapSize = record.totalHeapSize;
> this.usedHeapSize = record.usedHeapSize;
>
> @@ -830,8 +829,11 @@ WebInspector.TimelinePanel.FormattedReco
> } else if (record.type === recordTypes.TimerFire) {
> var timerInstalledRecord = timerRecords[record.data.timerId];
> if (timerInstalledRecord) {
> - this.callSiteScriptName = timerInstalledRecord.callerScriptName;
> - this.callSiteScriptLine = timerInstalledRecord.callerScriptLine;
> + if (timerInstalledRecord.stackTrace) {
> + var callSite = timerInstalledRecord.stackTrace[0];
> + this.callSiteScriptName = callSite.scriptName;
> + this.callSiteScriptLine = callSite.lineNumber;
> + }
> this.timeout = timerInstalledRecord.timeout;
> this.singleShot = timerInstalledRecord.singleShot;
> }
> @@ -931,8 +933,10 @@ WebInspector.TimelinePanel.FormattedReco
> if (this.data.scriptName && this.type !== recordTypes.FunctionCall)
> contentHelper._appendLinkRow("Function Call",
this.data.scriptName, this.data.scriptLine);
>
> - if (this.callerScriptName && this.type !== recordTypes.GCEvent)
> - contentHelper._appendLinkRow("Caller", this.callerScriptName,
this.callerScriptLine);
> + if (this.stackTrace && this.type !== recordTypes.GCEvent) {
> + var callSite = this.stackTrace[0];
> + contentHelper._appendLinkRow("Caller", callSite.scriptName,
callSite.lineNumber);
> + }
>
> if (this.usedHeapSize)
> contentHelper._appendTextRow("Used Heap Size",
WebInspector.UIString("%s of %s", Number.bytesToString(this.usedHeapSize),
Number.bytesToString(this.totalHeapSize)));
> @@ -955,7 +959,8 @@ WebInspector.TimelinePanel.FormattedReco
> return record.data.width + "\u2009\u00d7\u2009" +
record.data.height;
> case WebInspector.TimelineAgent.RecordType.TimerInstall:
> case WebInspector.TimelineAgent.RecordType.TimerRemove:
> - return this.callerScriptName ?
WebInspector.linkifyResourceAsNode(this.callerScriptName, "scripts",
this.callerScriptLine, "", "") : record.data.timerId;
> + var callSite = this.stackTrace;
> + return callSite ?
WebInspector.linkifyResourceAsNode(callSite.scriptName, "scripts",
callSite.lineNumber, "", "") : record.data.timerId;
> case WebInspector.TimelineAgent.RecordType.ParseHTML:
> case WebInspector.TimelineAgent.RecordType.RecalculateStyles:
> return this.callerScriptName ?
WebInspector.linkifyResourceAsNode(this.callerScriptName, "scripts",
this.callerScriptLine, "", "") : null;
> Index: WebKit/chromium/DEPS
> ===================================================================
> --- WebKit/chromium/DEPS (revision 59174)
> +++ WebKit/chromium/DEPS (working copy)
> @@ -32,7 +32,7 @@
>
> vars = {
> 'chromium_svn': 'http://src.chromium.org/svn/trunk/src',
> - 'chromium_rev': '46805',
> + 'chromium_rev': '46991',
>
> 'pthreads-win32_rev': '26716',
> }
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog (revision 59177)
> +++ LayoutTests/ChangeLog (working copy)
> @@ -1,3 +1,17 @@
> +2010-05-11 Jaime Yap <jaimeyap at google.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Test expectations changed due to new stackTrace field on timeline
records.
> + https://bugs.webkit.org/show_bug.cgi?id=37502
> +
> + * inspector/timeline-test.js:
> + *
platform/chromium-win/inspector/timeline-event-dispatch-expected.txt:
> + *
platform/chromium-win/inspector/timeline-mark-timeline-expected.txt:
> + *
platform/chromium-win/inspector/timeline-network-resource-expected.txt:
> + * platform/chromium-win/inspector/timeline-paint-expected.txt:
> + * platform/chromium-win/inspector/timeline-parse-html-expected.txt:
> +
> 2010-05-11 Dimitri Glazkov <dglazkov at chromium.org>
>
> Reviewed by Darin Adler.
> Index: LayoutTests/inspector/timeline-test.js
> ===================================================================
> --- LayoutTests/inspector/timeline-test.js (revision 59174)
> +++ LayoutTests/inspector/timeline-test.js (working copy)
> @@ -8,9 +8,7 @@ var timelineNonDeterministicProps = {
> identifier : 1,
> startTime : 1,
> width : 1,
> - callerScriptName: 1,
> - callerScriptLine: 1,
> - callerFunctionName: 1,
> + stackTrace: 1,
> url : 1,
> usedHeapSize: 1,
> totalHeapSize: 1,
> Index:
LayoutTests/platform/chromium-win/inspector/timeline-event-dispatch-expected.tx
t
> ===================================================================
> ---
LayoutTests/platform/chromium-win/inspector/timeline-event-dispatch-expected.tx
t (revision 59174)
> +++
LayoutTests/platform/chromium-win/inspector/timeline-event-dispatch-expected.tx
t (working copy)
> @@ -3,9 +3,7 @@ Tests the Timeline API instrumentation o
> Test Mouse Target
> EventDispatch Properties:
> + startTime : * DEFINED *
> -+ callerScriptName : * DEFINED *
> -+ callerScriptLine : * DEFINED *
> -+ callerFunctionName : * DEFINED *
> ++ stackTrace : * DEFINED *
> + data : {
> +- type : mousedown
> + }
> Index:
LayoutTests/platform/chromium-win/inspector/timeline-mark-timeline-expected.txt
> ===================================================================
> ---
LayoutTests/platform/chromium-win/inspector/timeline-mark-timeline-expected.txt
(revision 59174)
> +++
LayoutTests/platform/chromium-win/inspector/timeline-mark-timeline-expected.txt
(working copy)
> @@ -2,9 +2,7 @@ Tests the Timeline API mark feature
>
> MarkTimeline Properties:
> + startTime : * DEFINED *
> -+ callerScriptName : * DEFINED *
> -+ callerScriptLine : * DEFINED *
> -+ callerFunctionName : * DEFINED *
> ++ stackTrace : * DEFINED *
> + data : {
> +- message : MARK TIMELINE
> + }
> Index:
LayoutTests/platform/chromium-win/inspector/timeline-network-resource-expected.
txt
> ===================================================================
> ---
LayoutTests/platform/chromium-win/inspector/timeline-network-resource-expected.
txt (revision 59174)
> +++
LayoutTests/platform/chromium-win/inspector/timeline-network-resource-expected.
txt (working copy)
> @@ -3,9 +3,7 @@ Tests the Timeline API instrumentation o
>
> ResourceSendRequest Properties:
> + startTime : * DEFINED *
> -+ callerScriptName : * DEFINED *
> -+ callerScriptLine : * DEFINED *
> -+ callerFunctionName : * DEFINED *
> ++ stackTrace : * DEFINED *
> + data : {
> +- identifier : * DEFINED *
> +- url : * DEFINED *
> Index:
LayoutTests/platform/chromium-win/inspector/timeline-paint-expected.txt
> ===================================================================
> --- LayoutTests/platform/chromium-win/inspector/timeline-paint-expected.txt
(revision 59174)
> +++ LayoutTests/platform/chromium-win/inspector/timeline-paint-expected.txt
(working copy)
> @@ -2,9 +2,7 @@ Tests the Timeline API instrumentation o
>
> Paint Properties:
> + startTime : * DEFINED *
> -+ callerScriptName : * DEFINED *
> -+ callerScriptLine : * DEFINED *
> -+ callerFunctionName : * DEFINED *
> ++ stackTrace : * DEFINED *
> + data : {
> +- x : 0
> +- y : 0
> Index:
LayoutTests/platform/chromium-win/inspector/timeline-parse-html-expected.txt
> ===================================================================
> ---
LayoutTests/platform/chromium-win/inspector/timeline-parse-html-expected.txt
(revision 59174)
> +++
LayoutTests/platform/chromium-win/inspector/timeline-parse-html-expected.txt
(working copy)
> @@ -2,9 +2,7 @@ Tests the Timeline API instrumentation o
>
> ParseHTML Properties:
> + startTime : * DEFINED *
> -+ callerScriptName : * DEFINED *
> -+ callerScriptLine : * DEFINED *
> -+ callerFunctionName : * DEFINED *
> ++ stackTrace : * DEFINED *
> + data : {
> +- length : 9
> +- startLine : 0
R-ing this obsolete patch so that it no longer shows up in commit queue.
More information about the webkit-reviews
mailing list