Bug 105527 - [Web Inspector]Add WebSocket networking events in WebInspector Timeline panel
Summary: [Web Inspector]Add WebSocket networking events in WebInspector Timeline panel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 38831
  Show dependency treegraph
 
Reported: 2012-12-20 04:00 PST by pdeng6
Modified: 2013-03-04 17:42 PST (History)
12 users (show)

See Also:


Attachments
WebSocket networking events shown in Timeline panel (105.75 KB, image/png)
2012-12-20 07:24 PST, pdeng6
no flags Details
Patch (32.90 KB, patch)
2012-12-20 07:34 PST, pdeng6
no flags Details | Formatted Diff | Diff
Patch (34.57 KB, patch)
2012-12-20 23:12 PST, pdeng6
no flags Details | Formatted Diff | Diff
Patch (26.94 KB, patch)
2013-02-18 04:48 PST, pdeng6
no flags Details | Formatted Diff | Diff
WebSocket networking events shown in Timeline panel V2 (146.24 KB, image/png)
2013-02-18 23:46 PST, pdeng6
no flags Details
Patch (26.20 KB, patch)
2013-02-18 23:47 PST, pdeng6
no flags Details | Formatted Diff | Diff
Patch (29.09 KB, patch)
2013-02-19 03:09 PST, pdeng6
no flags Details | Formatted Diff | Diff
Patch (29.84 KB, patch)
2013-02-19 04:59 PST, pdeng6
no flags Details | Formatted Diff | Diff
Patch (61.97 KB, patch)
2013-02-19 19:02 PST, pdeng6
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description pdeng6 2012-12-20 04:00:51 PST
Sub-bug of #38831
Comment 1 pdeng6 2012-12-20 07:24:11 PST
Created attachment 180338 [details]
WebSocket networking events shown in Timeline panel
Comment 2 pdeng6 2012-12-20 07:34:31 PST
Created attachment 180340 [details]
Patch
Comment 3 WebKit Review Bot 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
Comment 4 Build Bot 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
Comment 5 pdeng6 2012-12-20 23:12:20 PST
Created attachment 180487 [details]
Patch
Comment 6 pdeng6 2013-01-22 21:16:19 PST
Hi Pavel, could you please or someone else in inspector team help reivew this patch? :)
thanks

Pan
Comment 7 pdeng6 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. :)
Comment 8 pdeng6 2013-02-18 04:48:11 PST
Created attachment 188853 [details]
Patch
Comment 9 Pavel Feldman 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)
Comment 10 pdeng6 2013-02-18 23:46:36 PST
Created attachment 189005 [details]
WebSocket networking events shown in Timeline panel V2
Comment 11 pdeng6 2013-02-18 23:47:19 PST
Created attachment 189006 [details]
Patch
Comment 12 Pavel Feldman 2013-02-18 23:51:52 PST
Comment on attachment 189006 [details]
Patch

Ok, we need a test and it is good to go.
Comment 13 pdeng6 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
Comment 14 pdeng6 2013-02-19 03:09:14 PST
Created attachment 189037 [details]
Patch
Comment 15 pdeng6 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
Comment 16 WebKit Review Bot 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
Comment 17 pdeng6 2013-02-19 04:59:05 PST
Created attachment 189063 [details]
Patch
Comment 18 Pavel Feldman 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.
Comment 19 WebKit Review Bot 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
Comment 20 pdeng6 2013-02-19 19:02:47 PST
Created attachment 189223 [details]
Patch
Comment 21 pdeng6 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
Comment 22 WebKit Review Bot 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>
Comment 23 WebKit Review Bot 2013-02-21 01:05:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Andrey Kosyakov 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()?)
Comment 25 pdeng6 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