Chromecast devtools delegate fixes.

Chromecast devtools delegate fixes.

This patch makes chromecast always bundle the frontend,
and exposes all content targets via remote debugging.

BUG=none

Committed: https://crrev.com/05a779da69456df3c948a916b9c4d781879a3608
Cr-Commit-Position: refs/heads/master@{#292865}

[dgozman, 2014-08-28 10:06:53]

dgozman@chromium.org changed reviewers:
+ gunsch@chromium.org, lcwu@chromium.org

[dgozman, 2014-08-28 10:06:53]

Hi,

Could you please take a look? I think, this is what chromecast needs for remote
debugging: exposing all the targets provided by content and delegating all the
work to content.

Unfortunately, I was not able to build/test this. When building with
chromecast=1 on linux, I get compile errors like this one:

../../device/hid/hid_service_win.h:42:41: error: unknown type name
'PHIDP_PREPARSED_DATA'
  static void CollectInfoFromButtonCaps(PHIDP_PREPARSED_DATA preparsed_data,

Thanks,
Dmitry

https://codereview.chromium.org/517493003/diff/1/chromecast/shell/browser/dev...
File chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc (left):

https://codereview.chromium.org/517493003/diff/1/chromecast/shell/browser/dev...
chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc:113: return
std::string();
I don't see that you are excluding discovery page from the build, thus removing
this android special-casing.

https://codereview.chromium.org/517493003/diff/1/chromecast/shell/browser/dev...
chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc:124: return false;
ditto

[lcwu1, 2014-08-28 16:14:46]

On 2014/08/28 10:06:53, dgozman wrote:
> Hi,
> 
> Could you please take a look? I think, this is what chromecast needs for
remote
> debugging: exposing all the targets provided by content and delegating all the
> work to content.
> 
> Unfortunately, I was not able to build/test this. When building with
> chromecast=1 on linux, I get compile errors like this one:
> 
> ../../device/hid/hid_service_win.h:42:41: error: unknown type name
> 'PHIDP_PREPARSED_DATA'
>   static void CollectInfoFromButtonCaps(PHIDP_PREPARSED_DATA preparsed_data,

Dmitry,

How did you build cast_shell? Here is what we do to build it (in debug mode):

$ build/gyp_chromium -Dchromecast=1
$ ninja -C out/Debug cast_shell

I just tried it with the latest Chromium tree and it built fine.

> 
> Thanks,
> Dmitry
> 
>
https://codereview.chromium.org/517493003/diff/1/chromecast/shell/browser/dev...
> File chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc (left):
> 
>
https://codereview.chromium.org/517493003/diff/1/chromecast/shell/browser/dev...
> chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc:113: return
> std::string();
> I don't see that you are excluding discovery page from the build, thus
removing
> this android special-casing.
> 
>
https://codereview.chromium.org/517493003/diff/1/chromecast/shell/browser/dev...
> chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc:124: return
false;
> ditto

[gunsch, 2014-08-28 16:28:21]

I patched your CL in and it built fine. After running gyp_chromium here,
grepping for "hid_service_win" in out/Debug/obj returns nothing here, so I
wonder if a clean build might make a difference in your case, or if you have a
local gyp config files potentially affecting the build?

https://codereview.chromium.org/517493003/diff/1/chromecast/shell/browser/dev...
File chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc (left):

https://codereview.chromium.org/517493003/diff/1/chromecast/shell/browser/dev...
chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc:124: return false;
On 2014/08/28 10:06:53, dgozman wrote:
> ditto

Can you explain the interaction between
GetDiscoveryPageHTML/BundlesFrontendResources and replacing the frontend URL?
(see chromecast/shell/browser/devtools/remote_debugging_server.cc). If that's
turned off, the host Chrome instance will refuse to connect, but it seems to me
they're intended as a group.

Otherwise, turning just these two off seems to work for me.

[dgozman, 2014-08-29 16:19:24]

Clearing out/ directory helped with building cast_shell. Thank you. Still was
not able to run it: many "Not implemented reached" and
ERROR:chromecast_config.cc(94)] Cannot initialize chromecast config.

I will change chromecast/shell/browser/devtools/remote_debugging_server.cc in
this patch based on your decision regarding bundling frontend (see inline
comment).

https://codereview.chromium.org/517493003/diff/1/chromecast/shell/browser/dev...
File chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc (left):

https://codereview.chromium.org/517493003/diff/1/chromecast/shell/browser/dev...
chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc:124: return false;
On 2014/08/28 16:28:21, gunsch wrote:
> On 2014/08/28 10:06:53, dgozman wrote:
> > ditto
> 
> Can you explain the interaction between
> GetDiscoveryPageHTML/BundlesFrontendResources and replacing the frontend URL?
> (see chromecast/shell/browser/devtools/remote_debugging_server.cc). If that's
> turned off, the host Chrome instance will refuse to connect, but it seems to
me
> they're intended as a group.
> 
> Otherwise, turning just these two off seems to work for me.

If you pass a specific frontend url in constructor, it is used for remote
debugging this browser/shell/whatever. BundlesFrontendResources method is
ignored in this case. This is what we do on android to save some space. Do you
think chromecast should do the same?

If you don't pass frontend url, you should bundle frontend to resources. This is
what we do on most platforms.

This interaction doesn't sound like the best design. We should probably fix
that.

[gunsch, 2014-08-29 16:39:44]

On 2014/08/29 16:19:24, dgozman wrote:
> Clearing out/ directory helped with building cast_shell. Thank you. Still was
> not able to run it: many "Not implemented reached" and
> ERROR:chromecast_config.cc(94)] Cannot initialize chromecast config.
> 
> I will change chromecast/shell/browser/devtools/remote_debugging_server.cc in
> this patch based on your decision regarding bundling frontend (see inline
> comment).
> 
>
https://codereview.chromium.org/517493003/diff/1/chromecast/shell/browser/dev...
> File chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc (left):
> 
>
https://codereview.chromium.org/517493003/diff/1/chromecast/shell/browser/dev...
> chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc:124: return
false;
> On 2014/08/28 16:28:21, gunsch wrote:
> > On 2014/08/28 10:06:53, dgozman wrote:
> > > ditto
> > 
> > Can you explain the interaction between
> > GetDiscoveryPageHTML/BundlesFrontendResources and replacing the frontend
URL?
> > (see chromecast/shell/browser/devtools/remote_debugging_server.cc). If
that's
> > turned off, the host Chrome instance will refuse to connect, but it seems to
> me
> > they're intended as a group.
> > 
> > Otherwise, turning just these two off seems to work for me.
> 
> If you pass a specific frontend url in constructor, it is used for remote
> debugging this browser/shell/whatever. BundlesFrontendResources method is
> ignored in this case. This is what we do on android to save some space. Do you
> think chromecast should do the same?
> 
> If you don't pass frontend url, you should bundle frontend to resources. This
is
> what we do on most platforms.
> 
> This interaction doesn't sound like the best design. We should probably fix
> that.

Ah yeah, running is a little tricky due to using Ozone for rendering (not fully
stable yet IIRC). If you add "--ozone-platform=egltest --ignore-gpu-blacklist"
running should be fine. The logs you mentioned are red herrings/things we
haven't fully ported from our local branch.

The decision for Chromecast is twofold: there's both an "Android Cast Receiver"
product (intended for use on Android TV) and a Linux Chromecast build (for the
dongle product "Chromecast").

For Chromecast (dongle), we'd like users to still navigate directly to
[IP]:9222. I assume this means we need to continue bundling resources.

For Android Cast Receiver, we'd like to take the Android-style approach of using
Unix domain sockets. I assume this means we need to use the remote frontend URL,
at which point bundling resources is useless.

If my assumptions are correct, then it seems to me that the current code is
fine, though we could refrain from bundling the dev-tools resources in our
Android Cast build. Does that sound valid to you?

[gunsch, 2014-08-29 17:13:40]

On 2014/08/29 16:39:44, gunsch wrote:
> On 2014/08/29 16:19:24, dgozman wrote:
> > Clearing out/ directory helped with building cast_shell. Thank you. Still
was
> > not able to run it: many "Not implemented reached" and
> > ERROR:chromecast_config.cc(94)] Cannot initialize chromecast config.
> > 
> > I will change chromecast/shell/browser/devtools/remote_debugging_server.cc
in
> > this patch based on your decision regarding bundling frontend (see inline
> > comment).
> > 
> >
>
https://codereview.chromium.org/517493003/diff/1/chromecast/shell/browser/dev...
> > File chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc (left):
> > 
> >
>
https://codereview.chromium.org/517493003/diff/1/chromecast/shell/browser/dev...
> > chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc:124: return
> false;
> > On 2014/08/28 16:28:21, gunsch wrote:
> > > On 2014/08/28 10:06:53, dgozman wrote:
> > > > ditto
> > > 
> > > Can you explain the interaction between
> > > GetDiscoveryPageHTML/BundlesFrontendResources and replacing the frontend
> URL?
> > > (see chromecast/shell/browser/devtools/remote_debugging_server.cc). If
> that's
> > > turned off, the host Chrome instance will refuse to connect, but it seems
to
> > me
> > > they're intended as a group.
> > > 
> > > Otherwise, turning just these two off seems to work for me.
> > 
> > If you pass a specific frontend url in constructor, it is used for remote
> > debugging this browser/shell/whatever. BundlesFrontendResources method is
> > ignored in this case. This is what we do on android to save some space. Do
you
> > think chromecast should do the same?
> > 
> > If you don't pass frontend url, you should bundle frontend to resources.
This
> is
> > what we do on most platforms.
> > 
> > This interaction doesn't sound like the best design. We should probably fix
> > that.
> 
> Ah yeah, running is a little tricky due to using Ozone for rendering (not
fully
> stable yet IIRC). If you add "--ozone-platform=egltest --ignore-gpu-blacklist"
> running should be fine. The logs you mentioned are red herrings/things we
> haven't fully ported from our local branch.
> 
> The decision for Chromecast is twofold: there's both an "Android Cast
Receiver"
> product (intended for use on Android TV) and a Linux Chromecast build (for the
> dongle product "Chromecast").
> 
> For Chromecast (dongle), we'd like users to still navigate directly to
> [IP]:9222. I assume this means we need to continue bundling resources.
> 
> For Android Cast Receiver, we'd like to take the Android-style approach of
using
> Unix domain sockets. I assume this means we need to use the remote frontend
URL,
> at which point bundling resources is useless.
> 
> If my assumptions are correct, then it seems to me that the current code is
> fine, though we could refrain from bundling the dev-tools resources in our
> Android Cast build. Does that sound valid to you?

Short update: you're correct, looks like we're crashing on startup right now due
to https://codereview.chromium.org/522853002/

[dgozman, 2014-08-29 18:11:57]

> Ah yeah, running is a little tricky due to using Ozone for rendering (not
fully
> stable yet IIRC). If you add "--ozone-platform=egltest --ignore-gpu-blacklist"
> running should be fine. The logs you mentioned are red herrings/things we
> haven't fully ported from our local branch.
Thanks, I'll try it.

> 
> The decision for Chromecast is twofold: there's both an "Android Cast
Receiver"
> product (intended for use on Android TV) and a Linux Chromecast build (for the
> dongle product "Chromecast").
> 
> For Chromecast (dongle), we'd like users to still navigate directly to
> [IP]:9222. I assume this means we need to continue bundling resources.
> 
> For Android Cast Receiver, we'd like to take the Android-style approach of
using
> Unix domain sockets. I assume this means we need to use the remote frontend
URL,
> at which point bundling resources is useless.
DevTools remote debugging is always done via sockets. You can remote debug
anything (including Chrome browser), if it's exposing remote debugging protocol.
In chrome this means running with --remote-debugging-port. In Chrome for
Android, it is based on whether android device is in developer mode.

All this is orthogonal to bundling frontend into sources. Our design implies
that debugging should be done using frontend of the same version as the browser
(for compatibility). To achieve this, we either bundle frontend into the product
or have a copy somewhere in the cloud. If you don't care much about resources
size, I'd suggest bundling - it's easier to maintain and generally faster.

> 
> If my assumptions are correct, then it seems to me that the current code is
> fine, though we could refrain from bundling the dev-tools resources in our
> Android Cast build. Does that sound valid to you?
That's right. If you go for devtools frontend in the cloud for your Android
Cast, you should strip it from the bundle - that's the only purpose.

[pfeldman, 2014-08-29 19:01:15]

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

[pfeldman, 2014-08-29 19:01:16]

We should always default to bundling the front-end. It makes things simpler,
makes user experience more pleasant. So shell should bundle.

In fact, I think that even production Chromecast bundles it. It is extra 1.5Mb
of deployment footprint.

[gunsch, 2014-08-29 20:00:28]

Thanks for the extra explanations. Makes sense, we're not super concerned about
bundle size on our Android build so bundling in both of our builds sounds good.

LGTM

[lcwu1, 2014-08-30 02:00:15]

lgtm

[dgozman, 2014-09-01 11:20:46]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-09-01 11:21:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dgozman@chromium.org/517493003/20001

[commit-bot: I haz the power, 2014-09-01 12:19:20]

Committed patchset #2 (id:20001) as 108faa4fe4878ae52ed69c18225233a3e136135c

[commit-bot: I haz the power, 2014-09-10 03:16:38]

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