Audio extension API.

Audio extension API.

Extension API to query and control audio device state.
The design doc is available at

https://docs.google.com/a/google.com/document/d/1szQEkGggTScHP8Ahum8ZrN7QxmHaCsF83XTq2AcsU-g/

This CL is not yet complete, but is compiling and working for
the portion that is finished. It contains most of the stubs
and implements the audio.onDeviceChanged event as well as
part of the audio.getInfo. I would like to land this to trunk
ASAP and hand over the work to Rahul.

BUG=175798
TEST=Trybots, local test app exercising audio API.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195743

[hshi1, 2013-04-22 21:04:34]

PTAL (starting with Rahul... will add OWNERs later).

[rkc, 2013-04-22 22:54:19]

LGTM with nits.

https://codereview.chromium.org/13486004/diff/29020/chrome/browser/extensions...
File chrome/browser/extensions/api/audio/audio_api.cc (right):

https://codereview.chromium.org/13486004/diff/29020/chrome/browser/extensions...
chrome/browser/extensions/api/audio/audio_api.cc:69: return false;
Add TODO here and below.

https://codereview.chromium.org/13486004/diff/29020/chrome/browser/extensions...
File chrome/browser/extensions/api/audio/audio_service.h (right):

https://codereview.chromium.org/13486004/diff/29020/chrome/browser/extensions...
chrome/browser/extensions/api/audio/audio_service.h:42: // The |callbac| will be
invoked once the query is completed.
|callback|

https://codereview.chromium.org/13486004/diff/29020/chrome/browser/extensions...
File chrome/browser/extensions/api/audio/audio_service_chromeos.cc (right):

https://codereview.chromium.org/13486004/diff/29020/chrome/browser/extensions...
chrome/browser/extensions/api/audio/audio_service_chromeos.cc:105: i !=
audio_nodes.end(); i++) {
s/i/iter
s/i++/++iter

https://codereview.chromium.org/13486004/diff/29020/chrome/chrome_browser_ext...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/13486004/diff/29020/chrome/chrome_browser_ext...
chrome/chrome_browser_extensions.gypi:756:
'browser/extensions/api/audio/audio_service_generic.cc',
Why not just call it audio_service.cc? The generic would be implied.

https://codereview.chromium.org/13486004/diff/29020/chrome/test/DEPS
File chrome/test/DEPS (right):

https://codereview.chromium.org/13486004/diff/29020/chrome/test/DEPS#newcode15
chrome/test/DEPS:15: "+chromeos",
You probably want to make this more specific, maybe +chromeos/dbus. The DEPS
owner will be able to comment more.

[hshi1, 2013-04-22 23:02:16]

https://codereview.chromium.org/13486004/diff/29020/chrome/browser/extensions...
File chrome/browser/extensions/api/audio/audio_api.cc (right):

https://codereview.chromium.org/13486004/diff/29020/chrome/browser/extensions...
chrome/browser/extensions/api/audio/audio_api.cc:69: return false;
On 2013/04/22 22:54:19, Rahul Chaturvedi wrote:
> Add TODO here and below.

Done.

https://codereview.chromium.org/13486004/diff/29020/chrome/browser/extensions...
File chrome/browser/extensions/api/audio/audio_service.h (right):

https://codereview.chromium.org/13486004/diff/29020/chrome/browser/extensions...
chrome/browser/extensions/api/audio/audio_service.h:42: // The |callbac| will be
invoked once the query is completed.
On 2013/04/22 22:54:19, Rahul Chaturvedi wrote:
> |callback|

Done.

https://codereview.chromium.org/13486004/diff/29020/chrome/browser/extensions...
File chrome/browser/extensions/api/audio/audio_service_chromeos.cc (right):

https://codereview.chromium.org/13486004/diff/29020/chrome/browser/extensions...
chrome/browser/extensions/api/audio/audio_service_chromeos.cc:105: i !=
audio_nodes.end(); i++) {
On 2013/04/22 22:54:19, Rahul Chaturvedi wrote:
> s/i/iter
> s/i++/++iter
> 

Done.

https://codereview.chromium.org/13486004/diff/29020/chrome/chrome_browser_ext...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/13486004/diff/29020/chrome/chrome_browser_ext...
chrome/chrome_browser_extensions.gypi:756:
'browser/extensions/api/audio/audio_service_generic.cc',
On 2013/04/22 22:54:19, Rahul Chaturvedi wrote:
> Why not just call it audio_service.cc? The generic would be implied.

Done.

https://codereview.chromium.org/13486004/diff/29020/chrome/test/DEPS
File chrome/test/DEPS (right):

https://codereview.chromium.org/13486004/diff/29020/chrome/test/DEPS#newcode15
chrome/test/DEPS:15: "+chromeos",
On 2013/04/22 22:54:19, Rahul Chaturvedi wrote:
> You probably want to make this more specific, maybe +chromeos/dbus. The DEPS
> owner will be able to comment more.
> 

Done.

[hshi1, 2013-04-22 23:28:41]

+mpcomplete@ (OWNER of browser/extensions/ and common/extensions)
+phajdan@ (OWNER of chrome/test)

Please take a look. Note (I have mentioned this earlier) that this CL is
self-contained but not yet functionally complete with regards to the Audio API.
My intention is to land this and hand over the work to Rahul who will complete
the remaining TODO items in a separate CL.

[Matt Perry, 2013-04-22 23:57:26]

mostly LGTM

https://codereview.chromium.org/13486004/diff/91005/chrome/browser/extensions...
File chrome/browser/extensions/api/audio/audio_api.cc (right):

https://codereview.chromium.org/13486004/diff/91005/chrome/browser/extensions...
chrome/browser/extensions/api/audio/audio_api.cc:41:
scoped_ptr<extensions::Event> event(new extensions::Event(
No need for extensions:: prefix. We're in the extensions namespace here.

https://codereview.chromium.org/13486004/diff/91005/chrome/common/extensions/...
File chrome/common/extensions/api/audio.idl (right):

https://codereview.chromium.org/13486004/diff/91005/chrome/common/extensions/...
chrome/common/extensions/api/audio.idl:65: static void onDeviceChanged();
This should pass down the same data as the getInfo callback. The app/extension
will likely need the new device data anyway.

[hshi1, 2013-04-23 00:07:34]

https://codereview.chromium.org/13486004/diff/91005/chrome/browser/extensions...
File chrome/browser/extensions/api/audio/audio_api.cc (right):

https://codereview.chromium.org/13486004/diff/91005/chrome/browser/extensions...
chrome/browser/extensions/api/audio/audio_api.cc:41:
scoped_ptr<extensions::Event> event(new extensions::Event(
On 2013/04/22 23:57:26, Matt Perry wrote:
> No need for extensions:: prefix. We're in the extensions namespace here.

Done.

https://codereview.chromium.org/13486004/diff/91005/chrome/common/extensions/...
File chrome/common/extensions/api/audio.idl (right):

https://codereview.chromium.org/13486004/diff/91005/chrome/common/extensions/...
chrome/common/extensions/api/audio.idl:65: static void onDeviceChanged();
On 2013/04/22 23:57:26, Matt Perry wrote:
> This should pass down the same data as the getInfo callback. The app/extension
> will likely need the new device data anyway.

This event is defined this way because it is not trivial (takes two extra async
calls through dbus::CrasAudioClient) to collect the list of audio device nodes
as well as the volume state. I would like to keep this simple so that we do not
duplicate too much code in the C++ layer.

[hshi1, 2013-04-23 00:10:21]

+thakis (missing OWNER reviewer for: chrome/chrome_browser_extensions.gypi and
chrome/browser/profiles/profile_dependency_manager.cc)

[Matt Perry, 2013-04-23 00:12:54]

https://codereview.chromium.org/13486004/diff/91005/chrome/common/extensions/...
File chrome/common/extensions/api/audio.idl (right):

https://codereview.chromium.org/13486004/diff/91005/chrome/common/extensions/...
chrome/common/extensions/api/audio.idl:65: static void onDeviceChanged();
On 2013/04/23 00:07:34, hshi1 wrote:
> On 2013/04/22 23:57:26, Matt Perry wrote:
> > This should pass down the same data as the getInfo callback. The
app/extension
> > will likely need the new device data anyway.
> 
> This event is defined this way because it is not trivial (takes two extra
async
> calls through dbus::CrasAudioClient) to collect the list of audio device nodes
> as well as the volume state. I would like to keep this simple so that we do
not
> duplicate too much code in the C++ layer.

Maybe I don't understand how onDeviceChanged will be used. But it seems likely
to me that extensions will almost always need to just call audio.getInfo() to
query what has changed. If that's the case, I'd rather complicate things at the
C++ layer to make the API easier to use for developers.

[hshi1, 2013-04-23 00:22:30]

On 2013/04/23 00:12:54, Matt Perry wrote:
>
https://codereview.chromium.org/13486004/diff/91005/chrome/common/extensions/...
> File chrome/common/extensions/api/audio.idl (right):
> 
>
https://codereview.chromium.org/13486004/diff/91005/chrome/common/extensions/...
> chrome/common/extensions/api/audio.idl:65: static void onDeviceChanged();
> On 2013/04/23 00:07:34, hshi1 wrote:
> > On 2013/04/22 23:57:26, Matt Perry wrote:
> > > This should pass down the same data as the getInfo callback. The
> app/extension
> > > will likely need the new device data anyway.
> > 
> > This event is defined this way because it is not trivial (takes two extra
> async
> > calls through dbus::CrasAudioClient) to collect the list of audio device
nodes
> > as well as the volume state. I would like to keep this simple so that we do
> not
> > duplicate too much code in the C++ layer.
> 
> Maybe I don't understand how onDeviceChanged will be used. But it seems likely
> to me that extensions will almost always need to just call audio.getInfo() to
> query what has changed. If that's the case, I'd rather complicate things at
the
> C++ layer to make the API easier to use for developers.

This sounds reasonable. Would you mind if I add a TODO here? (Or just change the
.idl like you suggested). As I mentioned, the GetInfo portion is not
functionally complete yet, I intend to get this landed so that Rahul can pick up
this task and complete wiring the remaining functions (I'll be working on
something else unfortunately).

[Matt Perry, 2013-04-23 00:27:47]

On 2013/04/23 00:22:30, hshi1 wrote:
> On 2013/04/23 00:12:54, Matt Perry wrote:
> >
>
https://codereview.chromium.org/13486004/diff/91005/chrome/common/extensions/...
> > File chrome/common/extensions/api/audio.idl (right):
> > 
> >
>
https://codereview.chromium.org/13486004/diff/91005/chrome/common/extensions/...
> > chrome/common/extensions/api/audio.idl:65: static void onDeviceChanged();
> > On 2013/04/23 00:07:34, hshi1 wrote:
> > > On 2013/04/22 23:57:26, Matt Perry wrote:
> > > > This should pass down the same data as the getInfo callback. The
> > app/extension
> > > > will likely need the new device data anyway.
> > > 
> > > This event is defined this way because it is not trivial (takes two extra
> > async
> > > calls through dbus::CrasAudioClient) to collect the list of audio device
> nodes
> > > as well as the volume state. I would like to keep this simple so that we
do
> > not
> > > duplicate too much code in the C++ layer.
> > 
> > Maybe I don't understand how onDeviceChanged will be used. But it seems
likely
> > to me that extensions will almost always need to just call audio.getInfo()
to
> > query what has changed. If that's the case, I'd rather complicate things at
> the
> > C++ layer to make the API easier to use for developers.
> 
> This sounds reasonable. Would you mind if I add a TODO here? (Or just change
the
> .idl like you suggested). As I mentioned, the GetInfo portion is not
> functionally complete yet, I intend to get this landed so that Rahul can pick
up
> this task and complete wiring the remaining functions (I'll be working on
> something else unfortunately).

Sure, a TODO sounds good.

[Nico, 2013-04-23 00:28:17]

My 3 files lgtm with the tweak below.

https://codereview.chromium.org/13486004/diff/89006/chrome/chrome_browser_ext...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/13486004/diff/89006/chrome/chrome_browser_ext...
chrome/chrome_browser_extensions.gypi:758:
'browser/extensions/api/audio/audio_service.cc',
I think it's more common to always include this, and to conditionally exclude it
on cros. I think you can also always include the _chromeos.cc file, it'll be
excluded automatically on non-cros (see build/file_patters.gypi)

[hshi1, 2013-04-23 00:46:55]

https://codereview.chromium.org/13486004/diff/89006/chrome/chrome_browser_ext...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/13486004/diff/89006/chrome/chrome_browser_ext...
chrome/chrome_browser_extensions.gypi:758:
'browser/extensions/api/audio/audio_service.cc',
I tried to conditionally exclude this file on cros before, but it somehow didn't
seem to work. What I did was to add the path of audio_service.cc under
'sources!' (and also tried to list it as a excludes rule under 'sources/'), and
then run ./build/gyp_chromium again before rebuilding, but in both attempts I
still end up linking the audio_service.o and ended up calling the stub functions
there.

I wonder if I did anything wrong. Is it required to clean up the entire out
directory if I make gypi changes like this?

On 2013/04/23 00:28:18, Nico wrote:
> I think it's more common to always include this, and to conditionally exclude
it
> on cros. I think you can also always include the _chromeos.cc file, it'll be
> excluded automatically on non-cros (see build/file_patters.gypi)

[hshi1, 2013-04-23 01:11:17]

On 2013/04/23 00:46:55, hshi1 wrote:
>
https://codereview.chromium.org/13486004/diff/89006/chrome/chrome_browser_ext...
> File chrome/chrome_browser_extensions.gypi (right):
> 
>
https://codereview.chromium.org/13486004/diff/89006/chrome/chrome_browser_ext...
> chrome/chrome_browser_extensions.gypi:758:
> 'browser/extensions/api/audio/audio_service.cc',
> I tried to conditionally exclude this file on cros before, but it somehow
didn't
> seem to work. What I did was to add the path of audio_service.cc under
> 'sources!' (and also tried to list it as a excludes rule under 'sources/'),
and
> then run ./build/gyp_chromium again before rebuilding, but in both attempts I
> still end up linking the audio_service.o and ended up calling the stub
functions
> there.
> 
> I wonder if I did anything wrong. Is it required to clean up the entire out
> directory if I make gypi changes like this?
> 
> On 2013/04/23 00:28:18, Nico wrote:
> > I think it's more common to always include this, and to conditionally
exclude
> it
> > on cros. I think you can also always include the _chromeos.cc file, it'll be
> > excluded automatically on non-cros (see build/file_patters.gypi)

I think there's a bug somewhere with the filename exclusion... I inspected the
generated out/Release/obj/chrome/browser_extensions.ninja and found that the
file audio_service.cc is still included. Not only that, but all the files listed
under the "sources!" section (such as browser/extensions/default_apps.cc) are
also still in the build.

This is really weird, I think I'll file a bug to track this problem.

[commit-bot: I haz the power, 2013-04-23 01:32:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/13486004/101002

[Nico, 2013-04-23 02:08:46]

https://codereview.chromium.org/13486004/diff/89006/chrome/chrome_browser_ext...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/13486004/diff/89006/chrome/chrome_browser_ext...
chrome/chrome_browser_extensions.gypi:758:
'browser/extensions/api/audio/audio_service.cc',
On 2013/04/23 00:46:55, hshi1 wrote:
> I tried to conditionally exclude this file on cros before, but it somehow
didn't
> seem to work. What I did was to add the path of audio_service.cc under
> 'sources!' (and also tried to list it as a excludes rule under 'sources/'),
and
> then run ./build/gyp_chromium again before rebuilding, but in both attempts I
> still end up linking the audio_service.o and ended up calling the stub
functions
> there.
> 
> I wonder if I did anything wrong. Is it required to clean up the entire out
> directory if I make gypi changes like this?
> 
> On 2013/04/23 00:28:18, Nico wrote:
> > I think it's more common to always include this, and to conditionally
exclude
> it
> > on cros. I think you can also always include the _chromeos.cc file, it'll be
> > excluded automatically on non-cros (see build/file_patters.gypi)
> 
> 
> 

No, you don't have to remove the whole out folder. `sources!` should work, or
you could try an `excludes` section (which takes a pattern instead of a literal
string)

[commit-bot: I haz the power, 2013-04-23 02:12:50]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, googleurl_unittests,
media_unittests, net_unittests, sql_unittests, ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[hshi1, 2013-04-23 02:55:46]

On 2013/04/23 02:08:46, Nico wrote:
> No, you don't have to remove the whole out folder. `sources!` should work, or
> you could try an `excludes` section (which takes a pattern instead of a
literal
> string)

I'm afraid something isn't quite right with this particular gypi. No matter what
I do, the exclusion rules would not prevent (1) the generic audio_service.cc
from being compiled and linked, and (2) the chromeos version of the same
AudioServiceImpl class from missing. I even tried to clean up the entire out
directory then do a GYP_DEFINES="chromeos=1 remove_webcore_debug_symbols=1
branding=Chrome buildtype=Official" GYP_GENERATORS="ninja" gclient runhooks, the
generated out/Release/obj/chrome/browser_extensions.ninja still contains the
generic audio_service.cc as a target. I also did an nm on the output chrome
binary and could not find any symbols of the methods unique to the
audio_service_chromeos.cc.

I'll wait a bit for the unit test to pass on the latest patch set and land it
for now.

[commit-bot: I haz the power, 2013-04-23 03:09:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/13486004/100008

[commit-bot: I haz the power, 2013-04-23 03:31:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/13486004/111007

[commit-bot: I haz the power, 2013-04-23 04:01:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/13486004/113001

[commit-bot: I haz the power, 2013-04-23 04:03:27]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-04-23 04:07:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/13486004/116005

[commit-bot: I haz the power, 2013-04-23 06:57:34]

Change committed as 195743

[hshi1, 2013-04-23 16:59:48]

Change is landed with the ugly gypi (with both 'chromeos==0' and 'chromeos==1'
sections) as I could not figure out how to get the exclusion rule to work.

I've filed GYP bug https://code.google.com/p/gyp/issues/detail?id=335