Sub-bug of #38831
Created attachment 180338 [details] WebSocket networking events shown in Timeline panel
Created attachment 180340 [details] Patch
Comment on attachment 180340 [details] Patch Attachment 180340 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15455062 New failing tests: inspector/timeline/timeline-enum-stability.html
Comment on attachment 180340 [details] Patch Attachment 180340 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15461189 New failing tests: inspector/timeline/timeline-enum-stability.html
Created attachment 180487 [details] Patch
Hi Pavel, could you please or someone else in inspector team help reivew this patch? :) thanks Pan
Currently, detailed timing information for websocket is not show up in inspector. Intent of this bug item is to show websocket create, handshake request/response and close in Timeline Panel. :)
Created attachment 188853 [details] Patch
Comment on attachment 188853 [details] Patch 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)
Created attachment 189005 [details] WebSocket networking events shown in Timeline panel V2
Created attachment 189006 [details] Patch
Comment on attachment 189006 [details] Patch Ok, we need a test and it is good to go.
(In reply to comment #9) > (From update of attachment 188853 [details]) > 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. > Done, thanks. > > 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. > new record types of websocket are not handled in _findParentRecord this time, so websocket record won't be glued. I save create websocket record here to keep the websocket URL for following events, I think websocket URL is useful for multiple websocket case (show them in PopupContent). > > 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) Done. I will prepare a test. Thanks for your review. Pan
Created attachment 189037 [details] Patch
(In reply to comment #12) > (From update of attachment 189006 [details]) > Ok, we need a test and it is good to go. I prepared a test for the events. however, I don't understand the output of identifier attribute are always: data : { identifier : 34 } I've thought they should be: data : { identifier : <number> } anything wrong? thanks Pan
Comment on attachment 189037 [details] Patch Attachment 189037 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16544074 New failing tests: inspector/timeline/timeline-websocket-event.html
Created attachment 189063 [details] Patch
Comment on attachment 189063 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189063&action=review Clearing r? due to failing tests. > LayoutTests/inspector/timeline/timeline-websocket-event.html:11 > + if (window.testRunner) You don't need this.
Comment on attachment 189063 [details] Patch Attachment 189063 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16611800 New failing tests: inspector/timeline/timeline-websocket-event.html
Created attachment 189223 [details] Patch
(In reply to comment #18) > (From update of attachment 189063 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189063&action=review > > Clearing r? due to failing tests. > > > LayoutTests/inspector/timeline/timeline-websocket-event.html:11 > > + if (window.testRunner) > > You don't need this. Done, thanks. The new added test needs websocket server, so I put it in http/tests/inspector/, timeline-test.js is also moved to http/tests/inspector/ since it is a need from server side. Corresponding refs in timeline/*.html are updated. Thanks! Pan
Comment on attachment 189223 [details] Patch Clearing flags on attachment: 189223 Committed r143573: <http://trac.webkit.org/changeset/143573>
All reviewed patches have been landed. Closing bug.
Comment on attachment 189223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189223&action=review > Source/WebCore/inspector/InspectorTimelineAgent.cpp:503 > + RefPtr<InspectorObject> record = TimelineRecordFactory::createGenericRecord(WTF::currentTimeMS(), m_maxCallStackDepth); Is there any reason this uses time that is potentially different from other timeline records (WTF::currentTimeMS() vs. TimelineAgent::timestamp()?)
(In reply to comment #24) > (From update of attachment 189223 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189223&action=review > > > Source/WebCore/inspector/InspectorTimelineAgent.cpp:503 > > + RefPtr<InspectorObject> record = TimelineRecordFactory::createGenericRecord(WTF::currentTimeMS(), m_maxCallStackDepth); > > Is there any reason this uses time that is potentially different from other timeline records (WTF::currentTimeMS() vs. TimelineAgent::timestamp()?) no special intent, TimelineAgent::timestamp() is the right one. :) thanks! Pan