Bug 32314 - [Chromium] Introduce a class for devtools message data
Summary: [Chromium] Introduce a class for devtools message data
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: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-09 01:55 PST by Yury Semikhatsky
Modified: 2009-12-09 08:03 PST (History)
3 users (show)

See Also:


Attachments
patch (6.58 KB, patch)
2009-12-09 02:17 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
patch (7.55 KB, patch)
2009-12-09 02:57 PST, Yury Semikhatsky
pfeldman: review+
Details | Formatted Diff | Diff
patch (2.02 KB, patch)
2009-12-09 06:56 PST, Yury Semikhatsky
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2009-12-09 01:55:41 PST
Currently devtools message passing interface allows to send messages with exactly 3 params, class and method names. Having a class describing message payload would add more flexibility.
Comment 1 Yury Semikhatsky 2009-12-09 02:17:40 PST
Created attachment 44525 [details]
patch
Comment 2 WebKit Review Bot 2009-12-09 02:21:36 PST
style-queue ran check-webkit-style on attachment 44525 [details] without any errors.
Comment 3 Yury Semikhatsky 2009-12-09 02:57:23 PST
Created attachment 44528 [details]
patch
Comment 4 WebKit Review Bot 2009-12-09 03:02:36 PST
style-queue ran check-webkit-style on attachment 44528 [details] without any errors.
Comment 5 Pavel Feldman 2009-12-09 03:18:26 PST
Comment on attachment 44528 [details]
patch

> +    virtual void sendMessageToFrontend(const WebDevToolsMessageData&) {}

nit: { } here and everywhere else.

>  
> +template <typename T> class WebVector;
> +

remove this.
Comment 6 Yury Semikhatsky 2009-12-09 05:21:30 PST
(In reply to comment #5)
> (From update of attachment 44528 [details])
> > +    virtual void sendMessageToFrontend(const WebDevToolsMessageData&) {}
> 
> nit: { } here and everywhere else.
> 
Done.

> >  
> > +template <typename T> class WebVector;
> > +
> 
> remove this.
Done.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	C	WebKit/chromium/public/WebDevToolsFrontendClient.h => WebKit/chromium/public/WebDevToolsMessageData.h
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/WebKit.gyp
	M	WebKit/chromium/public/WebDevToolsAgent.h
	M	WebKit/chromium/public/WebDevToolsAgentClient.h
	M	WebKit/chromium/public/WebDevToolsFrontend.h
	M	WebKit/chromium/public/WebDevToolsFrontendClient.h
Committed r51897
Comment 7 Yury Semikhatsky 2009-12-09 06:55:47 PST
Reopening the bug since we need to add methods for dispatching devtools messages on the IO thread that accept WebDevToolsMessageData parameter.
Comment 8 Yury Semikhatsky 2009-12-09 06:56:34 PST
Created attachment 44534 [details]
patch
Comment 9 Pavel Feldman 2009-12-09 06:57:51 PST
Comment on attachment 44534 [details]
patch

Add TODO to remove the original one.
Comment 10 Yury Semikhatsky 2009-12-09 07:05:43 PST
(In reply to comment #9)
> (From update of attachment 44534 [details])
> Add TODO to remove the original one.
Done.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/public/WebDevToolsAgent.h
	M	WebKit/chromium/public/WebDevToolsAgentClient.h
Committed r51899
Comment 11 Darin Fisher (:fishd, Google) 2009-12-09 07:48:31 PST
Comment on attachment 44534 [details]
patch

> +    WEBKIT_API static bool dispatchMessageFromFrontendOnIOThread(const WebDevToolsMessageData&);

It seems unfortunate that we need to mention things like "IOThread" in WebKit.  That's
an implementation detail of Chromium.  Is it possible to avoid specifying details like
this that cannot be understood without understanding Chromium?
Comment 12 Pavel Feldman 2009-12-09 08:03:04 PST
(In reply to comment #11)
> (From update of attachment 44534 [details])
> > +    WEBKIT_API static bool dispatchMessageFromFrontendOnIOThread(const WebDevToolsMessageData&);
> 
> It seems unfortunate that we need to mention things like "IOThread" in WebKit. 
> That's
> an implementation detail of Chromium.  Is it possible to avoid specifying
> details like
> this that cannot be understood without understanding Chromium?

It is named so on purpose. We need to execute these commands asynchronously wrt render thread. In our case it is IO thread that handles async dev tools agent <-> frontend communication. I do value abstraction a lot, but in this case I think we should be more transparent. It is interesting that devtools is the only feature that talks to WebKit/chromium on non-render thread. Or is there any other?