[Webkit-unassigned] [Bug 33995] Web Inspector: Additional instrumentation for Timeline

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33995


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #47195|                            |review-
               Flag|                            |




--- Comment #4 from Pavel Feldman <pfeldman at chromium.org>  2010-01-22 06:28:28 PST ---
(From update of attachment 47195)

> +        There are 3 patches should be applied: 
> +        The first one is a patch for WebInspector with popup dialog and the changes for 
> +        Caller info support for all kind of Timeline events and new Function Call event.
> +        The second one will be the patch in V8 code for Function Call info.
> +        The third one will be in V8 bindings for pushing Caller info to Timeline.
> +

What about JSC?


>  
> +int InspectorTimelineAgent::m_count = 0;
> +

s_instanceCount?

> +    copyCallerInfoToRecord(record);

populateCallerInfo?

> +        static ScriptObject createCallFunctionData(InspectorFrontend*, const String& name, int lineNumber);
> +

createFunctionCallData?

> -button.status-bar-item.toggled-1 .glyph {
> -    background-color: rgb(66, 129, 235);
> -}
> -
> -button.status-bar-item.toggled-2 .glyph {
> -    background-color: purple;   
> -}
> -

Please rebase.

>  
> +function _linkToFile(file, line, panel)
> +{

Either put this into utilities.js or declare on TimelinePanel's constructor.

> +                this._lastRecord.callFromName == formattedRecord.callFromName &&
> +                this._lastRecord.callFromLine == formattedRecord.callFromLine &&

callerURL, callerLine?


> +            this._recordStyles[recordTypes.FunctionCall] = { title: WebInspector.UIString("Function Call"), category: this.categories.scripting };

Ad this to WebCore/English.lproj/localizedStrings.js



> +        } else if (record.type === WebInspector.TimelineAgent.RecordType.TimerInstall) {
> +            this._timerRecords[record.data.timerId] = formattedRecord;
> +        } else if (record.type === WebInspector.TimelineAgent.RecordType.TimerFire) {

No need for {}

> +        case WebInspector.TimelineAgent.RecordType.FunctionCall:
> +            return record.data.scriptName + ":" + (1 + record.data.scriptLine);

Mind JSC vs V8 difference, add in bindings instead.

> +    this._callInfo = document.createElement("span");
> +    this._callInfo.className = "data dimmed";
> +    this._callInfo.textContent = "";
> +

I don't think we need this in the list.

> +    _createCell: function(content)
> +    {
> +        var text = document.createElement("label");
> +        text.className = "popup-message";
> +        text.appendChild(document.createTextNode(content));
> +        var cell = document.createElement("td");
> +        cell.appendChild(text);
> +        return cell;
> +    },
> +

Do not use table / td if possible.

> +        typedef std::pair<String, int> PositionInScript;
> +

private?

> +        void pushCallerInfo(const String& scriptName, int scriptLine) {
> +            m_callInfoStack.append(InspectorTimelineAgent::PositionInScript(scriptName, scriptLine));
> +        }
> +
> +        void popCallerInfo() { 
> +            ASSERT(m_callInfoStack.size() != 1);
> +            m_callInfoStack.shrink(m_callInfoStack.size() - 1); 
> +        }
> +

We do not generally put code into headers.

>      private:
> +        void copyCallerInfoToRecord(ScriptObject& record) {
> +            PositionInScript& callInfo = m_callInfoStack.last();
> +            record.set("callFromName", callInfo.first);
> +            record.set("callFromLine", callInfo.second);
> +        }
> +

Move to cpp

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.