Bug 105527

Summary: [Web Inspector]Add WebSocket networking events in WebInspector Timeline panel
Product: WebKit Reporter: pdeng6 <pan.deng>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, dglazkov, keishi, loislo, pfeldman, pmuellr, toyoshim, vsevik, web-inspector-bugs, webkit.review.bot, yurys, yutak
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 38831    
Attachments:
Description Flags
WebSocket networking events shown in Timeline panel
none
Patch
none
Patch
none
Patch
none
WebSocket networking events shown in Timeline panel V2
none
Patch
none
Patch
none
Patch
none
Patch none

pdeng6
Reported 2012-12-20 04:00:51 PST
Sub-bug of #38831
Attachments
WebSocket networking events shown in Timeline panel (105.75 KB, image/png)
2012-12-20 07:24 PST, pdeng6
no flags
Patch (32.90 KB, patch)
2012-12-20 07:34 PST, pdeng6
no flags
Patch (34.57 KB, patch)
2012-12-20 23:12 PST, pdeng6
no flags
Patch (26.94 KB, patch)
2013-02-18 04:48 PST, pdeng6
no flags
WebSocket networking events shown in Timeline panel V2 (146.24 KB, image/png)
2013-02-18 23:46 PST, pdeng6
no flags
Patch (26.20 KB, patch)
2013-02-18 23:47 PST, pdeng6
no flags
Patch (29.09 KB, patch)
2013-02-19 03:09 PST, pdeng6
no flags
Patch (29.84 KB, patch)
2013-02-19 04:59 PST, pdeng6
no flags
Patch (61.97 KB, patch)
2013-02-19 19:02 PST, pdeng6
no flags
pdeng6
Comment 1 2012-12-20 07:24:11 PST
Created attachment 180338 [details] WebSocket networking events shown in Timeline panel
pdeng6
Comment 2 2012-12-20 07:34:31 PST
WebKit Review Bot
Comment 3 2012-12-20 09:13:59 PST
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
Build Bot
Comment 4 2012-12-20 21:35:01 PST
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
pdeng6
Comment 5 2012-12-20 23:12:20 PST
pdeng6
Comment 6 2013-01-22 21:16:19 PST
Hi Pavel, could you please or someone else in inspector team help reivew this patch? :) thanks Pan
pdeng6
Comment 7 2013-02-18 04:46:42 PST
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. :)
pdeng6
Comment 8 2013-02-18 04:48:11 PST
Pavel Feldman
Comment 9 2013-02-18 05:17:02 PST
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)
pdeng6
Comment 10 2013-02-18 23:46:36 PST
Created attachment 189005 [details] WebSocket networking events shown in Timeline panel V2
pdeng6
Comment 11 2013-02-18 23:47:19 PST
Pavel Feldman
Comment 12 2013-02-18 23:51:52 PST
Comment on attachment 189006 [details] Patch Ok, we need a test and it is good to go.
pdeng6
Comment 13 2013-02-18 23:57:01 PST
(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
pdeng6
Comment 14 2013-02-19 03:09:14 PST
pdeng6
Comment 15 2013-02-19 03:12:17 PST
(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
WebKit Review Bot
Comment 16 2013-02-19 03:52:53 PST
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
pdeng6
Comment 17 2013-02-19 04:59:05 PST
Pavel Feldman
Comment 18 2013-02-19 06:14:05 PST
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.
WebKit Review Bot
Comment 19 2013-02-19 06:54:45 PST
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
pdeng6
Comment 20 2013-02-19 19:02:47 PST
pdeng6
Comment 21 2013-02-19 19:12:11 PST
(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
WebKit Review Bot
Comment 22 2013-02-21 01:05:50 PST
Comment on attachment 189223 [details] Patch Clearing flags on attachment: 189223 Committed r143573: <http://trac.webkit.org/changeset/143573>
WebKit Review Bot
Comment 23 2013-02-21 01:05:56 PST
All reviewed patches have been landed. Closing bug.
Andrey Kosyakov
Comment 24 2013-03-04 09:47:39 PST
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()?)
pdeng6
Comment 25 2013-03-04 17:42:30 PST
(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
Note You need to log in before you can comment on or make changes to this bug.