Bug 47968

Summary: Web Inspector: Handle WebSocket events via InspectorInstrumentation
Product: WebKit Reporter: Yuta Kitamura <yutak>
Component: Web Inspector (Deprecated)Assignee: Yuta Kitamura <yutak>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, beidson, bweinstein, commit-queue, eric, joepeck, keishi, loislo, pfeldman, pmuellr, podivilov, rik, timothy, ukai, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch v2 none

Description Yuta Kitamura 2010-10-19 23:52:40 PDT
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.
Comment 1 Yuta Kitamura 2010-10-20 00:04:25 PDT
Created attachment 71256 [details]
Patch
Comment 2 Fumitoshi Ukai 2010-10-20 00:15:28 PDT
Comment on attachment 71256 [details]
Patch

LGTM
Comment 3 Pavel Podivilov 2010-10-20 01:18:09 PDT
Looks good.
Comment 4 Yury Semikhatsky 2010-10-20 01:28:09 PDT
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.
Comment 5 Yuta Kitamura 2010-10-20 02:24:00 PDT
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.
Comment 6 Yuta Kitamura 2010-10-20 05:20:16 PDT
Created attachment 71276 [details]
Patch v2
Comment 7 WebKit Commit Bot 2010-10-20 20:59:33 PDT
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.
Comment 8 WebKit Commit Bot 2010-10-20 22:30:37 PDT
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.
Comment 9 WebKit Commit Bot 2010-10-21 02:23:52 PDT
Comment on attachment 71276 [details]
Patch v2

Clearing flags on attachment: 71276

Committed r70216: <http://trac.webkit.org/changeset/70216>
Comment 10 WebKit Commit Bot 2010-10-21 02:23:59 PDT
All reviewed patches have been landed.  Closing bug.