Allow customization of remote debugger URL targets.

Allow customization of remote debugger URL targets. Add a new DevToolsHttpHandler::RenderViewHostBinding interface to allow customizable mapping of page identifiers to/from RenderViewHost instances.

BUG=124395
TEST=DevTools still works

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

[Marshall, 2012-04-24 23:33:25]

Please review.

[pfeldman, 2012-04-25 05:45:15]

Overall looks good, a couple of nits in the review.

https://chromiumcodereview.appspot.com/10169043/diff/1/content/browser/debugg...
File content/browser/debugger/devtools_http_handler_impl.cc (right):

https://chromiumcodereview.appspot.com/10169043/diff/1/content/browser/debugg...
content/browser/debugger/devtools_http_handler_impl.cc:60: return
base::StringPrintf("%lu", index);
Nit: it might be a good time to migrate to the "<process_id>_<routing_id>" id
scheme. That would eliminate the need for ResetRenderViewHostBinding and would
make ids persistent across debugging sessions.

https://chromiumcodereview.appspot.com/10169043/diff/1/content/browser/debugg...
File content/browser/debugger/devtools_http_handler_impl.h (right):

https://chromiumcodereview.appspot.com/10169043/diff/1/content/browser/debugg...
content/browser/debugger/devtools_http_handler_impl.h:112:
scoped_ptr<DevToolsHttpHandlerDelegate::BindingHandler> binding_handler_;
You will often want existing class to implement this interface, I don't think
you should pass the ownership here.

https://chromiumcodereview.appspot.com/10169043/diff/1/content/public/browser...
File content/public/browser/devtools_http_handler_delegate.h (right):

https://chromiumcodereview.appspot.com/10169043/diff/1/content/public/browser...
content/public/browser/devtools_http_handler_delegate.h:21: class BindingHandler
{
If this binding is really optional, I'd rather move this interface into
DevToolsHttpHandler and call it RenderViewHostBinding. Clients will be able to
set it via DevToolsHttpHandler::SetRenderViewHostBinding.

https://chromiumcodereview.appspot.com/10169043/diff/1/content/public/browser...
content/public/browser/devtools_http_handler_delegate.h:26: virtual std::string
GetRenderViewHostIdentifier(RenderViewHost* rvh) = 0;
If you rename the interface to RenderViewHostBinding. These will be called
GetIdentifier and ForIdentifier.

[Marshall, 2012-04-25 15:14:57]

Please review.

Adding joi@ as OWNER for content/public/browser/devtools_http_handler.h review.

Thanks.

https://chromiumcodereview.appspot.com/10169043/diff/1/content/browser/debugg...
File content/browser/debugger/devtools_http_handler_impl.cc (right):

https://chromiumcodereview.appspot.com/10169043/diff/1/content/browser/debugg...
content/browser/debugger/devtools_http_handler_impl.cc:60: return
base::StringPrintf("%lu", index);
On 2012/04/25 05:45:15, pfeldman wrote:
> Nit: it might be a good time to migrate to the "<process_id>_<routing_id>" id
> scheme. That would eliminate the need for ResetRenderViewHostBinding and would
> make ids persistent across debugging sessions.

Done.

https://chromiumcodereview.appspot.com/10169043/diff/1/content/browser/debugg...
File content/browser/debugger/devtools_http_handler_impl.h (right):

https://chromiumcodereview.appspot.com/10169043/diff/1/content/browser/debugg...
content/browser/debugger/devtools_http_handler_impl.h:112:
scoped_ptr<DevToolsHttpHandlerDelegate::BindingHandler> binding_handler_;
On 2012/04/25 05:45:15, pfeldman wrote:
> You will often want existing class to implement this interface, I don't think
> you should pass the ownership here.

Done.

https://chromiumcodereview.appspot.com/10169043/diff/1/content/public/browser...
File content/public/browser/devtools_http_handler_delegate.h (right):

https://chromiumcodereview.appspot.com/10169043/diff/1/content/public/browser...
content/public/browser/devtools_http_handler_delegate.h:21: class BindingHandler
{
On 2012/04/25 05:45:15, pfeldman wrote:
> If this binding is really optional, I'd rather move this interface into
> DevToolsHttpHandler and call it RenderViewHostBinding. Clients will be able to
> set it via DevToolsHttpHandler::SetRenderViewHostBinding.

Done.

https://chromiumcodereview.appspot.com/10169043/diff/1/content/public/browser...
content/public/browser/devtools_http_handler_delegate.h:26: virtual std::string
GetRenderViewHostIdentifier(RenderViewHost* rvh) = 0;
On 2012/04/25 05:45:15, pfeldman wrote:
> If you rename the interface to RenderViewHostBinding. These will be called
> GetIdentifier and ForIdentifier.

Done.

[Jói, 2012-04-25 15:20:38]

LGTM for content/public, but please defer to Pavel for the overall LGTM.

Cheers,
Jói


On Wed, Apr 25, 2012 at 3:14 PM, <marshall@chromium.org> wrote:

> Please review.
>
> Adding joi@ as OWNER for content/public/browser/**devtools_http_handler.h
> review.
>
> Thanks.
>
>
>
> https://chromiumcodereview.**appspot.com/10169043/diff/1/**
>
content/browser/debugger/**devtools_http_handler_impl.cc<https://chromiumcodereview.appspot.com/10169043/diff/1/content/browser/debugger/devtools_http_handler_impl.cc>
> File content/browser/debugger/**devtools_http_handler_impl.cc (right):
>
> https://chromiumcodereview.**appspot.com/10169043/diff/1/**
>
content/browser/debugger/**devtools_http_handler_impl.cc#**newcode60<https://chromiumcodereview.appspot.com/10169043/diff/1/content/browser/debugger/devtools_http_handler_impl.cc#newcode60>
> content/browser/debugger/**devtools_http_handler_impl.cc:**60: return
> base::StringPrintf("%lu", index);
> On 2012/04/25 05:45:15, pfeldman wrote:
>
>> Nit: it might be a good time to migrate to the
>>
> "<process_id>_<routing_id>" id
>
>> scheme. That would eliminate the need for ResetRenderViewHostBinding
>>
> and would
>
>> make ids persistent across debugging sessions.
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10169043/diff/1/**
>
content/browser/debugger/**devtools_http_handler_impl.h<https://chromiumcodereview.appspot.com/10169043/diff/1/content/browser/debugger/devtools_http_handler_impl.h>
> File content/browser/debugger/**devtools_http_handler_impl.h (right):
>
> https://chromiumcodereview.**appspot.com/10169043/diff/1/**
>
content/browser/debugger/**devtools_http_handler_impl.h#**newcode112<https://chromiumcodereview.appspot.com/10169043/diff/1/content/browser/debugger/devtools_http_handler_impl.h#newcode112>
> content/browser/debugger/**devtools_http_handler_impl.h:**112:
> scoped_ptr<**DevToolsHttpHandlerDelegate::**BindingHandler>
> binding_handler_;
> On 2012/04/25 05:45:15, pfeldman wrote:
>
>> You will often want existing class to implement this interface, I
>>
> don't think
>
>> you should pass the ownership here.
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10169043/diff/1/**
>
content/public/browser/**devtools_http_handler_**delegate.h<https://chromiumcodereview.appspot.com/10169043/diff/1/content/public/browser/devtools_http_handler_delegate.h>
> File content/public/browser/**devtools_http_handler_**delegate.h (right):
>
> https://chromiumcodereview.**appspot.com/10169043/diff/1/**
>
content/public/browser/**devtools_http_handler_**delegate.h#newcode21<https://chromiumcodereview.appspot.com/10169043/diff/1/content/public/browser/devtools_http_handler_delegate.h#newcode21>
> content/public/browser/**devtools_http_handler_**delegate.h:21: class
> BindingHandler {
> On 2012/04/25 05:45:15, pfeldman wrote:
>
>> If this binding is really optional, I'd rather move this interface
>>
> into
>
>> DevToolsHttpHandler and call it RenderViewHostBinding. Clients will be
>>
> able to
>
>> set it via DevToolsHttpHandler::**SetRenderViewHostBinding.
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10169043/diff/1/**
>
content/public/browser/**devtools_http_handler_**delegate.h#newcode26<https://chromiumcodereview.appspot.com/10169043/diff/1/content/public/browser/devtools_http_handler_delegate.h#newcode26>
> content/public/browser/**devtools_http_handler_**delegate.h:26: virtual
> std::string GetRenderViewHostIdentifier(**RenderViewHost* rvh) = 0;
> On 2012/04/25 05:45:15, pfeldman wrote:
>
>> If you rename the interface to RenderViewHostBinding. These will be
>>
> called
>
>> GetIdentifier and ForIdentifier.
>>
>
> Done.
>
>
https://chromiumcodereview.**appspot.com/10169043/<https://chromiumcodereview...
>

[pfeldman, 2012-04-25 15:27:54]

lgtm.

Thanks for doing this. Note that as we allow debugging of non-render view hosts
remotely, this resolver is likely to be generalized to DevToolsAgentHost.

https://chromiumcodereview.appspot.com/10169043/diff/6001/content/browser/deb...
File content/browser/debugger/devtools_http_handler_impl.h (right):

https://chromiumcodereview.appspot.com/10169043/diff/6001/content/browser/deb...
content/browser/debugger/devtools_http_handler_impl.h:114:
DevToolsHttpHandler::RenderViewHostBinding* binding_;
Do you need DevToolsHttpHandler prefix here?

https://chromiumcodereview.appspot.com/10169043/diff/6001/content/browser/deb...
content/browser/debugger/devtools_http_handler_impl.h:115:
scoped_ptr<DevToolsHttpHandler::RenderViewHostBinding> default_binding_;
ditto

https://chromiumcodereview.appspot.com/10169043/diff/6001/content/public/brow...
File content/public/browser/devtools_http_handler.h (right):

https://chromiumcodereview.appspot.com/10169043/diff/6001/content/public/brow...
content/public/browser/devtools_http_handler.h:36: // Return the mapping of
identifier to RenderViewHost.
Returns

[Marshall, 2012-04-25 15:36:19]

The move to DevToolsAgentHost sounds good for the future.

https://chromiumcodereview.appspot.com/10169043/diff/6001/content/browser/deb...
File content/browser/debugger/devtools_http_handler_impl.h (right):

https://chromiumcodereview.appspot.com/10169043/diff/6001/content/browser/deb...
content/browser/debugger/devtools_http_handler_impl.h:114:
DevToolsHttpHandler::RenderViewHostBinding* binding_;
On 2012/04/25 15:27:54, pfeldman wrote:
> Do you need DevToolsHttpHandler prefix here?

Done.

https://chromiumcodereview.appspot.com/10169043/diff/6001/content/browser/deb...
content/browser/debugger/devtools_http_handler_impl.h:115:
scoped_ptr<DevToolsHttpHandler::RenderViewHostBinding> default_binding_;
On 2012/04/25 15:27:54, pfeldman wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10169043/diff/6001/content/public/brow...
File content/public/browser/devtools_http_handler.h (right):

https://chromiumcodereview.appspot.com/10169043/diff/6001/content/public/brow...
content/public/browser/devtools_http_handler.h:36: // Return the mapping of
identifier to RenderViewHost.
On 2012/04/25 15:27:54, pfeldman wrote:
> Returns

Done.

[commit-bot: I haz the power, 2012-04-25 16:43:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marshall@chromium.org/10169043/7006

[commit-bot: I haz the power, 2012-04-25 17:03:01]

Try job failure for 10169043-7006 (retry) on android for steps "Compile, build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[Marshall, 2012-04-25 17:07:56]

On 2012/04/25 17:03:01, I haz the power (commit-bot) wrote:
> Try job failure for 10169043-7006 (retry) on android for steps "Compile,
build".
> It's a second try, previously, steps "Compile, build" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

Fixed, retrying.

[commit-bot: I haz the power, 2012-04-25 17:08:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marshall@chromium.org/10169043/18002

[commit-bot: I haz the power, 2012-04-25 20:06:52]

Change committed as 133962