Cleaning up src/media to be consistent with static versus anonymous namespaces.

Cleaning up src/media to be consistent with static versus anonymous namespaces.
    
We were using a mix of both (and sometimes even static functions *inside* anonymous namespaces!) so we decided to stick to using static.
    
Also moved static/test code into media namespace and removed media:: prefixes.
    
BUG=none
TEST=compiles

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77135

[scherkus (not reviewing), 2011-03-04 20:42:34]

Few things to watch out for (and I don't think I'm consistent here):
  1) what about classes defined in .cc ?
  2) static stuff goes inside namespace media or not?
  3) any different feelings w.r.t. unit test files?

[tfarina, 2011-03-04 20:58:32]

On 2011/03/04 20:42:34, scherkus wrote:
> Few things to watch out for (and I don't think I'm consistent here):
>   1) what about classes defined in .cc ?
>   2) static stuff goes inside namespace media or not?
>   3) any different feelings w.r.t. unit test files?

Would be good to discuss this in chromium-dev mailing-list too? Nico (thakis)
has noted to me that neither of anonymous namespace or the static keyword are
really necessary in the .cc files. But people keep using it. If they are not
necessary, we could make a decision about, and enforce it later.

[Ami GONE FROM CHROMIUM, 2011-03-04 21:15:19]

One of static or anonymous namespace is required to avoid polluting the
containing namespace.  With neither approach, symbol conflicts can arise in
seemingly unrelated changes.
I'm not sure it's possible/necessary to get chromium-wide consensus on this
question (fwiw that's why the google style guide is silent on the subject),
and I think it's worth the trouble to make media/ at least internally
consistent.
If chromium-wide consensus is achieved, so much the better.

For the record, some of the reasons I prefer static:
a) when function declaration gets away (vertically) from the anon namespace
opening it can be hard to see/remember the anonymity, vs. static being right
there in the declaration (esp. when moving a function around inside a file).
b) static is friendlier to text-based search tools.
c) static doesn't mangle symbol names for later debugging confusion.

1) what about classes defined in .cc ?
>

I think you mean declared, not defined.
These are sufficiently rare that I think the namespace-pollution problem is
ignorable.

2) static stuff goes inside namespace media or not?
>

IMO yes, b/c it's simpler, avoids opening/closing namespaces, and avoids
having to specify media:: over and over inside the static stuff.


> 3) any different feelings w.r.t. unit test files?


Nope.

[tfarina, 2011-03-04 21:38:37]

On Fri, Mar 4, 2011 at 6:15 PM, Ami Fischman <fischman@chromium.org> wrote:
> One of static or anonymous namespace is required to avoid polluting the
> containing namespace.  With neither approach, symbol conflicts can arise in
> seemingly unrelated changes.
> I'm not sure it's possible/necessary to get chromium-wide consensus on this
> question (fwiw that's why the google style guide is silent on the subject),
> and I think it's worth the trouble to make media/ at least internally
> consistent.
> If chromium-wide consensus is achieved, so much the better.
> For the record, some of the reasons I prefer static:
> a) when function declaration gets away (vertically) from the anon namespace
> opening it can be hard to see/remember the anonymity, vs. static being right
> there in the declaration (esp. when moving a function around inside a file).
> b) static is friendlier to text-based search tools.
> c) static doesn't mangle symbol names for later debugging confusion.

+1 for the use of static keyword versus anonymous namespace so (but I
think people tend to use anonymous namespace to avoid typing 'static'
all around when there is many static constants and private functions).
And beside of all the technical reasons, because that can save a lot
of lines, that otherwise could be wasted.

-- 
A: Because it messes up the order in which people normally read text.
 See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

-Thiago

[scherkus (not reviewing), 2011-03-04 22:06:21]

thanks for the feedback

updated CL to move static/test stuff into respective namespaces

[Ami GONE FROM CHROMIUM, 2011-03-04 22:52:04]

LGTM

http://codereview.chromium.org/6628020/diff/1046/media/audio/audio_input_cont...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/6628020/diff/1046/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:12: static const int kMaxInputChannels =
2;
FWIW, for integral constants, using an unnamed enum is even sexier IMO, since it
requires no storage (which is more fun than out-of-line storage declarations for
static constants in classes, and by association even for bare constants):
enum { kMaxInputChannels = 2 };

Using an enum also prevents taking addresses/references of these literals, which
is almost always a good thing.

http://codereview.chromium.org/6628020/diff/1046/media/audio/audio_input_cont...
File media/audio/audio_input_controller_unittest.cc (left):

http://codereview.chromium.org/6628020/diff/1046/media/audio/audio_input_cont...
media/audio/audio_input_controller_unittest.cc:30: // Test AudioInputController
for create and close without recording audio.
Do you know if this was a TODO or what?

http://codereview.chromium.org/6628020/diff/1046/media/audio/audio_output_con...
File media/audio/audio_output_controller.cc (left):

http://codereview.chromium.org/6628020/diff/1046/media/audio/audio_output_con...
media/audio/audio_output_controller.cc:17: return false;
Wow.

[Ami GONE FROM CHROMIUM, 2011-03-04 22:53:11]

>
> LGTM
>

Except trybots seem unhappy.

[Ami GONE FROM WEBRTC_CHROMIUM, 2011-03-04 22:55:13]

>
> LGTM
>

Except trybots seem unhappy.

[scherkus (not reviewing), 2011-03-07 14:57:56]

http://codereview.chromium.org/6628020/diff/1046/media/audio/audio_input_cont...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/6628020/diff/1046/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:12: static const int kMaxInputChannels =
2;
On 2011/03/04 22:52:04, fischman wrote:
> FWIW, for integral constants, using an unnamed enum is even sexier IMO, since
it
> requires no storage (which is more fun than out-of-line storage declarations
for
> static constants in classes, and by association even for bare constants):
> enum { kMaxInputChannels = 2 };
> 
> Using an enum also prevents taking addresses/references of these literals,
which
> is almost always a good thing.

Neat suggestion!  I'll keep that around for another clean-up task and get this
CL landed first.

http://codereview.chromium.org/6628020/diff/1046/media/audio/audio_input_cont...
File media/audio/audio_input_controller_unittest.cc (left):

http://codereview.chromium.org/6628020/diff/1046/media/audio/audio_input_cont...
media/audio/audio_input_controller_unittest.cc:30: // Test AudioInputController
for create and close without recording audio.
On 2011/03/04 22:52:04, fischman wrote:
> Do you know if this was a TODO or what?

Believe it corresponds with TEST(AudioInputControllerTest, CreateAndClose)

I'll move the comment down to the test case

http://codereview.chromium.org/6628020/diff/1046/media/audio/audio_output_con...
File media/audio/audio_output_controller.cc (left):

http://codereview.chromium.org/6628020/diff/1046/media/audio/audio_output_con...
media/audio/audio_output_controller.cc:17: return false;
On 2011/03/04 22:52:04, fischman wrote:
> Wow.

I think when AudioParameters::IsValid() was created from a refactoring I guess
no one caught this part