Add content API for DevTools HTTP handler

Add content API for DevTools HTTP handler


BUG=104625
TEST=Existing tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112662

[jam, 2011-11-21 19:26:56]

thanks for doing this, some comments

http://codereview.chromium.org/8554008/diff/1001/chrome/browser/debugger/remo...
File chrome/browser/debugger/remote_debugging_server.h (right):

http://codereview.chromium.org/8554008/diff/1001/chrome/browser/debugger/remo...
chrome/browser/debugger/remote_debugging_server.h:23: const std::string& ip,
nit: while you're modifying this file, tabbing

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
File content/public/browser/devtools/devtools_http_handler.h (right):

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler.h:12: #include
"base/memory/scoped_ptr.h"
nit: not needed

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler.h:19: class
DevToolsHttpHandler
nit: please add a comment about what this class is used for

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler.h:20: : public
base::RefCountedThreadSafe<DevToolsHttpHandler> {
it seems that the refcounting is an internal implementation detail. do we have
to expose that to embedders? can we structure DevToolsHttpHandlerImpl such that
it has an inner context class that is refcounted, but the interface itself is
not? i.e. see this example http://codereview.chromium.org/8603012/

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler.h:23: CONTENT_EXPORT
static scoped_refptr<DevToolsHttpHandler> Start(
nit: usually for functions that construct an object, we call it "Create". also,
i'm curious why you're returning a scoped_refptr instead of a pointer? it seems
obvious that whoever calls a Create function on a refcounted object will want to
put in a scoped_refptr

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler.h:34:
DevToolsHttpHandler() {}
nit: not necessary

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler.h:40:
DISALLOW_COPY_AND_ASSIGN(DevToolsHttpHandler);
(and also in the other interface)

nit: no need for this macro on interfaces. these objects aren't instantiable so
they can't be copied or assigned in the first place

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
File content/public/browser/devtools/devtools_http_handler_delegate.h (right):

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler_delegate.h:24: class
CONTENT_EXPORT DevToolsHttpHandlerDelegate {
ditto

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler_delegate.h:27:
DevToolsHttpHandlerDelegate() {}
nit: not needed

[yurys, 2011-12-01 18:02:20]

http://codereview.chromium.org/8554008/diff/1001/chrome/browser/debugger/remo...
File chrome/browser/debugger/remote_debugging_server.h (right):

http://codereview.chromium.org/8554008/diff/1001/chrome/browser/debugger/remo...
chrome/browser/debugger/remote_debugging_server.h:23: const std::string& ip,
On 2011/11/21 19:26:56, John Abd-El-Malek wrote:
> nit: while you're modifying this file, tabbing

Done.

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
File content/public/browser/devtools/devtools_http_handler.h (right):

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler.h:12: #include
"base/memory/scoped_ptr.h"
On 2011/11/21 19:26:56, John Abd-El-Malek wrote:
> nit: not needed

Done.

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler.h:19: class
DevToolsHttpHandler
On 2011/11/21 19:26:56, John Abd-El-Malek wrote:
> nit: please add a comment about what this class is used for

Done.

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler.h:20: : public
base::RefCountedThreadSafe<DevToolsHttpHandler> {
On 2011/11/21 19:26:56, John Abd-El-Malek wrote:
> it seems that the refcounting is an internal implementation detail. do we have
> to expose that to embedders? can we structure DevToolsHttpHandlerImpl such
that
> it has an inner context class that is refcounted, but the interface itself is
> not? i.e. see this example http://codereview.chromium.org/8603012/

Done. I made DevToolsHttpHandlerImpl refcounted but DevToolsHttpHandler is not
refcounted and has protected destructor so the only way for the API clients to
delete the handler is to call Stop on it. The implementation class keeps
scoped_refptr to self until Stop is called on it. Please let me know if you
think that converting DevToolsHttpHandlerImpl into some internal refcounted
context would work better.

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler.h:23: CONTENT_EXPORT
static scoped_refptr<DevToolsHttpHandler> Start(
On 2011/11/21 19:26:56, John Abd-El-Malek wrote:
> nit: usually for functions that construct an object, we call it "Create".
also,
> i'm curious why you're returning a scoped_refptr instead of a pointer? it
seems
> obvious that whoever calls a Create function on a refcounted object will want
to
> put in a scoped_refptr 

Done. This class is not refcounted anymore. I think reason for returning here
scoped_refptr was to force clients to assign this object to a scoped_refptr when
returning a raw pointer compiler won't help you if you miss that.

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler.h:34:
DevToolsHttpHandler() {}
On 2011/11/21 19:26:56, John Abd-El-Malek wrote:
> nit: not necessary

Done.

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler.h:40:
DISALLOW_COPY_AND_ASSIGN(DevToolsHttpHandler);
On 2011/11/21 19:26:56, John Abd-El-Malek wrote:
> (and also in the other interface)
> 
> nit: no need for this macro on interfaces. these objects aren't instantiable
so
> they can't be copied or assigned in the first place

Done.

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
File content/public/browser/devtools/devtools_http_handler_delegate.h (right):

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler_delegate.h:24: class
CONTENT_EXPORT DevToolsHttpHandlerDelegate {
On 2011/11/21 19:26:56, John Abd-El-Malek wrote:
> ditto

Done.

http://codereview.chromium.org/8554008/diff/1001/content/public/browser/devto...
content/public/browser/devtools/devtools_http_handler_delegate.h:27:
DevToolsHttpHandlerDelegate() {}
On 2011/11/21 19:26:56, John Abd-El-Malek wrote:
> nit: not needed

Done.

[jam, 2011-12-01 19:04:27]

lgtm, thanks

http://codereview.chromium.org/8554008/diff/7001/content/public/browser/devto...
File content/public/browser/devtools_http_handler.h (right):

http://codereview.chromium.org/8554008/diff/7001/content/public/browser/devto...
content/public/browser/devtools_http_handler.h:30: // Will schedule tear down
task on the IO thread.
nit: no need to explain what the function does internally in its API

[yurys, 2011-12-02 06:42:59]

http://codereview.chromium.org/8554008/diff/7001/content/public/browser/devto...
File content/public/browser/devtools_http_handler.h (right):

http://codereview.chromium.org/8554008/diff/7001/content/public/browser/devto...
content/public/browser/devtools_http_handler.h:30: // Will schedule tear down
task on the IO thread.
On 2011/12/01 19:04:27, John Abd-El-Malek wrote:
> nit: no need to explain what the function does internally in its API

Done.