Introduce intercepting and handling devtools messages in the renderer.

Introduce intercepting and handling devtools messages in the renderer.

Add interception for DOM.setFileInputFiles, that gives appropriate permissions.
BUG=167100
TBR=jam


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183735

[kkania, 2013-02-13 01:57:51]

Let me know if this is an ok approach. I couldn't do exactly what you said,
because the old DomainHandler had a friend connection with the
DevtoolsBrowserTarget and assumed it handled all messages for the domain, which
is a bit different from what we want with the intercept stuff.

[pfeldman, 2013-02-13 17:38:24]

Overall looks good. A couple of suggestions below.

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/d...
File content/browser/devtools/devtools_browser_target.cc (right):

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/d...
content/browser/devtools/devtools_browser_target.cc:33: return
command->NoSuchMethodErrorResponse();
What if you put this logic into  DevToolsBrowserTarget::HandleMessage instead?
You would need no DevToolsBrowserTarget::DomainHandler altogether?

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/d...
content/browser/devtools/devtools_browser_target.cc:44:
handlers_deleter_(&handlers_),
Thanks!

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/d...
File content/browser/devtools/devtools_http_handler_impl.cc (right):

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/d...
content/browser/devtools/devtools_http_handler_impl.cc:627: new
DevToolsTracingHandler(browser_target_->GetNotifier()));
It is unfortunate browser target can't set itself as a notifier - your
overriding implementation that doesn't need notifications becomes overly
complex. I'd suggest adding public SetNotifier on Handler intead.

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/d...
File content/browser/devtools/devtools_protocol.cc (right):

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/d...
content/browser/devtools/devtools_protocol.cc:124:
DevToolsProtocol::Handler::Handler(const Notifier& notifier)
Method definition order should match the declaration order (the one in .h).

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/r...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/r...
content/browser/devtools/render_view_devtools_agent_host.cc:74: class
RendererOverridesHandler : public DevToolsProtocol::Handler {
Since we expect it to grow, I would immediately define it in its own file.

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/r...
content/browser/devtools/render_view_devtools_agent_host.cc:82:
"DOM.setFileInputFiles",
You would then be able to declare constants for the method and attribute names.

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/r...
content/browser/devtools/render_view_devtools_agent_host.cc:186:
overrides_handler_.reset(new RendererOverridesHandler(
That's the one I was talking about: it does not really need notifier.

[kkania, 2013-02-13 19:17:19]

addressed all comments (except I didn't make a constant for the command name)

+jam for OWNERS review of content/content_browser.gypi

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/d...
File content/browser/devtools/devtools_browser_target.cc (right):

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/d...
content/browser/devtools/devtools_browser_target.cc:33: return
command->NoSuchMethodErrorResponse();
On 2013/02/13 17:38:24, pfeldman wrote:
> What if you put this logic into  DevToolsBrowserTarget::HandleMessage instead?
> You would need no DevToolsBrowserTarget::DomainHandler altogether?

Whoops, good idea.

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/d...
content/browser/devtools/devtools_browser_target.cc:44:
handlers_deleter_(&handlers_),
On 2013/02/13 17:38:24, pfeldman wrote:
> Thanks!

Done.

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/d...
File content/browser/devtools/devtools_http_handler_impl.cc (right):

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/d...
content/browser/devtools/devtools_http_handler_impl.cc:627: new
DevToolsTracingHandler(browser_target_->GetNotifier()));
On 2013/02/13 17:38:24, pfeldman wrote:
> It is unfortunate browser target can't set itself as a notifier - your
> overriding implementation that doesn't need notifications becomes overly
> complex. I'd suggest adding public SetNotifier on Handler intead.

Done.

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/d...
File content/browser/devtools/devtools_protocol.cc (right):

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/d...
content/browser/devtools/devtools_protocol.cc:124:
DevToolsProtocol::Handler::Handler(const Notifier& notifier)
On 2013/02/13 17:38:24, pfeldman wrote:
> Method definition order should match the declaration order (the one in .h).

Done.

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/r...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/r...
content/browser/devtools/render_view_devtools_agent_host.cc:74: class
RendererOverridesHandler : public DevToolsProtocol::Handler {
On 2013/02/13 17:38:24, pfeldman wrote:
> Since we expect it to grow, I would immediately define it in its own file.

Done.

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/r...
content/browser/devtools/render_view_devtools_agent_host.cc:82:
"DOM.setFileInputFiles",
On 2013/02/13 17:38:24, pfeldman wrote:
> You would then be able to declare constants for the method and attribute
names.

I don't see the benefit of declaring a constant if it is only used in one place,
unless it clarifies usage (e.g., kStackSize would be better than a plain 4096).
Made 'files' a constant since it is used in two places.

https://codereview.chromium.org/12218134/diff/4002/content/browser/devtools/r...
content/browser/devtools/render_view_devtools_agent_host.cc:186:
overrides_handler_.reset(new RendererOverridesHandler(
On 2013/02/13 17:38:24, pfeldman wrote:
> That's the one I was talking about: it does not really need notifier.

Done.

[pfeldman, 2013-02-13 19:23:19]

lgtm

I'd like to see constants extracted at least for overridden method names so that
we saw what we override clearly.

Btw, you don't need jam's stamp - John suggests that we land the gypi changes as
TBR=jam.

[kkania, 2013-02-13 19:37:44]

thanks for quick review; made it a constant; will commit once dependent cl is in

[commit-bot: I haz the power, 2013-02-14 16:41:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkania@chromium.org/12218134/14003

[commit-bot: I haz the power, 2013-02-14 19:11:37]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-21 01:34:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkania@chromium.org/12218134/28001

[commit-bot: I haz the power, 2013-02-21 03:55:11]

Change committed as 183735