[cast_channel] Make cast_channel related files not depend on "cast_channel.h"

[cast_channel] Make cast_channel related files not depend on "cast_channel.h"

We are moving cast_channel related files from //extensions to //components and need to remove extensions dependencies.

- Added internal enum class ::cast_channel::ReadyState, ::cast_channel::ChannelError, ::cast_channel::ChannelAuthType and type conversion util functions
- Use internal enum class instead of enums in cast_channel.idl

BUG=723944
Review-Url: https://codereview.chromium.org/2891923004
Cr-Commit-Position: refs/heads/master@{#475733}
Committed: https://chromium.googlesource.com/chromium/src/+/13da92ea84809980287797b7829007e8b1953ffc

[zhaobin, 2017-05-18 18:50:05]

zhaobin@chromium.org changed reviewers:
+ mfoltz@chromium.org

[mark a. foltz, 2017-05-22 20:01:00]

Is the plan to move the *Internal enums to //components along with the Cast
Socket implementation code?

Would it make more sense to check the enums into //components first (in their
own namespace, e.g. cast_channel::) and then you wouldn't need to add "Internal"
to these temporary copies?

[zhaobin, 2017-05-23 09:18:08]

Patchset #2 (id:20001) has been deleted

[zhaobin, 2017-05-23 17:37:26]

On 2017/05/22 20:01:00, mark a. foltz wrote:
> Is the plan to move the *Internal enums to //components along with the Cast
> Socket implementation code?
> 
> Would it make more sense to check the enums into //components first (in their
> own namespace, e.g. cast_channel::) and then you wouldn't need to add
"Internal"
> to these temporary copies?

Done.

[zhaobin, 2017-05-23 17:38:00]

zhaobin@chromium.org changed reviewers:
+ imcheng@chromium.org

[zhaobin, 2017-05-23 17:38:01]

+ imcheng@

[imcheng, 2017-05-24 22:11:24]

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
File components/cast_channel/BUILD.gn (right):

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
components/cast_channel/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
s/2016/2017

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
File components/cast_channel/cast_channel_type.cc (right):

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
components/cast_channel/cast_channel_type.cc:26: }
}  // namespace

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
components/cast_channel/cast_channel_type.cc:29: return
kReadyStates[static_cast<int>(ready_state)];
This looks ok, but could be a problem if we added a new value without updating
the array above.

Using a switch statement seems a bit safer. Maybe a macro such as the following
would help with reducing boilerplate, if it's ok to change the string values:
https://cs.corp.google.com/piper///depot/google3/chrome_platform/cast/logging...

It seems we should have some unit tests for these.

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
File components/cast_channel/cast_channel_type.h (right):

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
components/cast_channel/cast_channel_type.h:12: // Map to enum ReadyState in
cast_channel.idl
s/Map/Maps, here and below.

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
components/cast_channel/cast_channel_type.h:42: std::string ToString(ReadyState
ready_state);
Prefer ReadyStateToString, ChannelErrorToString, ChannelAuthTypeToString rather
than overloading ToString.

https://codereview.chromium.org/2891923004/diff/40001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/2891923004/diff/40001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_channel_api.cc:319:
cast_channel::ToChannelAuthTypeInternal(channel_auth_),
Instead of this, can you just use ToChannelAuthType() when setting channel_auth_
above?

https://codereview.chromium.org/2891923004/diff/40001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_channel_type_util.cc (right):

https://codereview.chromium.org/2891923004/diff/40001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_channel_type_util.cc:13: return
static_cast<api::cast_channel::ReadyState>(ready_state);
static_cast from one enum to another isn't really safe. Again I think switch
statements is safer here (though a bit more verbose)

[mark a. foltz, 2017-05-24 22:46:36]

Looks good with Derek's comments addressed.

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
File components/cast_channel/cast_channel_type.cc (right):

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
components/cast_channel/cast_channel_type.cc:29: return
kReadyStates[static_cast<int>(ready_state)];
On 2017/05/24 at 22:11:23, imcheng wrote:
> It seems we should have some unit tests for these.

If these ToString methods are only for logging, then I'm not sure a unit test is
necessary.  However a switch statement is safer to avoid an out of bounds array
access if values are added.

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
File components/cast_channel/cast_channel_type.h (right):

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
components/cast_channel/cast_channel_type.h:5: #ifndef
COMPONENTS_CAST_CHANNEL_CAST_CHANNEL_TYPE_H_
Maybe cast_channel_enum.h?

[zhaobin, 2017-05-25 00:49:23]

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
File components/cast_channel/BUILD.gn (right):

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
components/cast_channel/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
On 2017/05/24 22:11:23, imcheng wrote:
> s/2016/2017

Done.

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
File components/cast_channel/cast_channel_type.cc (right):

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
components/cast_channel/cast_channel_type.cc:26: }
On 2017/05/24 22:11:23, imcheng wrote:
> }  // namespace

Code removed.

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
components/cast_channel/cast_channel_type.cc:29: return
kReadyStates[static_cast<int>(ready_state)];
On 2017/05/24 22:11:23, imcheng wrote:
> This looks ok, but could be a problem if we added a new value without updating
> the array above.
> 
> Using a switch statement seems a bit safer. Maybe a macro such as the
following
> would help with reducing boilerplate, if it's ok to change the string values:
>
https://cs.corp.google.com/piper///depot/google3/chrome_platform/cast/logging...
> 
> It seems we should have some unit tests for these.

Done.

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
components/cast_channel/cast_channel_type.cc:29: return
kReadyStates[static_cast<int>(ready_state)];
On 2017/05/24 22:46:36, mark a. foltz wrote:
> On 2017/05/24 at 22:11:23, imcheng wrote:
> > It seems we should have some unit tests for these.
> 
> If these ToString methods are only for logging, then I'm not sure a unit test
is
> necessary.  However a switch statement is safer to avoid an out of bounds
array
> access if values are added.

Done.

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
File components/cast_channel/cast_channel_type.h (right):

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
components/cast_channel/cast_channel_type.h:5: #ifndef
COMPONENTS_CAST_CHANNEL_CAST_CHANNEL_TYPE_H_
On 2017/05/24 22:46:36, mark a. foltz wrote:
> Maybe cast_channel_enum.h?

Done.

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
components/cast_channel/cast_channel_type.h:12: // Map to enum ReadyState in
cast_channel.idl
On 2017/05/24 22:11:23, imcheng wrote:
> s/Map/Maps, here and below.

Done.

https://codereview.chromium.org/2891923004/diff/40001/components/cast_channel...
components/cast_channel/cast_channel_type.h:42: std::string ToString(ReadyState
ready_state);
On 2017/05/24 22:11:23, imcheng wrote:
> Prefer ReadyStateToString, ChannelErrorToString, ChannelAuthTypeToString
rather
> than overloading ToString.

Done.

https://codereview.chromium.org/2891923004/diff/40001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/2891923004/diff/40001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_channel_api.cc:319:
cast_channel::ToChannelAuthTypeInternal(channel_auth_),
On 2017/05/24 22:11:23, imcheng wrote:
> Instead of this, can you just use ToChannelAuthType() when setting
channel_auth_
> above?

Done.

https://codereview.chromium.org/2891923004/diff/40001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_channel_type_util.cc (right):

https://codereview.chromium.org/2891923004/diff/40001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_channel_type_util.cc:13: return
static_cast<api::cast_channel::ReadyState>(ready_state);
On 2017/05/24 22:11:23, imcheng wrote:
> static_cast from one enum to another isn't really safe. Again I think switch
> statements is safer here (though a bit more verbose)

Done.

[imcheng, 2017-05-25 16:36:10]

lgtm

[mark a. foltz, 2017-05-26 16:55:51]

lgtm

You'll need to get a components/OWNERS to also review and approve.

https://codereview.chromium.org/2891923004/diff/80001/components/cast_channel...
File components/cast_channel/cast_channel_enum_unittest.cc (right):

https://codereview.chromium.org/2891923004/diff/80001/components/cast_channel...
components/cast_channel/cast_channel_enum_unittest.cc:11:
TEST(CastChannelTypeTest, TestReadyStateToString) {
Thanks for writing the unit test.  But I don't think it's worth the compilation
or maintenance time to test a function only used for debug logging.  Derek do
you feel strongly this is necessary?

https://codereview.chromium.org/2891923004/diff/80001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_channel_enum_util.cc (right):

https://codereview.chromium.org/2891923004/diff/80001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_channel_enum_util.cc:25: NOTREACHED();
<< "Unknown ready_state " << ready_state

Here and below.

[mark a. foltz, 2017-05-26 16:56:43]

https://codereview.chromium.org/2891923004/diff/80001/components/cast_channel...
File components/cast_channel/cast_channel_enum.cc (right):

https://codereview.chromium.org/2891923004/diff/80001/components/cast_channel...
components/cast_channel/cast_channel_enum.cc:24: NOTREACHED();
Also << "Unknown ready_state " << ready_state

here and below

[mark a. foltz, 2017-05-26 16:57:37]

Finally, can you add a components/cast_channel/OWNERS file that references the
existing OWNERS file in extensions/?  Thanks!

[imcheng, 2017-05-26 17:43:36]

lgtm

https://codereview.chromium.org/2891923004/diff/80001/components/cast_channel...
File components/cast_channel/cast_channel_enum.cc (right):

https://codereview.chromium.org/2891923004/diff/80001/components/cast_channel...
components/cast_channel/cast_channel_enum.cc:23: default:
Maybe get rid of the default case? It will help us catch new enums that we
forget to add here, in compile time.

https://codereview.chromium.org/2891923004/diff/80001/components/cast_channel...
File components/cast_channel/cast_channel_enum_unittest.cc (right):

https://codereview.chromium.org/2891923004/diff/80001/components/cast_channel...
components/cast_channel/cast_channel_enum_unittest.cc:11:
TEST(CastChannelTypeTest, TestReadyStateToString) {
On 2017/05/26 16:55:51, mark a. foltz wrote:
> Thanks for writing the unit test.  But I don't think it's worth the
compilation
> or maintenance time to test a function only used for debug logging.  Derek do
> you feel strongly this is necessary?

No I don't feel strongly. Thinking about it a bit more, using switch statements
should help us catch problems during compile time so unit tests are not
necessary.

[zhaobin, 2017-05-26 21:59:02]

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

[commit-bot: I haz the power, 2017-05-26 21:59: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-05-26 22:03:27]

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

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

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zhaobin, 2017-05-27 03:03:38]

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

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

Dry run: CQ is trying da patch.

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

[zhaobin, 2017-05-27 03:08:37]

zhaobin@chromium.org changed reviewers:
+ jochen@chromium.org, rdevlin.cronin@chromium.org

[zhaobin, 2017-05-27 03:08:38]

+jochen@ to review components/cast_channel/*

+rdevlin.cronin@ to review extensions/browser/BUILD.gn

https://codereview.chromium.org/2891923004/diff/80001/components/cast_channel...
File components/cast_channel/cast_channel_enum.cc (right):

https://codereview.chromium.org/2891923004/diff/80001/components/cast_channel...
components/cast_channel/cast_channel_enum.cc:23: default:
On 2017/05/26 17:43:36, imcheng wrote:
> Maybe get rid of the default case? It will help us catch new enums that we
> forget to add here, in compile time.

Done.

https://codereview.chromium.org/2891923004/diff/80001/components/cast_channel...
components/cast_channel/cast_channel_enum.cc:24: NOTREACHED();
On 2017/05/26 16:56:42, mark a. foltz wrote:
> Also << "Unknown ready_state " << ready_state
> 
> here and below

Done.

https://codereview.chromium.org/2891923004/diff/80001/components/cast_channel...
File components/cast_channel/cast_channel_enum_unittest.cc (right):

https://codereview.chromium.org/2891923004/diff/80001/components/cast_channel...
components/cast_channel/cast_channel_enum_unittest.cc:11:
TEST(CastChannelTypeTest, TestReadyStateToString) {
On 2017/05/26 17:43:36, imcheng wrote:
> On 2017/05/26 16:55:51, mark a. foltz wrote:
> > Thanks for writing the unit test.  But I don't think it's worth the
> compilation
> > or maintenance time to test a function only used for debug logging.  Derek
do
> > you feel strongly this is necessary?
> 
> No I don't feel strongly. Thinking about it a bit more, using switch
statements
> should help us catch problems during compile time so unit tests are not
> necessary.

Done.

https://codereview.chromium.org/2891923004/diff/80001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_channel_enum_util.cc (right):

https://codereview.chromium.org/2891923004/diff/80001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_channel_enum_util.cc:25: NOTREACHED();
On 2017/05/26 16:55:51, mark a. foltz wrote:
> << "Unknown ready_state " << ready_state
> 
> Here and below.

Done.

[commit-bot: I haz the power, 2017-05-27 03:48:21]

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

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

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

[zhaobin, 2017-05-27 05:22:48]

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

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

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-05-27 07:01:00]

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

[commit-bot: I haz the power, 2017-05-27 07:01:01]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-05-29 14:15:51]

lgtm

[mark a. foltz, 2017-05-30 21:20:08]

Can you update the patch description to match the current patchset?
Thanks :-)

[zhaobin, 2017-05-30 21:33:45]

Description was changed from

==========
[cast_channel] Make cast_channel related files not depend on "cast_channel.h"

We are moving cast_channel related files from //extensions to //components and
need to remove extensions dependencies.

- Added internal enum class ReadyStateInternal, ChannelErrorInternal,
ChannelAuthTypeInternal and type conversion util functions
- Use internal enum class instead of enums in cast_channel.idl

BUG=723944
==========

to

==========
[cast_channel] Make cast_channel related files not depend on "cast_channel.h"

We are moving cast_channel related files from //extensions to //components and
need to remove extensions dependencies.

- Added internal enum class ::cast_channel::ReadyState,
::cast_channel::ChannelError, ::cast_channel::ChannelAuthType and type
conversion util functions
- Use internal enum class instead of enums in cast_channel.idl

BUG=723944
==========

[Devlin, 2017-05-30 22:35:13]

extensions/browser/BUILD.gn lgtm

[zhaobin, 2017-05-30 22:40:34]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-05-30 22:40:35]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2891923004/#ps140001
(title: "fix windows compile errors")

[commit-bot: I haz the power, 2017-05-30 22:41:16]

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-05-31 01:50:24]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496184034847310,
"parent_rev": "182740235efc11d585d6710485d66fff4fcf84ae", "commit_rev":
"13da92ea84809980287797b7829007e8b1953ffc"}

[commit-bot: I haz the power, 2017-05-31 01:50:35]

Description was changed from

==========
[cast_channel] Make cast_channel related files not depend on "cast_channel.h"

We are moving cast_channel related files from //extensions to //components and
need to remove extensions dependencies.

- Added internal enum class ::cast_channel::ReadyState,
::cast_channel::ChannelError, ::cast_channel::ChannelAuthType and type
conversion util functions
- Use internal enum class instead of enums in cast_channel.idl

BUG=723944
==========

to

==========
[cast_channel] Make cast_channel related files not depend on "cast_channel.h"

We are moving cast_channel related files from //extensions to //components and
need to remove extensions dependencies.

- Added internal enum class ::cast_channel::ReadyState,
::cast_channel::ChannelError, ::cast_channel::ChannelAuthType and type
conversion util functions
- Use internal enum class instead of enums in cast_channel.idl

BUG=723944

Review-Url: https://codereview.chromium.org/2891923004
Cr-Commit-Position: refs/heads/master@{#475733}
Committed:
https://chromium.googlesource.com/chromium/src/+/13da92ea84809980287797b78290...
==========

[commit-bot: I haz the power, 2017-05-31 01:50:37]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/13da92ea84809980287797b78290...