Bug 47968 - Web Inspector: Handle WebSocket events via InspectorInstrumentation
Summary: Web Inspector: Handle WebSocket events via InspectorInstrumentation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yuta Kitamura
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-19 23:52 PDT by Yuta Kitamura
Modified: 2010-10-21 02:23 PDT (History)
15 users (show)

See Also:


Attachments
Patch (10.98 KB, patch)
2010-10-20 00:04 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v2 (13.14 KB, patch)
2010-10-20 05:20 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.