[webkit-reviews] review denied: [Bug 105527] [Web Inspector]Add WebSocket networking events in WebInspector Timeline panel : [Attachment 188853] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied pdeng6 <pan.deng at intel.com>'s
request for review:
Bug 105527: [Web Inspector]Add WebSocket networking events in WebInspector
Timeline panel
https://bugs.webkit.org/show_bug.cgi?id=105527

Attachment 188853: Patch
https://bugs.webkit.org/attachment.cgi?id=188853&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=188853&action=review


> Source/WebCore/inspector/TimelineRecordFactory.cpp:211
> +PassRefPtr<InspectorObject>
TimelineRecordFactory::createWebSocketCreateData(unsigned long identifier,
const KURL& url, const String& protocol)

Since these are only used once, you can inline them.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:376
> +	       return this._sendRequestRecords[record.data["identifier"]];

I am not sure we want to glue web socket records. I'd rather not.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:677
> +	   presentationModel._sendRequestRecords[record.data["identifier"]] =
this;

No glueing required, drop this.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:683
> +	   var webSocketCreateRecord =
presentationModel._sendRequestRecords[record.data["identifier"]];

ditto

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:952
> +		      
contentHelper._appendTextRow(WebInspector.UIString("URL"), this.webSocketURL);

Strings should be a part of localizedStrings.js (under WebCore/English.lproj)