Bug 105527 – [Web Inspector]Add WebSocket networking events in WebInspector Timeline panel

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[pdeng6]

Sub-bug of #38831

[pdeng6]

Created attachment 180338 [details]
WebSocket networking events shown in Timeline panel

[pdeng6]

Created attachment 180340 [details]
Patch

[WebKit Review Bot]

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 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]

Created attachment 180487 [details]
Patch

[pdeng6]

Hi Pavel, could you please or someone else in inspector team help reivew this patch? :)
thanks

Pan

[pdeng6]

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]

Created attachment 188853 [details]
Patch

[Pavel Feldman]

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]

Created attachment 189005 [details]
WebSocket networking events shown in Timeline panel V2

[pdeng6]

Created attachment 189006 [details]
Patch

[Pavel Feldman]

Comment on attachment 189006 [details]
Patch

Ok, we need a test and it is good to go.

[pdeng6]

(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]

Created attachment 189037 [details]
Patch

[pdeng6]

(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 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]

Created attachment 189063 [details]
Patch

[Pavel Feldman]

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 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]

Created attachment 189223 [details]
Patch

[pdeng6]

(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 on attachment 189223 [details]
Patch

Clearing flags on attachment: 189223

Committed r143573: <http://trac.webkit.org/changeset/143573>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andrey Kosyakov]

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]

(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