Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23723009:
Implement GetAudioOutputDeviceNames for Cras. (Closed)
|
Created:
7 years, 3 months ago by Jói Modified:
7 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Visibility:
Public. |
DescriptionImplement GetAudioOutputDeviceNames for Cras.
This does the same as the GetAudioInputDeviceNames implementation for
Cras, which is to say it just adds the default device to the list,
leaving it to Cras to route appropriately.
BUG=276894
Patch Set 1 #Patch Set 2 : Remove empty impl. #
Total comments: 5
Messages
Total messages: 12 (0 generated)
Currently untested, sending out for feedback on whether this seems right, will commit only after adding to the test. Cheers, Jói
Dylan - can you take a look at this? Is it OK to assume that all ChromeOS devices will always have exactly one device for input & output and there's really no way for the browser to enumerate them?
On 2013/09/09 18:13:42, tommi wrote: > Dylan - can you take a look at this? Is it OK to assume that all ChromeOS > devices will always have exactly one device for input & output and there's > really no way for the browser to enumerate them? Not exactly, with the addition of device selection to the system tray, Chrome now enumerates all devices and lets the user choose. That affects where the "default" device maps. We need to figure out how that selection will interact with the selection in this part of Chrome. If a user or web page picks a device for a given stream, does that stream move devices if the user selects a new default through the system ui? Ignoring those high level issues, this patch doesn't look like it will do any harm.
On 2013/09/09 20:32:26, dgreid wrote: > On 2013/09/09 18:13:42, tommi wrote: > > Dylan - can you take a look at this? Is it OK to assume that all ChromeOS > > devices will always have exactly one device for input & output and there's > > really no way for the browser to enumerate them? > > Not exactly, with the addition of device selection to the system tray, Chrome > now enumerates all devices and lets the user choose. That affects where the > "default" device maps. We need to figure out how that selection will interact > with the selection in this part of Chrome. If a user or web page picks a device > for a given stream, does that stream move devices if the user selects a new > default through the system ui? > > Ignoring those high level issues, this patch doesn't look like it will do any > harm. Sgtm. Lgtm.
Message was sent while issue was closed.
https://codereview.chromium.org/23723009/diff/3001/media/audio/cras/audio_man... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/23723009/diff/3001/media/audio/cras/audio_man... media/audio/cras/audio_manager_cras.cc:19: namespace { minor-question: hasn't media folks decided to use static over unnamed namespaces? https://codereview.chromium.org/23723009/diff/3001/media/audio/cras/audio_man... media/audio/cras/audio_manager_cras.cc:21: void AddDefaultDevice(media::AudioDeviceNames* device_names) { micro-style-nit: no media:: here.
Message was sent while issue was closed.
https://codereview.chromium.org/23723009/diff/3001/media/audio/cras/audio_man... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/23723009/diff/3001/media/audio/cras/audio_man... media/audio/cras/audio_manager_cras.cc:19: namespace { On 2013/09/15 03:17:45, tfarina wrote: > minor-question: hasn't media folks decided to use static over unnamed > namespaces? Not that I know of. Tommi? https://codereview.chromium.org/23723009/diff/3001/media/audio/cras/audio_man... media/audio/cras/audio_manager_cras.cc:21: void AddDefaultDevice(media::AudioDeviceNames* device_names) { On 2013/09/15 03:17:45, tfarina wrote: > micro-style-nit: no media:: here. Addressed, although not here (this patch is closed) but in https://codereview.chromium.org/23475037/ which consolidates this.
Message was sent while issue was closed.
https://codereview.chromium.org/23723009/diff/3001/media/audio/cras/audio_man... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/23723009/diff/3001/media/audio/cras/audio_man... media/audio/cras/audio_manager_cras.cc:19: namespace { On 2013/09/16 08:56:45, Jói wrote: > On 2013/09/15 03:17:45, tfarina wrote: > > minor-question: hasn't media folks decided to use static over unnamed > > namespaces? > > Not that I know of. Tommi? Yes, that's correct.
OK, will fix in the files I'm touching. I guess this means we use static for file-scoped functions, but we still use an anonymous namespace for file-scoped variables? Or do we try to use just static? The former seems to be fairly common, see e.g. https://code.google.com/p/chromium/codesearch#search/&q=namespace%5Cs*%7B%20f... where most of the usages of an anonymous namespace seem to be to enclose file-scoped constants and variables. Cheers, Jói On Mon, Sep 16, 2013 at 12:47 PM, <tommi@chromium.org> wrote: > > https://codereview.chromium.org/23723009/diff/3001/media/audio/cras/audio_man... > File media/audio/cras/audio_manager_cras.cc (right): > > https://codereview.chromium.org/23723009/diff/3001/media/audio/cras/audio_man... > media/audio/cras/audio_manager_cras.cc:19: namespace { > On 2013/09/16 08:56:45, Jói wrote: >> >> On 2013/09/15 03:17:45, tfarina wrote: >> > minor-question: hasn't media folks decided to use static over > > unnamed >> >> > namespaces? > > >> Not that I know of. Tommi? > > > Yes, that's correct. > > https://codereview.chromium.org/23723009/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Andrew gave me the details on this a couple of years back but I can't find it right now so I'm doing the lazy thing and ask :) My guess would have been "just static" but now I'm not so sure. Fwiw since most media folks don't only work in media/ it would be fine with me to just use the same style as in content/ and/or chrome/. On Mon, Sep 16, 2013 at 1:22 PM, Jói Sigurðsson <joi@chromium.org> wrote: > OK, will fix in the files I'm touching. > > I guess this means we use static for file-scoped functions, but we > still use an anonymous namespace for file-scoped variables? Or do we > try to use just static? The former seems to be fairly common, see e.g. > > https://code.google.com/p/chromium/codesearch#search/&q=namespace%5Cs*%7B%20f... > where most of the usages of an anonymous namespace seem to be to > enclose file-scoped constants and variables. > > Cheers, > Jói > > > On Mon, Sep 16, 2013 at 12:47 PM, <tommi@chromium.org> wrote: > > > > > https://codereview.chromium.org/23723009/diff/3001/media/audio/cras/audio_man... > > File media/audio/cras/audio_manager_cras.cc (right): > > > > > https://codereview.chromium.org/23723009/diff/3001/media/audio/cras/audio_man... > > media/audio/cras/audio_manager_cras.cc:19: namespace { > > On 2013/09/16 08:56:45, Jói wrote: > >> > >> On 2013/09/15 03:17:45, tfarina wrote: > >> > minor-question: hasn't media folks decided to use static over > > > > unnamed > >> > >> > namespaces? > > > > > >> Not that I know of. Tommi? > > > > > > Yes, that's correct. > > > > https://codereview.chromium.org/23723009/ > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks. FWIW, it's not important for the current CL, which only had a few functions in an anonymous namespace. Just wanted to clarify the policy if there is one. Cheers, Jói On Mon, Sep 16, 2013 at 4:16 PM, Tommi <tommi@chromium.org> wrote: > Andrew gave me the details on this a couple of years back but I can't find > it right now so I'm doing the lazy thing and ask :) > My guess would have been "just static" but now I'm not so sure. Fwiw since > most media folks don't only work in media/ it would be fine with me to just > use the same style as in content/ and/or chrome/. > > > On Mon, Sep 16, 2013 at 1:22 PM, Jói Sigurðsson <joi@chromium.org> wrote: >> >> OK, will fix in the files I'm touching. >> >> I guess this means we use static for file-scoped functions, but we >> still use an anonymous namespace for file-scoped variables? Or do we >> try to use just static? The former seems to be fairly common, see e.g. >> >> https://code.google.com/p/chromium/codesearch#search/&q=namespace%5Cs*%7B%20f... >> where most of the usages of an anonymous namespace seem to be to >> enclose file-scoped constants and variables. >> >> Cheers, >> Jói >> >> >> On Mon, Sep 16, 2013 at 12:47 PM, <tommi@chromium.org> wrote: >> > >> > >> > https://codereview.chromium.org/23723009/diff/3001/media/audio/cras/audio_man... >> > File media/audio/cras/audio_manager_cras.cc (right): >> > >> > >> > https://codereview.chromium.org/23723009/diff/3001/media/audio/cras/audio_man... >> > media/audio/cras/audio_manager_cras.cc:19: namespace { >> > On 2013/09/16 08:56:45, Jói wrote: >> >> >> >> On 2013/09/15 03:17:45, tfarina wrote: >> >> > minor-question: hasn't media folks decided to use static over >> > >> > unnamed >> >> >> >> > namespaces? >> > >> > >> >> Not that I know of. Tommi? >> > >> > >> > Yes, that's correct. >> > >> > https://codereview.chromium.org/23723009/ >> >> To unsubscribe from this group and stop receiving emails from it, send an >> email to chromium-reviews+unsubscribe@chromium.org. > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2013/09/16 15:28:56, Jói wrote: > Thanks. FWIW, it's not important for the current CL, which only had a > few functions in an anonymous namespace. Just wanted to clarify the > policy if there is one. Many moons ago (IIRC before the style was clarified), media folk preferred static not for speed but because it made for more legible symbols when debugging and the Java-ness requirement of sticking static before everything makes it clear what's static and what isn't as opposed to hunting for the anonymous namespace opening/closing braces. At this point it's more about keeping code consistent. The one place where it all breaks down is defining classes inside of .cc files. Sigh.
OK, thanks Andrew. On Tue, Sep 17, 2013 at 4:26 AM, <scherkus@chromium.org> wrote: > On 2013/09/16 15:28:56, Jói wrote: >> >> Thanks. FWIW, it's not important for the current CL, which only had a >> few functions in an anonymous namespace. Just wanted to clarify the >> policy if there is one. > > > Many moons ago (IIRC before the style was clarified), media folk preferred > static not for speed but because it made for more legible symbols when > debugging > and the Java-ness requirement of sticking static before everything makes it > clear what's static and what isn't as opposed to hunting for the anonymous > namespace opening/closing braces. > > At this point it's more about keeping code consistent. > > The one place where it all breaks down is defining classes inside of .cc > files. > Sigh. > > https://codereview.chromium.org/23723009/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
