Introduce networking.cast API

Introduce networking.cast API

The new API will be private to cast extension, and will contain
networkingPrivate methods only used/needed by cast extension and setup
app.
Method implementations are not changed, they are just copied from
networkingPrivate API to networking.cast API. The goal is to
eventually remove networkingPrivate implementation - when extensions
white-listed for the API move to the networking.cast API.

List of methods that are being moved to networking.cast API:
verifyDestination
verifyDestinationAndEncryptCredentials
verifyDestinationAndEncryptData
setWifiTDLSEnabledState
getWifiTDLSStatus

For now, the ChromeNetworkingCast just calls into the networking
private API's crypto verification delegate - the next step will be to
move this logic into ChromeNetworkingCastDelegate.

BUG=678737
Review-Url: https://codereview.chromium.org/2726223004
Cr-Commit-Position: refs/heads/master@{#457489}
Committed: https://chromium.googlesource.com/chromium/src/+/d2ff1ae2c1175126133ec36bc400b3aaed08092b

[tbarzic, 2017-03-03 22:58:01]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 22:58:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-04 00:13:45]

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

[commit-bot: I haz the power, 2017-03-04 00:13:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tbarzic, 2017-03-04 01:44:30]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-04 01:44:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-04 02:30:06]

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

[commit-bot: I haz the power, 2017-03-04 02:30:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tbarzic, 2017-03-04 21:26:10]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-04 21:26:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-04 21:31:48]

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

[commit-bot: I haz the power, 2017-03-04 21:31:49]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[tbarzic, 2017-03-05 00:27:32]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-05 00:27:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-05 00:42:42]

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

[commit-bot: I haz the power, 2017-03-05 00:42:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tbarzic, 2017-03-05 05:46:00]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-05 05:46:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-05 06:08:34]

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

[commit-bot: I haz the power, 2017-03-05 06:08:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tbarzic, 2017-03-06 04:28:45]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 04:28:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 05:29:27]

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

[commit-bot: I haz the power, 2017-03-06 05:29:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbarzic, 2017-03-06 06:26:49]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 06:27:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2017-03-06 06:48:14]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 06:48:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 07:53:34]

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

[commit-bot: I haz the power, 2017-03-06 07:53:35]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tbarzic, 2017-03-06 17:39:22]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 17:39:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2017-03-06 18:13:06]

Description was changed from

==========
DRAFT: Introduce networking.cast API

The new API will be private to cast extension, and will contain
networkingPrivate methods only used/needed by cast extension and setup
app.

BUG=
==========

to

==========
Introduce networking.cast API

The new API will be private to cast extension, and will contain
networkingPrivate methods only used/needed by cast extension and setup
app.
Method implementations are not changed, they are just copied from
networkingPrivate API to networking.cast API. The goal is to
eventually remove networkingPrivate implementation - when extensions
white-listed for the API move to the networking.cast API.

List of methods that are being moved to networking.cast API:
verifyDestination
verifyDestinationAndEncryptCredentials
verifyDestinationAndEncryptData
setWifiTDLSEnabledState
getWifiTDLSStatus

BUG=678737
==========

[tbarzic, 2017-03-06 18:15:15]

Description was changed from

==========
Introduce networking.cast API

The new API will be private to cast extension, and will contain
networkingPrivate methods only used/needed by cast extension and setup
app.
Method implementations are not changed, they are just copied from
networkingPrivate API to networking.cast API. The goal is to
eventually remove networkingPrivate implementation - when extensions
white-listed for the API move to the networking.cast API.

List of methods that are being moved to networking.cast API:
verifyDestination
verifyDestinationAndEncryptCredentials
verifyDestinationAndEncryptData
setWifiTDLSEnabledState
getWifiTDLSStatus

BUG=678737
==========

to

==========
Introduce networking.cast API

The new API will be private to cast extension, and will contain
networkingPrivate methods only used/needed by cast extension and setup
app.
Method implementations are not changed, they are just copied from
networkingPrivate API to networking.cast API. The goal is to
eventually remove networkingPrivate implementation - when extensions
white-listed for the API move to the networking.cast API.

List of methods that are being moved to networking.cast API:
verifyDestination
verifyDestinationAndEncryptCredentials
verifyDestinationAndEncryptData
setWifiTDLSEnabledState
getWifiTDLSStatus

The CL removed VeryfyDelegate from NetworkingPrivateAPI delegate
and moves the logic to a new NetworkingCastDelegate that is
provided by ExtensionsAPIClient.

BUG=678737
==========

[tbarzic, 2017-03-06 18:20:54]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 18:21:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 19:25:40]

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

[commit-bot: I haz the power, 2017-03-06 19:25:41]

Dry run: This issue passed the CQ dry run.

[tbarzic, 2017-03-06 19:33:45]

tbarzic@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, stevenjb@chromium.org

[tbarzic, 2017-03-06 19:33:46]

When you get time, please take a look.

[stevenjb, 2017-03-07 23:28:04]

Are you sure this won't break existing cast extensions?

Even if it won't, could we split this into at least two CLs, one adding the new
API, and one removing the calls from networkingPrivate? We should be able to use
the same verify delegate for both APIs I think.

[tbarzic, 2017-03-08 01:30:15]

Description was changed from

==========
Introduce networking.cast API

The new API will be private to cast extension, and will contain
networkingPrivate methods only used/needed by cast extension and setup
app.
Method implementations are not changed, they are just copied from
networkingPrivate API to networking.cast API. The goal is to
eventually remove networkingPrivate implementation - when extensions
white-listed for the API move to the networking.cast API.

List of methods that are being moved to networking.cast API:
verifyDestination
verifyDestinationAndEncryptCredentials
verifyDestinationAndEncryptData
setWifiTDLSEnabledState
getWifiTDLSStatus

The CL removed VeryfyDelegate from NetworkingPrivateAPI delegate
and moves the logic to a new NetworkingCastDelegate that is
provided by ExtensionsAPIClient.

BUG=678737
==========

to

==========
Introduce networking.cast API

The new API will be private to cast extension, and will contain
networkingPrivate methods only used/needed by cast extension and setup
app.
Method implementations are not changed, they are just copied from
networkingPrivate API to networking.cast API. The goal is to
eventually remove networkingPrivate implementation - when extensions
white-listed for the API move to the networking.cast API.

List of methods that are being moved to networking.cast API:
verifyDestination
verifyDestinationAndEncryptCredentials
verifyDestinationAndEncryptData
setWifiTDLSEnabledState
getWifiTDLSStatus

For now, the ChromeNetworkingCast just calls into the networking
private API's crypto verification delegate - the next move will be to
move this logic into ChromeNetworkingCastDelegate.

BUG=678737
==========

[tbarzic, 2017-03-08 01:33:16]

I reduced the scope of this cl to only add the new API, networking private
changes will be uploaded separately

[stevenjb, 2017-03-08 17:44:35]

This was much simpler to review, thank you for separating it out.

This LGTM, but I have one entirely optional suggestion:

The transition I think be more clear if we extract VerifyDelegate to its own
file, e.g. networking_verify_delegate. It would probably need a new location,
e.g. extensions/browser/networking (or extensions/browser/api/networking? I
would defer to Devlin on that). We would also want to move crypto_verify_impl to
someplace like c/b/extensions/networking, and move the factories also. The
extraction should be done as a separate CL first, if we decide to do it.

The tricky part would be what to do about VerificationProperties. We already
convert that struct to CryptoVerifyImpl::Credentials::Credentials in
crypto_verify_impl.cc. I would suggest passing those as a DictionaryValue
instead of a struct and modify Credentials() to take a DictionaryValue. That way
it won't matter which API that they come from, and we won't have the current
awkwardness where ChromeNetworkingCastDelegate is converting a dictionary to
networking_private:::VerificationProperties.

[tbarzic, 2017-03-08 18:34:50]

On 2017/03/08 17:44:35, stevenjb wrote:
> This was much simpler to review, thank you for separating it out.
> 
> This LGTM, but I have one entirely optional suggestion:
> 
> The transition I think be more clear if we extract VerifyDelegate to its own
> file, e.g. networking_verify_delegate. It would probably need a new location,
> e.g. extensions/browser/networking (or extensions/browser/api/networking? I
> would defer to Devlin on that). We would also want to move crypto_verify_impl
to
> someplace like c/b/extensions/networking, and move the factories also. The
> extraction should be done as a separate CL first, if we decide to do it.
> 

I planned to move VerifyDelegate implementation to networking_cast_delegate,
and have networking_private implementation just use that one (since the plan
is to eventually remove the networking private implementation).

> The tricky part would be what to do about VerificationProperties. We already
> convert that struct to CryptoVerifyImpl::Credentials::Credentials in
> crypto_verify_impl.cc. I would suggest passing those as a DictionaryValue
> instead of a struct and modify Credentials() to take a DictionaryValue. That
way
> it won't matter which API that they come from, and we won't have the current
> awkwardness where ChromeNetworkingCastDelegate is converting a dictionary to
> networking_private:::VerificationProperties.

Another option would be to have CryptoVerifyImpl methods take Credentails struct
(though, Credentials would have to be defined as part of the interface rather
than
an implementation detail)

[Devlin, 2017-03-11 03:07:53]

Sorry for the delay on this; I'll see if I can't get to this over the weekend so
you have it first thing Monday.

[Devlin, 2017-03-13 05:21:50]

Do we have a hard timeline for when items should move over?  If so, can we add a
TODO to remove the networkingPrivate versions?  If not, can we get one?

https://codereview.chromium.org/2726223004/diff/240001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_cast/chrome_networking_cast_delegate.cc
(right):

https://codereview.chromium.org/2726223004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_cast/chrome_networking_cast_delegate.cc:27:
return private_api::VerificationProperties::FromValue(*cast_properties_dict);
If these are exactly the same, could we use a reference to
networking.cast.VerificationProperties in networkingPrivate to avoid this
method?

https://codereview.chromium.org/2726223004/diff/240001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_cast/chrome_networking_cast_delegate.h
(right):

https://codereview.chromium.org/2726223004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_cast/chrome_networking_cast_delegate.h:48:
//     NetworkingPrivateDelegate:VerifyDelegate.
nit: NetworkingPrivateDelegate::VerifyDelegate (two ::)

https://codereview.chromium.org/2726223004/diff/240001/chrome/common/extensio...
File chrome/common/extensions/permissions/permission_set_unittest.cc (right):

https://codereview.chromium.org/2726223004/diff/240001/chrome/common/extensio...
chrome/common/extensions/permissions/permission_set_unittest.cc:841:
skip.insert(APIPermission::kNetworkingCast);
Is this going to remain private?  If not, this is the wrong spot for it (and I
worry could lead to us forgetting to move it later).

https://codereview.chromium.org/2726223004/diff/240001/extensions/browser/api...
File extensions/browser/api/networking_cast/networking_cast_api.cc (right):

https://codereview.chromium.org/2726223004/diff/240001/extensions/browser/api...
extensions/browser/api/networking_cast/networking_cast_api.cc:31: return
RespondNow(Error("Not supported."));
Why is the API compiled on unsupported platforms?

https://codereview.chromium.org/2726223004/diff/240001/extensions/browser/api...
extensions/browser/api/networking_cast/networking_cast_api.cc:72: return
did_respond() ? AlreadyResponded() : RespondLater();
Can these respond synchronously?  If so, we should comment that explicitly.  If
not, we shouldn't be using this pattern.

https://codereview.chromium.org/2726223004/diff/240001/extensions/browser/api...
File extensions/browser/api/networking_cast/networking_cast_api.h (right):

https://codereview.chromium.org/2726223004/diff/240001/extensions/browser/api...
extensions/browser/api/networking_cast/networking_cast_api.h:25:
NETWORKINGPRIVATE_VERIFYDESTINATION);
I don't think we should reuse the same histograms here.

https://codereview.chromium.org/2726223004/diff/240001/extensions/common/api/...
File extensions/common/api/_permission_features.json (right):

https://codereview.chromium.org/2726223004/diff/240001/extensions/common/api/...
extensions/common/api/_permission_features.json:298: "platforms": ["chromeos",
"mac", "win", "linux"],
What's left?

https://codereview.chromium.org/2726223004/diff/240001/extensions/common/api/...
File extensions/common/api/networking_cast.idl (right):

https://codereview.chromium.org/2726223004/diff/240001/extensions/common/api/...
extensions/common/api/networking_cast.idl:7: [nodoc] namespace networking.cast {
Does this belong at the //extensions level instead of the //chrome level?

https://codereview.chromium.org/2726223004/diff/240001/extensions/common/api/...
extensions/common/api/networking_cast.idl:10: Connected,
If items have to migrate over anyway, make these SCREAMING_STYLE.

https://codereview.chromium.org/2726223004/diff/240001/extensions/common/api/...
extensions/common/api/networking_cast.idl:95: // |enabled| If true, enable TDLS,
otherwise disable TDLS.
nit: |enabled|: - missing ':'

[stevenjb, 2017-03-13 17:12:26]

https://codereview.chromium.org/2726223004/diff/240001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_cast/chrome_networking_cast_delegate.cc
(right):

https://codereview.chromium.org/2726223004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_cast/chrome_networking_cast_delegate.cc:27:
return private_api::VerificationProperties::FromValue(*cast_properties_dict);
On 2017/03/13 05:21:49, Devlin wrote:
> If these are exactly the same, could we use a reference to
> networking.cast.VerificationProperties in networkingPrivate to avoid this
> method?

Originally this was a much larger CL that did this, and I suggested separating
out the networkingPrivate changes to a follow up CL. I'm a little (probably
overly) nervous about changes to networkingPrivate breaking Chrome OS packaged
apps and I felt that a more incremental change would be better.

That said, since this only affects Cast, It should be fine to go ahead and move
VerificationProperties to networkingCast in this CL. I does seem like it would
be less than ideal to move it later. Apologies for the churn.

[tbarzic, 2017-03-13 19:15:12]

Description was changed from

==========
Introduce networking.cast API

The new API will be private to cast extension, and will contain
networkingPrivate methods only used/needed by cast extension and setup
app.
Method implementations are not changed, they are just copied from
networkingPrivate API to networking.cast API. The goal is to
eventually remove networkingPrivate implementation - when extensions
white-listed for the API move to the networking.cast API.

List of methods that are being moved to networking.cast API:
verifyDestination
verifyDestinationAndEncryptCredentials
verifyDestinationAndEncryptData
setWifiTDLSEnabledState
getWifiTDLSStatus

For now, the ChromeNetworkingCast just calls into the networking
private API's crypto verification delegate - the next move will be to
move this logic into ChromeNetworkingCastDelegate.

BUG=678737
==========

to

==========
Introduce networking.cast API

The new API will be private to cast extension, and will contain
networkingPrivate methods only used/needed by cast extension and setup
app.
Method implementations are not changed, they are just copied from
networkingPrivate API to networking.cast API. The goal is to
eventually remove networkingPrivate implementation - when extensions
white-listed for the API move to the networking.cast API.

List of methods that are being moved to networking.cast API:
verifyDestination
verifyDestinationAndEncryptCredentials
verifyDestinationAndEncryptData
setWifiTDLSEnabledState
getWifiTDLSStatus

For now, the ChromeNetworkingCast just calls into the networking
private API's crypto verification delegate - the next step will be to
move this logic into ChromeNetworkingCastDelegate.

BUG=678737
==========

[tbarzic, 2017-03-13 22:13:34]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 22:14:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 22:25:41]

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

[commit-bot: I haz the power, 2017-03-13 22:25:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tbarzic, 2017-03-13 22:34:46]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[tbarzic, 2017-03-13 22:35:01]

https://codereview.chromium.org/2726223004/diff/240001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_cast/chrome_networking_cast_delegate.cc
(right):

https://codereview.chromium.org/2726223004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_cast/chrome_networking_cast_delegate.cc:27:
return private_api::VerificationProperties::FromValue(*cast_properties_dict);
On 2017/03/13 17:12:26, stevenjb wrote:
> On 2017/03/13 05:21:49, Devlin wrote:
> > If these are exactly the same, could we use a reference to
> > networking.cast.VerificationProperties in networkingPrivate to avoid this
> > method?
> 
> Originally this was a much larger CL that did this, and I suggested separating
> out the networkingPrivate changes to a follow up CL. I'm a little (probably
> overly) nervous about changes to networkingPrivate breaking Chrome OS packaged
> apps and I felt that a more incremental change would be better.
> 
> That said, since this only affects Cast, It should be fine to go ahead and
move
> VerificationProperties to networkingCast in this CL. I does seem like it would
> be less than ideal to move it later. Apologies for the churn.

Actually, I'm not sure that would work well, and extensions that have
networkingPrivate API might not have permission to use networking.cast API (and
thus types in networking.cast). This can be worked around (e.g. enabling
networking.cast when networkingPrivate permission is present), but I think it
would be cleaner from the API perspective to have different types.

I'll try to get rid of this step when I move VerifyDelegate logic to
NetworkingCAstDelegate, e.g. by making the delegate interface independent of the
networking API

https://codereview.chromium.org/2726223004/diff/240001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_cast/chrome_networking_cast_delegate.h
(right):

https://codereview.chromium.org/2726223004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_cast/chrome_networking_cast_delegate.h:48:
//     NetworkingPrivateDelegate:VerifyDelegate.
On 2017/03/13 05:21:49, Devlin wrote:
> nit: NetworkingPrivateDelegate::VerifyDelegate (two ::)

Done.

https://codereview.chromium.org/2726223004/diff/240001/chrome/common/extensio...
File chrome/common/extensions/permissions/permission_set_unittest.cc (right):

https://codereview.chromium.org/2726223004/diff/240001/chrome/common/extensio...
chrome/common/extensions/permissions/permission_set_unittest.cc:841:
skip.insert(APIPermission::kNetworkingCast);
On 2017/03/13 05:21:49, Devlin wrote:
> Is this going to remain private?  If not, this is the wrong spot for it (and I
> worry could lead to us forgetting to move it later).

yes, this is supposed to remain private.

https://codereview.chromium.org/2726223004/diff/240001/extensions/browser/api...
File extensions/browser/api/networking_cast/networking_cast_api.cc (right):

https://codereview.chromium.org/2726223004/diff/240001/extensions/browser/api...
extensions/browser/api/networking_cast/networking_cast_api.cc:31: return
RespondNow(Error("Not supported."));
On 2017/03/13 05:21:49, Devlin wrote:
> Why is the API compiled on unsupported platforms?

Done.

https://codereview.chromium.org/2726223004/diff/240001/extensions/browser/api...
extensions/browser/api/networking_cast/networking_cast_api.cc:72: return
did_respond() ? AlreadyResponded() : RespondLater();
On 2017/03/13 05:21:49, Devlin wrote:
> Can these respond synchronously?  If so, we should comment that explicitly. 
If
> not, we shouldn't be using this pattern.

Yes, it can respond synchronously, in tests.

https://codereview.chromium.org/2726223004/diff/240001/extensions/browser/api...
File extensions/browser/api/networking_cast/networking_cast_api.h (right):

https://codereview.chromium.org/2726223004/diff/240001/extensions/browser/api...
extensions/browser/api/networking_cast/networking_cast_api.h:25:
NETWORKINGPRIVATE_VERIFYDESTINATION);
On 2017/03/13 05:21:49, Devlin wrote:
> I don't think we should reuse the same histograms here.

Done.

https://codereview.chromium.org/2726223004/diff/240001/extensions/common/api/...
File extensions/common/api/_permission_features.json (right):

https://codereview.chromium.org/2726223004/diff/240001/extensions/common/api/...
extensions/common/api/_permission_features.json:298: "platforms": ["chromeos",
"mac", "win", "linux"],
On 2017/03/13 05:21:49, Devlin wrote:
> What's left?

Nothing.

Though, I removed linux now.

https://codereview.chromium.org/2726223004/diff/240001/extensions/common/api/...
File extensions/common/api/networking_cast.idl (right):

https://codereview.chromium.org/2726223004/diff/240001/extensions/common/api/...
extensions/common/api/networking_cast.idl:7: [nodoc] namespace networking.cast {
On 2017/03/13 05:21:49, Devlin wrote:
> Does this belong at the //extensions level instead of the //chrome level?

Hm, yeah, I guess we could move it to //chrome instead.

https://codereview.chromium.org/2726223004/diff/240001/extensions/common/api/...
extensions/common/api/networking_cast.idl:10: Connected,
On 2017/03/13 05:21:49, Devlin wrote:
> If items have to migrate over anyway, make these SCREAMING_STYLE.

Done.

https://codereview.chromium.org/2726223004/diff/240001/extensions/common/api/...
extensions/common/api/networking_cast.idl:95: // |enabled| If true, enable TDLS,
otherwise disable TDLS.
On 2017/03/13 05:21:49, Devlin wrote:
> nit: |enabled|: - missing ':'

Done.

[commit-bot: I haz the power, 2017-03-13 22:36:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 22:48:05]

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

[commit-bot: I haz the power, 2017-03-13 22:48:06]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbarzic, 2017-03-13 22:52:28]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 22:53:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 00:09:46]

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

[commit-bot: I haz the power, 2017-03-14 00:09:47]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-03-14 19:24:44]

https://codereview.chromium.org/2726223004/diff/320001/extensions/browser/api...
File extensions/browser/api/networking_private/networking_cast_delegate.h
(right):

https://codereview.chromium.org/2726223004/diff/320001/extensions/browser/api...
extensions/browser/api/networking_private/networking_cast_delegate.h:33: const
api::networking_private::VerificationProperties& properties,
Shouldn't we switch this to use api::networking_cast::VerificationProperties
now?

[tbarzic, 2017-03-14 19:56:55]

https://codereview.chromium.org/2726223004/diff/320001/extensions/browser/api...
File extensions/browser/api/networking_private/networking_cast_delegate.h
(right):

https://codereview.chromium.org/2726223004/diff/320001/extensions/browser/api...
extensions/browser/api/networking_private/networking_cast_delegate.h:33: const
api::networking_private::VerificationProperties& properties,
On 2017/03/14 19:24:43, stevenjb wrote:
> Shouldn't we switch this to use api::networking_cast::VerificationProperties
> now?

I plan to add a Credentials struct to the interface and switch properties param
to that.

api::networking_cast ::VerificationProperties doesn't work with networking cast
API being under //chrome

[stevenjb, 2017-03-14 20:16:48]

On 2017/03/14 19:56:55, tbarzic wrote:
>
https://codereview.chromium.org/2726223004/diff/320001/extensions/browser/api...
> File extensions/browser/api/networking_private/networking_cast_delegate.h
> (right):
> 
>
https://codereview.chromium.org/2726223004/diff/320001/extensions/browser/api...
> extensions/browser/api/networking_private/networking_cast_delegate.h:33: const
> api::networking_private::VerificationProperties& properties,
> On 2017/03/14 19:24:43, stevenjb wrote:
> > Shouldn't we switch this to use api::networking_cast::VerificationProperties
> > now?
> 
> I plan to add a Credentials struct to the interface and switch properties
param
> to that.
> 
> api::networking_cast ::VerificationProperties doesn't work with networking
cast
> API being under //chrome

Ugh, right, crypto_verify_ still points to
NetworkingPrivateDelegate::VerifyDelegate
and takes api::networking_private::VerificationProperties.

I'm still OK with this as-is if Devlin is, but in the short term it is pretty
confusing. I still think it might be worth making VerifyDelegate a separate
class/file and have it take a DictionaryValue that can be passed by either API
(since the dictionaries will be the same).

[tbarzic, 2017-03-14 20:28:21]

On 2017/03/14 20:16:48, stevenjb wrote:
> On 2017/03/14 19:56:55, tbarzic wrote:
> >
>
https://codereview.chromium.org/2726223004/diff/320001/extensions/browser/api...
> > File extensions/browser/api/networking_private/networking_cast_delegate.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/2726223004/diff/320001/extensions/browser/api...
> > extensions/browser/api/networking_private/networking_cast_delegate.h:33:
const
> > api::networking_private::VerificationProperties& properties,
> > On 2017/03/14 19:24:43, stevenjb wrote:
> > > Shouldn't we switch this to use
api::networking_cast::VerificationProperties
> > > now?
> > 
> > I plan to add a Credentials struct to the interface and switch properties
> param
> > to that.
> > 
> > api::networking_cast ::VerificationProperties doesn't work with networking
> cast
> > API being under //chrome
> 
> Ugh, right, crypto_verify_ still points to
> NetworkingPrivateDelegate::VerifyDelegate
> and takes api::networking_private::VerificationProperties.
> 
> I'm still OK with this as-is if Devlin is, but in the short term it is pretty
> confusing. I still think it might be worth making VerifyDelegate a separate
> class/file and have it take a DictionaryValue that can be passed by either API
> (since the dictionaries will be the same).

When I say I plan to, I mean I plan to send out a CL that makes this less
confusing this week, so short tearm is very short term :)
I just thought to separate this into smaller cls
In short, I plan to remove NetworkingPrivateDelegate::CryptoDelegate, and move
CryptoVerifyImpl logic to ChromeNetworkingCastDelegate,
which will implement networking_private/networking_cast_delegate.h interface
(though, I might rename that).
Further more, I'll move CryptoVerifyImpl::Credentials to NetworkingCastDelegate,
and use that as params for Verify
* methods (so the interface doesn't directly depend on a API type).

[stevenjb, 2017-03-14 21:37:06]

sgtm and still lgtm :)


On Tue, Mar 14, 2017 at 1:28 PM, <tbarzic@chromium.org> wrote:

> On 2017/03/14 20:16:48, stevenjb wrote:
> > On 2017/03/14 19:56:55, tbarzic wrote:
> > >
> >
> https://codereview.chromium.org/2726223004/diff/320001/
> extensions/browser/api/networking_private/networking_cast_delegate.h
> > > File extensions/browser/api/networking_private/networking_
> cast_delegate.h
> > > (right):
> > >
> > >
> >
> https://codereview.chromium.org/2726223004/diff/320001/
> extensions/browser/api/networking_private/networking_
> cast_delegate.h#newcode33
> > > extensions/browser/api/networking_private/networking_
> cast_delegate.h:33:
> const
> > > api::networking_private::VerificationProperties& properties,
> > > On 2017/03/14 19:24:43, stevenjb wrote:
> > > > Shouldn't we switch this to use
> api::networking_cast::VerificationProperties
> > > > now?
> > >
> > > I plan to add a Credentials struct to the interface and switch
> properties
> > param
> > > to that.
> > >
> > > api::networking_cast ::VerificationProperties doesn't work with
> networking
> > cast
> > > API being under //chrome
> >
> > Ugh, right, crypto_verify_ still points to
> > NetworkingPrivateDelegate::VerifyDelegate
> > and takes api::networking_private::VerificationProperties.
> >
> > I'm still OK with this as-is if Devlin is, but in the short term it is
> pretty
> > confusing. I still think it might be worth making VerifyDelegate a
> separate
> > class/file and have it take a DictionaryValue that can be passed by
> either API
> > (since the dictionaries will be the same).
>
> When I say I plan to, I mean I plan to send out a CL that makes this less
> confusing this week, so short tearm is very short term :)
> I just thought to separate this into smaller cls
> In short, I plan to remove NetworkingPrivateDelegate::CryptoDelegate, and
> move
> CryptoVerifyImpl logic to ChromeNetworkingCastDelegate,
> which will implement networking_private/networking_cast_delegate.h
> interface
> (though, I might rename that).
> Further more, I'll move CryptoVerifyImpl::Credentials to
> NetworkingCastDelegate,
> and use that as params for Verify
> * methods (so the interface doesn't directly depend on a API type).
>
> https://codereview.chromium.org/2726223004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Devlin, 2017-03-14 22:19:55]

>
https://codereview.chromium.org/2726223004/diff/240001/chrome/common/extensio...
> chrome/common/extensions/permissions/permission_set_unittest.cc:841:
> skip.insert(APIPermission::kNetworkingCast);
> On 2017/03/13 05:21:49, Devlin wrote:
> > Is this going to remain private?  If not, this is the wrong spot for it (and
I
> > worry could lead to us forgetting to move it later).
> 
> yes, this is supposed to remain private.

Sorry in advance for bikeshedding. :)

Is networking.cast the right name/place for this?  Or would something like
cast.networkingPrivate be more appropriate?  And even if we stick with
networking.*, should we keep the Private suffix, so that this is clear?

[stevenjb, 2017-03-14 22:28:47]

We made the decision a while back to drop 'Private' from APIs used in kiosk
apps.

e.g. 'networkingPrivate' is now actually 'networking.onc' (aliased to the
deprecated networkingPrivate name / implementation).

So unless we want to revisit that discussion, networking.cast would be
consistent with that pattern.


On Tue, Mar 14, 2017 at 3:19 PM, <rdevlin.cronin@chromium.org> wrote:

> >
> https://codereview.chromium.org/2726223004/diff/240001/
> chrome/common/extensions/permissions/permission_set_unittest.cc#newcode841
> > chrome/common/extensions/permissions/permission_set_unittest.cc:841:
> > skip.insert(APIPermission::kNetworkingCast);
> > On 2017/03/13 05:21:49, Devlin wrote:
> > > Is this going to remain private? If not, this is the wrong spot for it
> (and
> I
> > > worry could lead to us forgetting to move it later).
> >
> > yes, this is supposed to remain private.
>
> Sorry in advance for bikeshedding. :)
>
> Is networking.cast the right name/place for this? Or would something like
> cast.networkingPrivate be more appropriate? And even if we stick with
> networking.*, should we keep the Private suffix, so that this is clear?
>
> https://codereview.chromium.org/2726223004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Devlin, 2017-03-14 22:31:37]

On 2017/03/14 22:28:47, stevenjb wrote:
> We made the decision a while back to drop 'Private' from APIs used in kiosk
> apps.
> 
> e.g. 'networkingPrivate' is now actually 'networking.onc' (aliased to the
> deprecated networkingPrivate name / implementation).
> 
> So unless we want to revisit that discussion, networking.cast would be
> consistent with that pattern.
> 
> 
> On Tue, Mar 14, 2017 at 3:19 PM, <mailto:rdevlin.cronin@chromium.org> wrote:
> 
> > >
> > https://codereview.chromium.org/2726223004/diff/240001/
> > chrome/common/extensions/permissions/permission_set_unittest.cc#newcode841
> > > chrome/common/extensions/permissions/permission_set_unittest.cc:841:
> > > skip.insert(APIPermission::kNetworkingCast);
> > > On 2017/03/13 05:21:49, Devlin wrote:
> > > > Is this going to remain private? If not, this is the wrong spot for it
> > (and
> > I
> > > > worry could lead to us forgetting to move it later).
> > >
> > > yes, this is supposed to remain private.
> >
> > Sorry in advance for bikeshedding. :)
> >
> > Is networking.cast the right name/place for this? Or would something like
> > cast.networkingPrivate be more appropriate? And even if we stick with
> > networking.*, should we keep the Private suffix, so that this is clear?
> >
> > https://codereview.chromium.org/2726223004/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

It makes sense iff this API is intended to be made public for use in Kiosk apps
(this was part of what my question about private vs not was).  If it is, then
Private doesn't make sense, but honestly, I'm not entirely sure cast does either
- looking at the API methods, they don't seem very cast-related (beyond the fact
that right now they are only used by cast).

[Devlin, 2017-03-14 22:32:42]

On 2017/03/14 22:31:37, Devlin wrote:
> On 2017/03/14 22:28:47, stevenjb wrote:
> > We made the decision a while back to drop 'Private' from APIs used in kiosk
> > apps.
> > 
> > e.g. 'networkingPrivate' is now actually 'networking.onc' (aliased to the
> > deprecated networkingPrivate name / implementation).
> > 
> > So unless we want to revisit that discussion, networking.cast would be
> > consistent with that pattern.
> > 
> > 
> > On Tue, Mar 14, 2017 at 3:19 PM, <mailto:rdevlin.cronin@chromium.org> wrote:
> > 
> > > >
> > > https://codereview.chromium.org/2726223004/diff/240001/
> > > chrome/common/extensions/permissions/permission_set_unittest.cc#newcode841
> > > > chrome/common/extensions/permissions/permission_set_unittest.cc:841:
> > > > skip.insert(APIPermission::kNetworkingCast);
> > > > On 2017/03/13 05:21:49, Devlin wrote:
> > > > > Is this going to remain private? If not, this is the wrong spot for it
> > > (and
> > > I
> > > > > worry could lead to us forgetting to move it later).
> > > >
> > > > yes, this is supposed to remain private.
> > >
> > > Sorry in advance for bikeshedding. :)
> > >
> > > Is networking.cast the right name/place for this? Or would something like
> > > cast.networkingPrivate be more appropriate? And even if we stick with
> > > networking.*, should we keep the Private suffix, so that this is clear?
> > >
> > > https://codereview.chromium.org/2726223004/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> It makes sense iff this API is intended to be made public for use in Kiosk
apps
> (this was part of what my question about private vs not was).  If it is, then
> Private doesn't make sense, but honestly, I'm not entirely sure cast does
either
> - looking at the API methods, they don't seem very cast-related (beyond the
fact
> that right now they are only used by cast).

Also, if this is going to be made public, we should give more thought to how
much/what we want to expose, since as cast moves more towards a component of
chrome (and can use things like mojo), I was hoping to be able to trim down some
of these massive APIs. :)

[tbarzic, 2017-03-14 22:43:39]

On 2017/03/14 22:32:42, Devlin wrote:
> On 2017/03/14 22:31:37, Devlin wrote:
> > On 2017/03/14 22:28:47, stevenjb wrote:
> > > We made the decision a while back to drop 'Private' from APIs used in
kiosk
> > > apps.
> > > 
> > > e.g. 'networkingPrivate' is now actually 'networking.onc' (aliased to the
> > > deprecated networkingPrivate name / implementation).
> > > 
> > > So unless we want to revisit that discussion, networking.cast would be
> > > consistent with that pattern.
> > > 
> > > 
> > > On Tue, Mar 14, 2017 at 3:19 PM, <mailto:rdevlin.cronin@chromium.org>
wrote:
> > > 
> > > > >
> > > > https://codereview.chromium.org/2726223004/diff/240001/
> > > >
chrome/common/extensions/permissions/permission_set_unittest.cc#newcode841
> > > > > chrome/common/extensions/permissions/permission_set_unittest.cc:841:
> > > > > skip.insert(APIPermission::kNetworkingCast);
> > > > > On 2017/03/13 05:21:49, Devlin wrote:
> > > > > > Is this going to remain private? If not, this is the wrong spot for
it
> > > > (and
> > > > I
> > > > > > worry could lead to us forgetting to move it later).
> > > > >
> > > > > yes, this is supposed to remain private.
> > > >
> > > > Sorry in advance for bikeshedding. :)
> > > >
> > > > Is networking.cast the right name/place for this? Or would something
like
> > > > cast.networkingPrivate be more appropriate? And even if we stick with
> > > > networking.*, should we keep the Private suffix, so that this is clear?
> > > >
> > > > https://codereview.chromium.org/2726223004/
> > > >
> > > 
> > > -- 
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> > It makes sense iff this API is intended to be made public for use in Kiosk
> apps
> > (this was part of what my question about private vs not was).  If it is,
then
> > Private doesn't make sense, but honestly, I'm not entirely sure cast does
> either
> > - looking at the API methods, they don't seem very cast-related (beyond the
> fact
> > that right now they are only used by cast).
> 
> Also, if this is going to be made public, we should give more thought to how
> much/what we want to expose, since as cast moves more towards a component of
> chrome (and can use things like mojo), I was hoping to be able to trim down
some
> of these massive APIs. :)

No, this is not intended to become public, and won't be available in kiosk, so
I'd be fine appending Private to the name.
I think cast.networkingPrivate might be OK name.

(note that verify* methods are actually cast related (though, not to casting) -
they verify Chromecast certificates, and are used to encrypt potentially
sensitive data that should be passed to the Chromecast device during setup
flow).

[stevenjb, 2017-03-14 23:03:14]

Ah, I assumed we needed these for kiosk mode, otherwise I might have
suggested we just leave these in networkingPrivate until we completely
deprecate the cast extensions using them....

And yeah, these methods were implemented kind of generically, I think with
the idea that they might be used elsewhere, but in practice they are not
and likely never will be.

That said, cast.networkingPrivate might be a bit confusing (given we
already have networkingPrivate).

All of the following seem fine to me:

castPrivate.networking
networking.castPrivate
cast.private

If you both much prefer chrome.cast.networkingPrivate I can live with that,
but will feel bad for someone new trying to understand the history and
difference between that and chrome.networkingPrivate :)



On Tue, Mar 14, 2017 at 3:43 PM, <tbarzic@chromium.org> wrote:

> On 2017/03/14 22:32:42, Devlin wrote:
> > On 2017/03/14 22:31:37, Devlin wrote:
> > > On 2017/03/14 22:28:47, stevenjb wrote:
> > > > We made the decision a while back to drop 'Private' from APIs used in
> kiosk
> > > > apps.
> > > >
> > > > e.g. 'networkingPrivate' is now actually 'networking.onc' (aliased
> to the
> > > > deprecated networkingPrivate name / implementation).
> > > >
> > > > So unless we want to revisit that discussion, networking.cast would
> be
> > > > consistent with that pattern.
> > > >
> > > >
> > > > On Tue, Mar 14, 2017 at 3:19 PM, <mailto:rdevlin.cronin@chromium.org
> >
> wrote:
> > > >
> > > > > >
> > > > > https://codereview.chromium.org/2726223004/diff/240001/
> > > > >
> chrome/common/extensions/permissions/permission_set_unittest.cc#newcode841
> > > > > > chrome/common/extensions/permissions/permission_set_
> unittest.cc:841:
> > > > > > skip.insert(APIPermission::kNetworkingCast);
> > > > > > On 2017/03/13 05:21:49, Devlin wrote:
> > > > > > > Is this going to remain private? If not, this is the wrong
> spot for
> it
> > > > > (and
> > > > > I
> > > > > > > worry could lead to us forgetting to move it later).
> > > > > >
> > > > > > yes, this is supposed to remain private.
> > > > >
> > > > > Sorry in advance for bikeshedding. :)
> > > > >
> > > > > Is networking.cast the right name/place for this? Or would
> something
> like
> > > > > cast.networkingPrivate be more appropriate? And even if we stick
> with
> > > > > networking.*, should we keep the Private suffix, so that this is
> clear?
> > > > >
> > > > > https://codereview.chromium.org/2726223004/
> > > > >
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google
> Groups
> > > > "Chromium-reviews" group.
> > > > To unsubscribe from this group and stop receiving emails from it,
> send an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > > It makes sense iff this API is intended to be made public for use in
> Kiosk
> > apps
> > > (this was part of what my question about private vs not was). If it is,
> then
> > > Private doesn't make sense, but honestly, I'm not entirely sure cast
> does
> > either
> > > - looking at the API methods, they don't seem very cast-related
> (beyond the
> > fact
> > > that right now they are only used by cast).
> >
> > Also, if this is going to be made public, we should give more thought to
> how
> > much/what we want to expose, since as cast moves more towards a
> component of
> > chrome (and can use things like mojo), I was hoping to be able to trim
> down
> some
> > of these massive APIs. :)
>
> No, this is not intended to become public, and won't be available in
> kiosk, so
> I'd be fine appending Private to the name.
> I think cast.networkingPrivate might be OK name.
>
> (note that verify* methods are actually cast related (though, not to
> casting) -
> they verify Chromecast certificates, and are used to encrypt potentially
> sensitive data that should be passed to the Chromecast device during setup
> flow).
>
> https://codereview.chromium.org/2726223004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Devlin, 2017-03-14 23:05:54]

On 2017/03/14 23:03:14, stevenjb wrote:
> Ah, I assumed we needed these for kiosk mode, otherwise I might have
> suggested we just leave these in networkingPrivate until we completely
> deprecate the cast extensions using them....
> 
> And yeah, these methods were implemented kind of generically, I think with
> the idea that they might be used elsewhere, but in practice they are not
> and likely never will be.
> 
> That said, cast.networkingPrivate might be a bit confusing (given we
> already have networkingPrivate).
> 
> All of the following seem fine to me:
> 
> castPrivate.networking
> networking.castPrivate
> cast.private
> 
> If you both much prefer chrome.cast.networkingPrivate I can live with that,
> but will feel bad for someone new trying to understand the history and
> difference between that and chrome.networkingPrivate :)

I'm fine with any of these (including cast.networkingPrivate), or even something
like castPrivate (if we wanted to avoid nesting).  As long as we have the clear
Private suffix, I'm happy.

[tbarzic, 2017-03-15 01:05:40]

OK, renamed the API to networking.castPrivate

[Devlin, 2017-03-15 02:14:45]

Now that this is moved into //chrome, can we simplify the delegate?  At a
minimum, we should be able to avoid going through the indirection of the browser
client.  I think we can also take out some of the methods (those that aren't
overridden in the test).

Also, we should move the delegate base class (if it's still needed) to the
//chrome layer, as well.

[tbarzic, 2017-03-15 02:43:50]

On 2017/03/15 02:14:45, Devlin wrote:
> Now that this is moved into //chrome, can we simplify the delegate?  At a
> minimum, we should be able to avoid going through the indirection of the
browser
> client.  I think we can also take out some of the methods (those that aren't
> overridden in the test).
> 
> Also, we should move the delegate base class (if it's still needed) to the
> //chrome layer, as well.

Yeah, I could remove the delegate base class from this cl, but I'll just add it
again in the next CL where I replace NetworkingPrivateDelegate::VerifyDelegate
with NetworkingCastPrivateDelegate (as networking private API will still need to
use this logic) - that's the main reason I kept the delegate,even though
networking.cast API was moved to chrome.

[Devlin, 2017-03-16 01:26:21]

You need to update histograms.xml (and get a metrics reviewer), but otherwise
lgtm.  Thanks for your patience while bikeshedding. :)

https://codereview.chromium.org/2726223004/diff/340001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_cast_private/chrome_networking_cast_private_delegate.cc
(right):

https://codereview.chromium.org/2726223004/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_cast_private/chrome_networking_cast_private_delegate.cc:25:
new ChromeNetworkingCastPrivateDelegate(
nit: prefer base::MakeUnique

[tbarzic, 2017-03-16 04:02:37]

tbarzic@chromium.org changed reviewers:
+ isherman@chromium.org

[tbarzic, 2017-03-16 04:02:38]

+isherman for histograms

https://codereview.chromium.org/2726223004/diff/340001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_cast_private/chrome_networking_cast_private_delegate.cc
(right):

https://codereview.chromium.org/2726223004/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_cast_private/chrome_networking_cast_private_delegate.cc:25:
new ChromeNetworkingCastPrivateDelegate(
On 2017/03/16 01:26:21, Devlin wrote:
> nit: prefer base::MakeUnique

I tried initially, but MakeUnique does not work well with private ctors.

[Ilya Sherman, 2017-03-16 05:21:37]

histograms.xml lgtm

[Devlin, 2017-03-16 14:47:06]

https://codereview.chromium.org/2726223004/diff/340001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_cast_private/chrome_networking_cast_private_delegate.cc
(right):

https://codereview.chromium.org/2726223004/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_cast_private/chrome_networking_cast_private_delegate.cc:25:
new ChromeNetworkingCastPrivateDelegate(
On 2017/03/16 04:02:38, tbarzic wrote:
> On 2017/03/16 01:26:21, Devlin wrote:
> > nit: prefer base::MakeUnique
> 
> I tried initially, but MakeUnique does not work well with private ctors.

Ack; forgot that MakeUnique had that limitation.

[tbarzic, 2017-03-16 17:00:37]

The CQ bit was checked by tbarzic@chromium.org

[tbarzic, 2017-03-16 17:00:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2726223004/#ps360001
(title: "histograms")

[commit-bot: I haz the power, 2017-03-16 17:01:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 18:04:47]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1489683637132830,
"parent_rev": "7f75cce9905c0fe40761f247a47bfc66ef2f3321", "commit_rev":
"d2ff1ae2c1175126133ec36bc400b3aaed08092b"}

[commit-bot: I haz the power, 2017-03-16 18:05:32]

Description was changed from

==========
Introduce networking.cast API

The new API will be private to cast extension, and will contain
networkingPrivate methods only used/needed by cast extension and setup
app.
Method implementations are not changed, they are just copied from
networkingPrivate API to networking.cast API. The goal is to
eventually remove networkingPrivate implementation - when extensions
white-listed for the API move to the networking.cast API.

List of methods that are being moved to networking.cast API:
verifyDestination
verifyDestinationAndEncryptCredentials
verifyDestinationAndEncryptData
setWifiTDLSEnabledState
getWifiTDLSStatus

For now, the ChromeNetworkingCast just calls into the networking
private API's crypto verification delegate - the next step will be to
move this logic into ChromeNetworkingCastDelegate.

BUG=678737
==========

to

==========
Introduce networking.cast API

The new API will be private to cast extension, and will contain
networkingPrivate methods only used/needed by cast extension and setup
app.
Method implementations are not changed, they are just copied from
networkingPrivate API to networking.cast API. The goal is to
eventually remove networkingPrivate implementation - when extensions
white-listed for the API move to the networking.cast API.

List of methods that are being moved to networking.cast API:
verifyDestination
verifyDestinationAndEncryptCredentials
verifyDestinationAndEncryptData
setWifiTDLSEnabledState
getWifiTDLSStatus

For now, the ChromeNetworkingCast just calls into the networking
private API's crypto verification delegate - the next step will be to
move this logic into ChromeNetworkingCastDelegate.

BUG=678737

Review-Url: https://codereview.chromium.org/2726223004
Cr-Commit-Position: refs/heads/master@{#457489}
Committed:
https://chromium.googlesource.com/chromium/src/+/d2ff1ae2c1175126133ec36bc400...
==========

[commit-bot: I haz the power, 2017-03-16 18:05:34]

Committed patchset #19 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/d2ff1ae2c1175126133ec36bc400...