Bug 47968 – Web Inspector: Handle WebSocket events via InspectorInstrumentation

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

[Yuta Kitamura]

Putting WebSocket-related APIs to Inspector into InspectorInstrumentation class.

This change reduces the number of "#if ENABLE(INSPECTOR)" blocks from WebSocketChannel and will help when I will add WebSocket stuff in the Timeline panel later.

I will upload a patch for review shortly.

[Yuta Kitamura]

Created attachment 71256 [details]
Patch

[Fumitoshi Ukai]

Comment on attachment 71256 [details]
Patch

LGTM

[Pavel Podivilov]

Looks good.

[Yury Semikhatsky]

Comment on attachment 71256 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71256&action=review

> WebCore/websockets/WebSocketChannel.cpp:39
>  #include "InspectorController.h"

Please remove this import, InspectorInstrumentation should be enough.

> WebCore/websockets/WebSocketChannel.cpp:75
> +    InspectorInstrumentation::didCreateWebSocket(m_context, identifier(), url, m_context->url());

Now that you removed #if ENABLE(INSPECTOR) guard, identifier() will be called even when inspector is disabled, but the method is behind the guard:

	#if ENABLE(INSPECTOR)
	unsigned long WebSocketChannel::identifier()

r- for this.

[Yuta Kitamura]

Good catch, will fix. Thanks.

(In reply to comment #4)
> (From update of attachment 71256 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71256&action=review
> 
> > WebCore/websockets/WebSocketChannel.cpp:39
> >  #include "InspectorController.h"
> 
> Please remove this import, InspectorInstrumentation should be enough.

Currently we use InspectorController to obtain resource identifier
(controller->inspectedPage()->progress()->createUniqueIdentifier()).

I have now realized that we can obtain a Page from ScriptExecutionContext*,
so I will rewrite the code and remove this #include line.

> 
> > WebCore/websockets/WebSocketChannel.cpp:75
> > +    InspectorInstrumentation::didCreateWebSocket(m_context, identifier(), url, m_context->url());
> 
> Now that you removed #if ENABLE(INSPECTOR) guard, identifier() will be called even when inspector is disabled, but the method is behind the guard:
> 
>     #if ENABLE(INSPECTOR)
>     unsigned long WebSocketChannel::identifier()

Absolutely. Thank you for pointing this out.

> 
> r- for this.

[Yuta Kitamura]

Created attachment 71276 [details]
Patch v2

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 71276 [details]:

http/tests/xmlhttprequest/origin-whitelisting-removal.html
http/tests/appcache/non-html.xhtml

Please file bugs against the tests.  The author(s) of the test(s) have been CCed on this bug.  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 71276 [details]:

http/tests/appcache/update-cache.html
fast/loader/recursive-before-unload-crash.html

Please file bugs against the tests.  The author(s) of the test(s) have been CCed on this bug.  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 71276 [details]
Patch v2

Clearing flags on attachment: 71276

Committed r70216: <http://trac.webkit.org/changeset/70216>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.