Chromecast: adds CastNetworkDelegate.

Chromecast: adds CastNetworkDelegate.

This provides an interface for special handling of "whitelisted" URLs
in internal code.

It also denies local file access for Chromecast, except on Android
(with a flag).

R=lcwu@chromium.org,byungchul@chromium.org
BUG=336640
Committed: https://crrev.com/dacd4f6f6d3e81a2522a19f2dc0660b9ada62b31
Cr-Commit-Position: refs/heads/master@{#297079}

[lcwu1, 2014-09-25 01:19:24]

Did you forget to upload the source files for CastNetworkDelegate?

https://codereview.chromium.org/598303002/diff/20001/chromecast/service/cast_...
File chromecast/service/cast_service.h (right):

https://codereview.chromium.org/598303002/diff/20001/chromecast/service/cast_...
chromecast/service/cast_service.h:26: CastNetworkDelegate* network_delegate);
Network delegate should be part of the url request context so you shouldn't have
to pass in explicitly.

[gunsch, 2014-09-25 15:47:56]

It appears that way :) Uploaded now.

https://codereview.chromium.org/598303002/diff/20001/chromecast/service/cast_...
File chromecast/service/cast_service.h (right):

https://codereview.chromium.org/598303002/diff/20001/chromecast/service/cast_...
chromecast/service/cast_service.h:26: CastNetworkDelegate* network_delegate);
On 2014/09/25 01:19:24, lcwu1 wrote:
> Network delegate should be part of the url request context so you shouldn't
have
> to pass in explicitly.

Ooh, good idea. Looking at our local code, we also pass in the url request
context unnecessarily.

[lcwu1, 2014-09-25 18:53:18]

https://codereview.chromium.org/598303002/diff/40001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/598303002/diff/40001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:110: 'service/cast_network_delegate.h',
I think it makes more sense for the network delegate files to be in either
chromecast/shell/browser (like what chrome or content shell dose) or maybe in
chromecast/net (as the base class in in net/base). And they should be in
cast_shell_common target.

https://codereview.chromium.org/598303002/diff/40001/chromecast/service/cast_...
File chromecast/service/cast_service.h (right):

https://codereview.chromium.org/598303002/diff/40001/chromecast/service/cast_...
chromecast/service/cast_service.h:20: class CastNetworkDelegate;
This forward decl is no longer needed.

https://codereview.chromium.org/598303002/diff/40001/chromecast/shell/browser...
File chromecast/shell/browser/cast_browser_context.cc (right):

https://codereview.chromium.org/598303002/diff/40001/chromecast/shell/browser...
chromecast/shell/browser/cast_browser_context.cc:8: #include
"base/files/file_util.h"
Now that you added file_util.h, you might as well add file_path.h.

[gunsch, 2014-09-25 19:24:59]

https://codereview.chromium.org/598303002/diff/40001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/598303002/diff/40001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:110: 'service/cast_network_delegate.h',
On 2014/09/25 18:53:18, lcwu1 wrote:
> I think it makes more sense for the network delegate files to be in either
> chromecast/shell/browser (like what chrome or content shell dose) or maybe in
> chromecast/net (as the base class in in net/base). And they should be in
> cast_shell_common target.

I disagree. See my internal CL. ChromecastService depends on
CastNetworkDelegate, which it's a layering violation for us. This was already a
layering violation before I merged it internally and moved it from the
cast_shell and cast_shell_internal targets to
cast_service/cast_service_internal.

https://codereview.chromium.org/598303002/diff/40001/chromecast/service/cast_...
File chromecast/service/cast_service.h (right):

https://codereview.chromium.org/598303002/diff/40001/chromecast/service/cast_...
chromecast/service/cast_service.h:20: class CastNetworkDelegate;
On 2014/09/25 18:53:18, lcwu1 wrote:
> This forward decl is no longer needed.

Done.

https://codereview.chromium.org/598303002/diff/40001/chromecast/shell/browser...
File chromecast/shell/browser/cast_browser_context.cc (right):

https://codereview.chromium.org/598303002/diff/40001/chromecast/shell/browser...
chromecast/shell/browser/cast_browser_context.cc:8: #include
"base/files/file_util.h"
On 2014/09/25 18:53:18, lcwu1 wrote:
> Now that you added file_util.h, you might as well add file_path.h.

Not referenced directly in this file.

[lcwu1, 2014-09-25 20:07:04]

https://codereview.chromium.org/598303002/diff/40001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/598303002/diff/40001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:110: 'service/cast_network_delegate.h',
On 2014/09/25 19:24:59, gunsch wrote:
> On 2014/09/25 18:53:18, lcwu1 wrote:
> > I think it makes more sense for the network delegate files to be in either
> > chromecast/shell/browser (like what chrome or content shell dose) or maybe
in
> > chromecast/net (as the base class in in net/base). And they should be in
> > cast_shell_common target.
> 
> I disagree. See my internal CL. ChromecastService depends on
> CastNetworkDelegate, which it's a layering violation for us. This was already
a
> layering violation before I merged it internally and moved it from the
> cast_shell and cast_shell_internal targets to
> cast_service/cast_service_internal.

ChromecastService should not depend on CastNetworkDelegate. The network delegate
should be used (called) inside content's network requests. If Cast service has
to depend on network delegate, then there is something wrong that we need to
fix.

Basically I don't see network delegate as part of the cast service. It should be
part of the content embedder.

https://codereview.chromium.org/598303002/diff/40001/chromecast/shell/browser...
File chromecast/shell/browser/cast_browser_context.cc (right):

https://codereview.chromium.org/598303002/diff/40001/chromecast/shell/browser...
chromecast/shell/browser/cast_browser_context.cc:8: #include
"base/files/file_util.h"
On 2014/09/25 19:24:59, gunsch wrote:
> On 2014/09/25 18:53:18, lcwu1 wrote:
> > Now that you added file_util.h, you might as well add file_path.h.
> 
> Not referenced directly in this file.

Line 78 and 107 has a direct reference to base::FilePath.

[gunsch, 2014-09-25 20:46:34]

https://codereview.chromium.org/598303002/diff/40001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/598303002/diff/40001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:110: 'service/cast_network_delegate.h',
On 2014/09/25 20:07:04, lcwu1 wrote:
> On 2014/09/25 19:24:59, gunsch wrote:
> > On 2014/09/25 18:53:18, lcwu1 wrote:
> > > I think it makes more sense for the network delegate files to be in either
> > > chromecast/shell/browser (like what chrome or content shell dose) or maybe
> in
> > > chromecast/net (as the base class in in net/base). And they should be in
> > > cast_shell_common target.
> > 
> > I disagree. See my internal CL. ChromecastService depends on
> > CastNetworkDelegate, which it's a layering violation for us. This was
already
> a
> > layering violation before I merged it internally and moved it from the
> > cast_shell and cast_shell_internal targets to
> > cast_service/cast_service_internal.
> 
> ChromecastService should not depend on CastNetworkDelegate. The network
delegate
> should be used (called) inside content's network requests. If Cast service has
> to depend on network delegate, then there is something wrong that we need to
> fix.
> 
> Basically I don't see network delegate as part of the cast service. It should
be
> part of the content embedder.
> 

Done.

https://codereview.chromium.org/598303002/diff/40001/chromecast/shell/browser...
File chromecast/shell/browser/cast_browser_context.cc (right):

https://codereview.chromium.org/598303002/diff/40001/chromecast/shell/browser...
chromecast/shell/browser/cast_browser_context.cc:8: #include
"base/files/file_util.h"
Discussed offline, part of BrowserContext's public API so not necessary to
re-include here.

[lcwu1, 2014-09-26 22:38:59]

https://codereview.chromium.org/598303002/diff/100001/chromecast/shell/browse...
File chromecast/shell/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/598303002/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/cast_browser_main_parts.cc:106:
url_request_context_factory_->GetSystemGetter()));
Now that I looked at the internal CLs, I think I am fine with your network
delegate class structure. My original comment of not passing in a network
delegate object in CastService's Create function is based on the assumption that
CastNetworkDelegate is not referenced in Chromecast code directly. (That is, it
is only used by higher-level content or net code.) But now that we make it also
do DeviceIdSettings stuff, it makes sense to pass in the network delegate. (And
in fact, we have to pass it in anyway because we don't expose url request
context factory in cast service.)

[gunsch, 2014-09-26 23:03:06]

https://codereview.chromium.org/598303002/diff/100001/chromecast/shell/browse...
File chromecast/shell/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/598303002/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/cast_browser_main_parts.cc:106:
url_request_context_factory_->GetSystemGetter()));
On 2014/09/26 22:38:59, lcwu1 wrote:
> Now that I looked at the internal CLs, I think I am fine with your network
> delegate class structure. My original comment of not passing in a network
> delegate object in CastService's Create function is based on the assumption
that
> CastNetworkDelegate is not referenced in Chromecast code directly. (That is,
it
> is only used by higher-level content or net code.) But now that we make it
also
> do DeviceIdSettings stuff, it makes sense to pass in the network delegate.
(And
> in fact, we have to pass it in anyway because we don't expose url request
> context factory in cast service.)

Done.

[lcwu1, 2014-09-26 23:07:31]

LGTM % a gyp fix

https://codereview.chromium.org/598303002/diff/120001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/598303002/diff/120001/chromecast/chromecast.g...
chromecast/chromecast.gyp:105: 'cast_common',
Is this dependency still needed?

[gunsch, 2014-09-26 23:10:08]

https://codereview.chromium.org/598303002/diff/120001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/598303002/diff/120001/chromecast/chromecast.g...
chromecast/chromecast.gyp:105: 'cast_common',
On 2014/09/26 23:07:31, lcwu1 wrote:
> Is this dependency still needed?

Yes; cast_network_delegate links in chromecast/common/chromecast_switches.

[gunsch, 2014-09-26 23:10:13]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-09-26 23:11:43]

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

[gunsch, 2014-09-26 23:27:25]

The CQ bit was unchecked by gunsch@chromium.org

[gunsch, 2014-09-26 23:28:20]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-09-26 23:28:54]

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

[commit-bot: I haz the power, 2014-09-27 00:01:17]

Committed patchset #8 (id:140001) as 8ad29168721d6d3c91717ad6440eee297b2c5678

[commit-bot: I haz the power, 2014-09-27 00:01:53]

Patchset 8 (id:??) landed as
https://crrev.com/dacd4f6f6d3e81a2522a19f2dc0660b9ada62b31
Cr-Commit-Position: refs/heads/master@{#297079}