Change the media::PowerLineFrequency from an enum class to a proper class

Change the media::PowerLineFrequency from an enum class to a proper class
so that querying its value in conditional statements won't require ugly
static_cast(s)

BUG=525167

[suvanjanmukherjee, 2015-09-08 23:43:06]

suvanjanmukherjee@gmail.com changed reviewers:
+ ajose@chromium.org, wolenetz@chromium.org

[suvanjanmukherjee, 2015-09-08 23:46:33]

Please have a look at my change that changes the PowerLineFrequency enum used to
map to integer frequencies to a C++ class which supports the int() operator so
that using it in condition expressions doesn't require static_casts.

I added unit tests to ensure the objects of this class is actually mapping to
the expected integers. This is my first ever chromium contribution (as a matter
of fact, first ever open source contribution) so any comments regarding coding
style, suggested alternative implementations are welcome.

[wolenetz, 2015-09-09 19:39:15]

wolenetz@chromium.org changed reviewers:
+ dcheng@chromium.org, mcasas@chromium.org

[wolenetz, 2015-09-09 19:39:15]

Thank you for working on this!

I've provided some general comments below, mostly around style, and looped in a
couple further reviewers whom I think are also necessary:

mcasas@ & dcheng@, please also take a look at everything. Especially, I normally
use DISALLOW_COPY_AND_ASSIGN, so the intentional lack of same in this CL could
use further review beyond mine.

Since PowerLineFrequency is used in IPC messaging, R+= dcheng@chromium.org for
security review of usage of the updated PowerLineFrequency in
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/med...
(suvanjanmukherjee@: please see the relevant portion of
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/med...
for contact info for other potential IPC message security reviewers if dcheng@
is unavailable).

Similarly, PowerLineFrequency, though declared in //media/base/..., it's
primarily used in //media/capture/video, which has a specific set of reviewers. 
R+=mcasas@chromium.org, though see also the OWNERS file for other potential
//media/capture/video reviewers:
https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide...

https://codereview.chromium.org/1323093006/diff/1/media/base/BUILD.gn
File media/base/BUILD.gn (right):

https://codereview.chromium.org/1323093006/diff/1/media/base/BUILD.gn#newcode412
media/base/BUILD.gn:412: "video_capture_types_unittest.cc",
Please also update //src/media/media.gyp

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
File media/base/video_capture_types.cc (right):

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.cc:14: void PowerLineFrequency::check(int freq)
nit: remember to follow the updated declaration order here in the impl, too.

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.cc:22: freq_ =  freq;
nit: s/  / /

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:59: private:
public before private:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:62: void check(int freq);
methods before data members:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:65: static constexpr int FREQUENCY_DEFAULT = 0;
constexpr usage is currently banned in Chromium (see
http://chromium-cpp.appspot.com/)

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:71: PowerLineFrequency(int freq);
use "explicit"

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:72: PowerLineFrequency& operator = (int freq);
For this and the next line, use consistent and correct whitespace.

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
File media/base/video_capture_types_unittest.cc (right):

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types_unittest.cc:16: case
PowerLineFrequency::FREQUENCY_50HZ: result = true; break;
nit: make the case and default code blocks multi-line. See, for example,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi...

[dcheng, 2015-09-09 22:50:09]

Why not just make this a plain old enum?

[suvanjanmukherjee, 2015-09-09 23:39:49]

On 2015/09/09 22:50:09, dcheng wrote:
> Why not just make this a plain old enum?

I guess the whole point of an enum class is to ensure people can't set arbitrary
integer values to these objects. The new class I implemented relaxes this strict
checking a bit because I only check upper and lower bounds but allow all values
in the middle. I could implement a check like
DCHECK(freq == FREQUENCY_DEFAULT || freq == FREQUENCY_50HZ || freq ==
FREQUENCY_60HZ) to completely imitate the previous behavior. Let me know what
you think.

[dcheng, 2015-09-10 00:08:13]

On 2015/09/09 at 23:39:49, suvanjanmukherjee wrote:
> On 2015/09/09 22:50:09, dcheng wrote:
> > Why not just make this a plain old enum?
> 
> I guess the whole point of an enum class is to ensure people can't set
arbitrary integer values to these objects. The new class I implemented relaxes
this strict checking a bit because I only check upper and lower bounds but allow
all values in the middle. I could implement a check like
> DCHECK(freq == FREQUENCY_DEFAULT || freq == FREQUENCY_50HZ || freq ==
FREQUENCY_60HZ) to completely imitate the previous behavior. Let me know what
you think.

OK. I didn't look closely enough at this patch the first time. To be honest, I
think the enum member names don't add much.
  int power_line_frequency = 50;
is probably just as descriptive as:
  PowerLineFrequency power_line_frequency = PowerLineFrequency::FREQUENCY_50HZ
and much less verbose.

That being said, if you want to keep the enums (I can see some arguments for
this), I'd suggest an alternate approach: change
VideoCaptureDevice::GetPowerLineFrequencyForLocation(),
VideoCaptureDevice::GetPowerLineFrequency(),
VideoCaptureDeviceLinux::TranslatePowerLineFrequencyToV4L2() to pass the
frequency around as a PowerLineFrequency instead of an int. This avoids the need
for so much casting, and you don't need to implement this class with a bunch of
overloaded operators.

Btw, I think GetPowerLineFrequencyForLocation() is wrong: Japan uses both 50Hz
and 60Hz mains =)

[ajose, 2015-09-10 17:24:55]

Thanks a lot, Suvanjan!

Considering there are only a few valid states (50HZ, 60HZ, maybe 400HZ...)
AFAIK, I think it makes sense to keep this more constrained than just being an
int, despite the verbosity. I like dcheng@'s idea about changing functions to
pass PowerLineFrequency around over int.  

dcheng@: GetPowerLineFrequencyForLocation was the reason PowerLineFrequency was
added in the first place :) so that users in locations with multiple frequencies
can override the location-based frequency if they need to, by adding a
constraint to getUserMedia. That said, this is a stopgap while a more permanent
solution is worked out [1].

[1] https://code.google.com/p/chromium/issues/detail?id=520106

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
File media/base/video_capture_types.cc (right):

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.cc:15: {
Move open curly brace to the end of the same line as the last parameter: 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Decl...

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:79:
static_assert(static_cast<int>(PowerLineFrequency::FREQUENCY_DEFAULT) == 0,
Remove static_casts?

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
File media/base/video_capture_types_unittest.cc (right):

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types_unittest.cc:6: PowerLineFrequency
p50_a(PowerLineFrequency::FREQUENCY_50HZ);
Consider using more descriptive variable names.

[suvanjanmukherjee, 2015-09-13 15:32:25]

suvanjanmukherjee@gmail.com changed reviewers:
+ xhwang@chromium.org

[suvanjanmukherjee, 2015-09-13 15:33:46]

Incorporating suggestions from previous review and passing around
PowerLineFrequency objects in VideoDeviceCapture instead of integers as
suggested in the previous review.

https://codereview.chromium.org/1323093006/diff/1/media/base/BUILD.gn
File media/base/BUILD.gn (right):

https://codereview.chromium.org/1323093006/diff/1/media/base/BUILD.gn#newcode412
media/base/BUILD.gn:412: "video_capture_types_unittest.cc",
On 2015/09/09 19:39:15, wolenetz wrote:
> Please also update //src/media/media.gyp

Done.

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
File media/base/video_capture_types.cc (right):

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.cc:14: void PowerLineFrequency::check(int freq)
On 2015/09/09 19:39:15, wolenetz wrote:
> nit: remember to follow the updated declaration order here in the impl, too.

Done.

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.cc:15: {
On 2015/09/10 17:24:55, ajose wrote:
> Move open curly brace to the end of the same line as the last parameter: 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Decl...

Done.

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.cc:22: freq_ =  freq;
On 2015/09/09 19:39:15, wolenetz wrote:
> nit: s/  / /

Done.

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:59: private:
On 2015/09/09 19:39:15, wolenetz wrote:
> public before private:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

Done.

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:62: void check(int freq);
On 2015/09/09 19:39:15, wolenetz wrote:
> methods before data members:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

Done.

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:65: static constexpr int FREQUENCY_DEFAULT = 0;
On 2015/09/09 19:39:15, wolenetz wrote:
> constexpr usage is currently banned in Chromium (see
> http://chromium-cpp.appspot.com/)

Done.

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:71: PowerLineFrequency(int freq);
On 2015/09/09 19:39:15, wolenetz wrote:
> use "explicit"

Done.

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:72: PowerLineFrequency& operator = (int freq);
On 2015/09/09 19:39:15, wolenetz wrote:
> For this and the next line, use consistent and correct whitespace.

Done.

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:79:
static_assert(static_cast<int>(PowerLineFrequency::FREQUENCY_DEFAULT) == 0,
On 2015/09/10 17:24:55, ajose wrote:
> Remove static_casts?

Done.

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
File media/base/video_capture_types_unittest.cc (right):

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types_unittest.cc:6: PowerLineFrequency
p50_a(PowerLineFrequency::FREQUENCY_50HZ);
On 2015/09/10 17:24:55, ajose wrote:
> Consider using more descriptive variable names.

Using only one variable in the new test. Simple setting and testing. Please let
me know if using more descriptive names is still advisable

https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
media/base/video_capture_types_unittest.cc:16: case
PowerLineFrequency::FREQUENCY_50HZ: result = true; break;
On 2015/09/09 19:39:15, wolenetz wrote:
> nit: make the case and default code blocks multi-line. See, for example,
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi...

No longer using a switch case here

[dcheng, 2015-09-13 15:55:22]

On 2015/09/13 at 15:33:46, suvanjanmukherjee wrote:
> Incorporating suggestions from previous review and passing around
PowerLineFrequency objects in VideoDeviceCapture instead of integers as
suggested in the previous review.
> 
> https://codereview.chromium.org/1323093006/diff/1/media/base/BUILD.gn
> File media/base/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/BUILD.gn#newcode412
> media/base/BUILD.gn:412: "video_capture_types_unittest.cc",
> On 2015/09/09 19:39:15, wolenetz wrote:
> > Please also update //src/media/media.gyp
> 
> Done.
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> File media/base/video_capture_types.cc (right):
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> media/base/video_capture_types.cc:14: void PowerLineFrequency::check(int freq)
> On 2015/09/09 19:39:15, wolenetz wrote:
> > nit: remember to follow the updated declaration order here in the impl, too.
> 
> Done.
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> media/base/video_capture_types.cc:15: {
> On 2015/09/10 17:24:55, ajose wrote:
> > Move open curly brace to the end of the same line as the last parameter: 
> >
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Decl...
> 
> Done.
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> media/base/video_capture_types.cc:22: freq_ =  freq;
> On 2015/09/09 19:39:15, wolenetz wrote:
> > nit: s/  / /
> 
> Done.
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> File media/base/video_capture_types.h (right):
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> media/base/video_capture_types.h:59: private:
> On 2015/09/09 19:39:15, wolenetz wrote:
> > public before private:
> >
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
> 
> Done.
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> media/base/video_capture_types.h:62: void check(int freq);
> On 2015/09/09 19:39:15, wolenetz wrote:
> > methods before data members:
> >
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
> 
> Done.
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> media/base/video_capture_types.h:65: static constexpr int FREQUENCY_DEFAULT =
0;
> On 2015/09/09 19:39:15, wolenetz wrote:
> > constexpr usage is currently banned in Chromium (see
> > http://chromium-cpp.appspot.com/)
> 
> Done.
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> media/base/video_capture_types.h:71: PowerLineFrequency(int freq);
> On 2015/09/09 19:39:15, wolenetz wrote:
> > use "explicit"
> 
> Done.
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> media/base/video_capture_types.h:72: PowerLineFrequency& operator = (int
freq);
> On 2015/09/09 19:39:15, wolenetz wrote:
> > For this and the next line, use consistent and correct whitespace.
> 
> Done.
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> media/base/video_capture_types.h:79:
static_assert(static_cast<int>(PowerLineFrequency::FREQUENCY_DEFAULT) == 0,
> On 2015/09/10 17:24:55, ajose wrote:
> > Remove static_casts?
> 
> Done.
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> File media/base/video_capture_types_unittest.cc (right):
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> media/base/video_capture_types_unittest.cc:6: PowerLineFrequency
p50_a(PowerLineFrequency::FREQUENCY_50HZ);
> On 2015/09/10 17:24:55, ajose wrote:
> > Consider using more descriptive variable names.
> 
> Using only one variable in the new test. Simple setting and testing. Please
let me know if using more descriptive names is still advisable
> 
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> media/base/video_capture_types_unittest.cc:16: case
PowerLineFrequency::FREQUENCY_50HZ: result = true; break;
> On 2015/09/09 19:39:15, wolenetz wrote:
> > nit: make the case and default code blocks multi-line. See, for example,
> >
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi...
> 
> No longer using a switch case here

Why not just change this back to an enum class? The class is unnecessary, now
that the need to cast is largely eliminated.

[wolenetz, 2015-09-24 19:10:07]

On 2015/09/13 15:55:22, dcheng wrote:
> On 2015/09/13 at 15:33:46, suvanjanmukherjee wrote:
> > Incorporating suggestions from previous review and passing around
> PowerLineFrequency objects in VideoDeviceCapture instead of integers as
> suggested in the previous review.
> > 
> > https://codereview.chromium.org/1323093006/diff/1/media/base/BUILD.gn
> > File media/base/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/BUILD.gn#newcode412
> > media/base/BUILD.gn:412: "video_capture_types_unittest.cc",
> > On 2015/09/09 19:39:15, wolenetz wrote:
> > > Please also update //src/media/media.gyp
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> > File media/base/video_capture_types.cc (right):
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> > media/base/video_capture_types.cc:14: void PowerLineFrequency::check(int
freq)
> > On 2015/09/09 19:39:15, wolenetz wrote:
> > > nit: remember to follow the updated declaration order here in the impl,
too.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> > media/base/video_capture_types.cc:15: {
> > On 2015/09/10 17:24:55, ajose wrote:
> > > Move open curly brace to the end of the same line as the last parameter: 
> > >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Decl...
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> > media/base/video_capture_types.cc:22: freq_ =  freq;
> > On 2015/09/09 19:39:15, wolenetz wrote:
> > > nit: s/  / /
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> > File media/base/video_capture_types.h (right):
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> > media/base/video_capture_types.h:59: private:
> > On 2015/09/09 19:39:15, wolenetz wrote:
> > > public before private:
> > >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> > media/base/video_capture_types.h:62: void check(int freq);
> > On 2015/09/09 19:39:15, wolenetz wrote:
> > > methods before data members:
> > >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> > media/base/video_capture_types.h:65: static constexpr int FREQUENCY_DEFAULT
=
> 0;
> > On 2015/09/09 19:39:15, wolenetz wrote:
> > > constexpr usage is currently banned in Chromium (see
> > > http://chromium-cpp.appspot.com/)
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> > media/base/video_capture_types.h:71: PowerLineFrequency(int freq);
> > On 2015/09/09 19:39:15, wolenetz wrote:
> > > use "explicit"
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> > media/base/video_capture_types.h:72: PowerLineFrequency& operator = (int
> freq);
> > On 2015/09/09 19:39:15, wolenetz wrote:
> > > For this and the next line, use consistent and correct whitespace.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> > media/base/video_capture_types.h:79:
> static_assert(static_cast<int>(PowerLineFrequency::FREQUENCY_DEFAULT) == 0,
> > On 2015/09/10 17:24:55, ajose wrote:
> > > Remove static_casts?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> > File media/base/video_capture_types_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> > media/base/video_capture_types_unittest.cc:6: PowerLineFrequency
> p50_a(PowerLineFrequency::FREQUENCY_50HZ);
> > On 2015/09/10 17:24:55, ajose wrote:
> > > Consider using more descriptive variable names.
> > 
> > Using only one variable in the new test. Simple setting and testing. Please
> let me know if using more descriptive names is still advisable
> > 
> >
>
https://codereview.chromium.org/1323093006/diff/1/media/base/video_capture_ty...
> > media/base/video_capture_types_unittest.cc:16: case
> PowerLineFrequency::FREQUENCY_50HZ: result = true; break;
> > On 2015/09/09 19:39:15, wolenetz wrote:
> > > nit: make the case and default code blocks multi-line. See, for example,
> > >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi...
> > 
> > No longer using a switch case here
> 
> Why not just change this back to an enum class? The class is unnecessary, now
> that the need to cast is largely eliminated.

Given there is a newer CL (https://codereview.chromium.org/1345993002/) being
reviewed that makes this one obsolete, please close this CL so that it doesn't
pollute reviewers' TODO lists :)

Thanks!
Matt