html_viewer: Add DevToolsAgentImpl.

html_viewer: Add DevToolsAgentImpl.

DevToolsAgentImpl connects the DevTools mojo service with the DevTools agent support in blink.

BUG=478249
Committed: https://crrev.com/32166090d27c6450957f59eedaa6ea5fa12762e4
Cr-Commit-Position: refs/heads/master@{#333516}

Committed: https://crrev.com/9675e4abe4bd0da5005841081ca9547cdbd24c53
Cr-Commit-Position: refs/heads/master@{#333834}

[yzshen1, 2015-06-02 22:13:44]

yzshen@chromium.org changed reviewers:
+ sky@chromium.org

[yzshen1, 2015-06-02 22:13:46]

Hi, Scott.

Would you please take a look? Thanks!

[sky, 2015-06-02 23:28:32]

You'll need to copy update "copy_mandoline_assets" in
mandoline/app/android/BUILD.gn to list devtools_service as well.

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
File components/html_viewer/devtools_agent_impl.cc (right):

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
components/html_viewer/devtools_agent_impl.cc:61: // TODO(yzshen): (1)
Eventually the handling code for Page.navigate (and some
We have that class now. browser is the one that handles load requests.

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
components/html_viewer/devtools_agent_impl.cc:81: blink::WebURLRequest
request{GURL(url)};
Should you validate url?

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
File components/html_viewer/devtools_agent_impl.h (right):

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
components/html_viewer/devtools_agent_impl.h:40: void sendProtocolMessage(int
call_id,
Because blink is in another repo we keep overrides virtual with no override.

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/html...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/html...
components/html_viewer/html_document.cc:173: devtools_agent_.reset(new
DevToolsAgentImpl(main_frame, shell_.get()));
Why do we always create devtools agent? Shouldn't we only do it if needed?

[dgozman, 2015-06-03 09:55:55]

dgozman@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2015-06-03 09:55:55]

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
File components/html_viewer/devtools_agent_impl.cc (right):

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
components/html_viewer/devtools_agent_impl.cc:73:
make_scoped_ptr(static_cast<base::DictionaryValue*>(value.release()));
There is |Value::GetAsDictionary| for this purpose. With it, you can also omit
type check above.

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
File components/html_viewer/devtools_agent_impl.h (right):

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
components/html_viewer/devtools_agent_impl.h:24: public
blink::WebDevToolsAgentClient,
Could you please add me to future patches which touch WebDevToolsAgentClient?

[yzshen1, 2015-06-05 17:29:30]

Thank you Scott and Dmitry!

Please take another look.

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
File components/html_viewer/devtools_agent_impl.cc (right):

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
components/html_viewer/devtools_agent_impl.cc:61: // TODO(yzshen): (1)
Eventually the handling code for Page.navigate (and some
On 2015/06/02 23:28:32, sky wrote:
> We have that class now. browser is the one that handles load requests.

I think currently we don't have the ability to do this without adding new
interfaces to browser.
We use the content handler pattern to load requests. When requested to load a
new URL, we replace the existing html_viewer with a new instance.

That won't work for this scenario: here we need the request to be handled by the
same html_viewer, otherwise the state we setup for performance test will be
lost.

I will start investigation and discussion to handle this properly at the browser
side. (I think we probably need to let browser decide how to choose handlers for
a request, instead of relying on the shell to do it with MIME/URL mapping.)

I personally feel that there is no need to make that a blocking issue for this
CL. The temp code I put here is just ~20 lines and I don't expect to add more
temp code here.

What do you think? Thanks!

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
components/html_viewer/devtools_agent_impl.cc:73:
make_scoped_ptr(static_cast<base::DictionaryValue*>(value.release()));
On 2015/06/03 09:55:55, dgozman wrote:
> There is |Value::GetAsDictionary| for this purpose. With it, you can also omit
> type check above.

Done. That is much better.

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
components/html_viewer/devtools_agent_impl.cc:81: blink::WebURLRequest
request{GURL(url)};
On 2015/06/02 23:28:32, sky wrote:
> Should you validate url?

Done.

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
File components/html_viewer/devtools_agent_impl.h (right):

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
components/html_viewer/devtools_agent_impl.h:24: public
blink::WebDevToolsAgentClient,
On 2015/06/03 09:55:55, dgozman wrote:
> Could you please add me to future patches which touch WebDevToolsAgentClient?

Definitely!

I thought you might not be interested in this because it uses
WebDevToolsAgentClient but doesn't modify anything there. Now that I know you
are interested, I will for sure add you to future patches.

Thanks for all the help. :)

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
components/html_viewer/devtools_agent_impl.h:40: void sendProtocolMessage(int
call_id,
On 2015/06/02 23:28:32, sky wrote:
> Because blink is in another repo we keep overrides virtual with no override.

Done.

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/html...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/html...
components/html_viewer/html_document.cc:173: devtools_agent_.reset(new
DevToolsAgentImpl(main_frame, shell_.get()));
On 2015/06/02 23:28:32, sky wrote:
> Why do we always create devtools agent? Shouldn't we only do it if needed?

This |devtools_agent_| registers itself with the DevTools service. It does
nothing else unless the DevTools service sets a client and runs commands on it.
It is probably not very costly. We always creates a similar object in Chrome too
in render_frame_impl.

If we want to create it on-demand:
Currently there isn't a clean way for the DevTools service to ask the
html_viewer to create |devtools_agent_|, because it has no way to query the
existing html_viewer instances.
That could be changed when we add such capability to the browser app. 

What do you think? Thanks!

[sky, 2015-06-05 19:13:30]

LGTM

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
File components/html_viewer/devtools_agent_impl.cc (right):

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/devt...
components/html_viewer/devtools_agent_impl.cc:61: // TODO(yzshen): (1)
Eventually the handling code for Page.navigate (and some
On 2015/06/05 17:29:29, yzshen1 wrote:
> On 2015/06/02 23:28:32, sky wrote:
> > We have that class now. browser is the one that handles load requests.
> 
> I think currently we don't have the ability to do this without adding new
> interfaces to browser.
> We use the content handler pattern to load requests. When requested to load a
> new URL, we replace the existing html_viewer with a new instance.
> 
> That won't work for this scenario: here we need the request to be handled by
the
> same html_viewer, otherwise the state we setup for performance test will be
> lost.
> 
> I will start investigation and discussion to handle this properly at the
browser
> side. (I think we probably need to let browser decide how to choose handlers
for
> a request, instead of relying on the shell to do it with MIME/URL mapping.)
> 
> I personally feel that there is no need to make that a blocking issue for this
> CL. The temp code I put here is just ~20 lines and I don't expect to add more
> temp code here.
> 
> What do you think? Thanks!

Fair enough.

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/html...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1163953002/diff/1/components/html_viewer/html...
components/html_viewer/html_document.cc:173: devtools_agent_.reset(new
DevToolsAgentImpl(main_frame, shell_.get()));
On 2015/06/05 17:29:29, yzshen1 wrote:
> On 2015/06/02 23:28:32, sky wrote:
> > Why do we always create devtools agent? Shouldn't we only do it if needed?
> 
> This |devtools_agent_| registers itself with the DevTools service. It does
> nothing else unless the DevTools service sets a client and runs commands on
it.
> It is probably not very costly. We always creates a similar object in Chrome
too
> in render_frame_impl.
> 
> If we want to create it on-demand:
> Currently there isn't a clean way for the DevTools service to ask the
> html_viewer to create |devtools_agent_|, because it has no way to query the
> existing html_viewer instances.
> That could be changed when we add such capability to the browser app. 
> 
> What do you think? Thanks!

SG

[dgozman, 2015-06-09 05:24:00]

lgtm with comments

https://codereview.chromium.org/1163953002/diff/20001/components/html_viewer/...
File components/html_viewer/devtools_agent_impl.cc (right):

https://codereview.chromium.org/1163953002/diff/20001/components/html_viewer/...
components/html_viewer/devtools_agent_impl.cc:74: command->GetString("method",
&method);
if (!command->GetString(...))
  return;

https://codereview.chromium.org/1163953002/diff/20001/components/html_viewer/...
components/html_viewer/devtools_agent_impl.cc:82: return;
Json rpc protocol we use here does not advise to silently drop errors.
You'd better pass everything you can't handle to blink, and it will do the error
handling for free.

[yzshen1, 2015-06-09 15:47:40]

Thank you Dmitry!

https://codereview.chromium.org/1163953002/diff/20001/components/html_viewer/...
File components/html_viewer/devtools_agent_impl.cc (right):

https://codereview.chromium.org/1163953002/diff/20001/components/html_viewer/...
components/html_viewer/devtools_agent_impl.cc:74: command->GetString("method",
&method);
On 2015/06/09 05:23:59, dgozman wrote:
> if (!command->GetString(...))
>   return;

Done.

https://codereview.chromium.org/1163953002/diff/20001/components/html_viewer/...
components/html_viewer/devtools_agent_impl.cc:82: return;
On 2015/06/09 05:23:59, dgozman wrote:
> Json rpc protocol we use here does not advise to silently drop errors.
> You'd better pass everything you can't handle to blink, and it will do the
error
> handling for free.

Done.

[yzshen1, 2015-06-09 15:52:35]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2015-06-09 15:52:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1163953002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-06-09 15:52:58]

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

[commit-bot: I haz the power, 2015-06-09 15:58:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-09 15:58:04]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yzshen1, 2015-06-09 16:43:19]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2015-06-09 16:43:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1163953002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-06-09 16:44:12]

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

[commit-bot: I haz the power, 2015-06-09 17:35:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-09 17:35:59]

Patchset 5 (id:??) landed as
https://crrev.com/32166090d27c6450957f59eedaa6ea5fa12762e4
Cr-Commit-Position: refs/heads/master@{#333516}

[yzshen1, 2015-06-09 20:55:14]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1172123002/ by yzshen@chromium.org.

The reason for reverting is: Maybe the reason of the html_viewer_apptests
flakiness:

http://build.chromium.org/p/chromium.mojo/builders/Chromium%20Mojo%20Linux/bu...


Running mojo:html_viewer_apptests...
DEBUG:Tests for mojo:html_viewer_apptests:
AXProviderTest.
  HelloWorld

DEBUG:Command:
/mnt/data/b/build/slave/chromium_mojo/build/src/out/Release/mojo_runner
--gtest_filter=AXProviderTest.HelloWorld mojo:html_viewer_apptests

[  FAILED  ] Command (exit code -11):
/mnt/data/b/build/slave/chromium_mojo/build/src/out/Release/mojo_runner
--gtest_filter=AXProviderTest.HelloWorld mojo:html_viewer_apptests.

[yzshen1, 2015-06-10 22:02:59]

yzshen@chromium.org changed reviewers:
+ msw@chromium.org

[yzshen1, 2015-06-10 22:05:39]

Hi, Mike.

Would you please review the diff between patchset 6 and patchset 5?

Although it touches quite a few files, it is actually a small change: I moved
kRemoteDebuggingPort definition to devtools_service/pubic/cpp/switches.{h,cc},
so that it can be shared by the runner and html_viewer.

Thanks!

[msw, 2015-06-10 22:22:47]

lgtm, thanks!

[yzshen1, 2015-06-10 22:58:44]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2015-06-10 22:58:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1163953002/#ps100001
(title: "Don't create DevToolsAgentImpl instance unless --remote-debugging-port
is specified")

[commit-bot: I haz the power, 2015-06-10 23:00:06]

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

[commit-bot: I haz the power, 2015-06-10 23:11:10]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-10 23:12:10]

Patchset 6 (id:??) landed as
https://crrev.com/9675e4abe4bd0da5005841081ca9547cdbd24c53
Cr-Commit-Position: refs/heads/master@{#333834}