DevTools: Enable network emulation on Service Workers

DevTools: Enable network emulation on Service Workers

This patch moves the functionality of
RenderFrameDevToolsAgentHost::DispatchProtocolMessage() to
DevToolsAgentHostImpl so that SW agent host can handle the
emulateNetworkConditions message.

BUG=444820
Committed: https://crrev.com/2d99237c6d9167c0822cdf046a9fc2bd99cc8517
Cr-Commit-Position: refs/heads/master@{#321898}

[Kunihiko Sakamoto, 2015-03-23 07:50:35]

ksakamoto@chromium.org changed reviewers:
+ pfeldman@chromium.org

[Kunihiko Sakamoto, 2015-03-23 07:50:36]

PTAL

[pfeldman, 2015-03-23 09:05:25]

https://codereview.chromium.org/1029523002/diff/1/content/browser/devtools/se...
File content/browser/devtools/service_worker_devtools_agent_host.cc (right):

https://codereview.chromium.org/1029523002/diff/1/content/browser/devtools/se...
content/browser/devtools/service_worker_devtools_agent_host.cc:103: void
ServiceWorkerDevToolsAgentHost::DispatchProtocolMessage(
Lets have this implemented in the devtools_agent_host_impl so that all
implementations could benefit from this. Subclasses will register their
dispatchers in the protected protocol handler. It should return boolean to
specify whether it handled anything.

[pfeldman, 2015-03-23 09:59:01]

A couple of nits and it is good to go.

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.cc:232: return false;
See my comment below, this should be "true" since you consider malformed request
handled at this point.

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.cc:255: if (!IsAttached())
This check is already present in SendMessageToClient.

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.h (right):

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.h:63: void
DispatchOnInspectorFrontend(const std::string& message);
This is essentially the same as the SendMessageToClient above.

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
File content/browser/devtools/forwarding_agent_host.cc (right):

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
content/browser/devtools/forwarding_agent_host.cc:49: return false;
you should always report this command as handled.

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
File content/browser/devtools/ipc_devtools_agent_host.cc (right):

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
content/browser/devtools/ipc_devtools_agent_host.cc:21: if
(!DevToolsAgentHostImpl::DispatchProtocolMessage(message)) {
Replace nested condition with guard expression:

if (DevTools ...)
  return true;

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
File content/browser/devtools/render_frame_devtools_agent_host.cc (left):

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:185: return;
You are changing this semantics since the new code will be sending malformed
commands over IPC. It was returning here originally.

[Kunihiko Sakamoto, 2015-03-23 10:23:58]

Thank you for the review!

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.cc:232: return false;
On 2015/03/23 09:59:00, pfeldman wrote:
> See my comment below, this should be "true" since you consider malformed
request
> handled at this point.

Done.

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.h (right):

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.h:63: void
DispatchOnInspectorFrontend(const std::string& message);
On 2015/03/23 09:59:00, pfeldman wrote:
> This is essentially the same as the SendMessageToClient above.

Merged.

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
File content/browser/devtools/forwarding_agent_host.cc (right):

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
content/browser/devtools/forwarding_agent_host.cc:49: return false;
On 2015/03/23 09:59:01, pfeldman wrote:
> you should always report this command as handled.

Done.

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
File content/browser/devtools/ipc_devtools_agent_host.cc (right):

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
content/browser/devtools/ipc_devtools_agent_host.cc:21: if
(!DevToolsAgentHostImpl::DispatchProtocolMessage(message)) {
On 2015/03/23 09:59:01, pfeldman wrote:
> Replace nested condition with guard expression:
> 
> if (DevTools ...)
>   return true;

Done.

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
File content/browser/devtools/render_frame_devtools_agent_host.cc (left):

https://codereview.chromium.org/1029523002/diff/20001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:185: return;
On 2015/03/23 09:59:01, pfeldman wrote:
> You are changing this semantics since the new code will be sending malformed
> commands over IPC. It was returning here originally.

Ah right, thanks for pointing it out.

[pfeldman, 2015-03-23 10:31:37]

lgtm

https://codereview.chromium.org/1029523002/diff/40001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/1029523002/diff/40001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.cc:247: if
(protocol_handler_->HandleOptionalCommand(command.Pass()))
nit: return protocol_handler_->HandleOptionalCommand(command.Pass());

[Kunihiko Sakamoto, 2015-03-23 10:45:51]

ksakamoto@chromium.org changed reviewers:
+ nasko@chromium.org

[Kunihiko Sakamoto, 2015-03-23 10:45:51]

+nasko for content/public

https://codereview.chromium.org/1029523002/diff/40001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/1029523002/diff/40001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.cc:247: if
(protocol_handler_->HandleOptionalCommand(command.Pass()))
On 2015/03/23 10:31:37, pfeldman wrote:
> nit: return protocol_handler_->HandleOptionalCommand(command.Pass());

Done.

[pfeldman, 2015-03-23 12:12:06]

On 2015/03/23 10:45:51, Kunihiko Sakamoto wrote:
> +nasko for content/public

I think TBR-ing void->bool is fine.

[nasko, 2015-03-23 14:14:41]

content/public/browser/devtools_agent_host.h LGTM

[Kunihiko Sakamoto, 2015-03-23 23:24:18]

The CQ bit was checked by ksakamoto@chromium.org

[Kunihiko Sakamoto, 2015-03-23 23:24:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1029523002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-03-23 23:25:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1029523002/60001

[commit-bot: I haz the power, 2015-03-23 23:48:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-23 23:49:02]

Patchset 4 (id:??) landed as
https://crrev.com/2d99237c6d9167c0822cdf046a9fc2bd99cc8517
Cr-Commit-Position: refs/heads/master@{#321898}