Restrict deprecated parts of audio API usage to whitelisted apps

Restrict deprecated parts of audio API usage to whitelisted apps

Removes two deprecated properties from AudioDeviceInfo, which should
be safe to do at this point, as the object is not currently used by
the apps whitelisted for audio API.

BUG=673392
Review-Url: https://codereview.chromium.org/2688773002
Cr-Commit-Position: refs/heads/master@{#454204}
Committed: https://chromium.googlesource.com/chromium/src/+/7c6ba299357a5780218eb7a7fd00a35b9cbecc96

[tbarzic, 2017-02-10 00:43:09]

Description was changed from

==========
Restrict deprecated parts of audio API usage to whitelisetd apps

BUG=673392
==========

to

==========
Restrict deprecated parts of audio API usage to whitelisted apps

BUG=673392
==========

[tbarzic, 2017-02-15 23:47:05]

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

[commit-bot: I haz the power, 2017-02-15 23:48:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2017-02-16 00:33:19]

Description was changed from

==========
Restrict deprecated parts of audio API usage to whitelisted apps

BUG=673392
==========

to

==========
Restrict deprecated parts of audio API usage to whitelisted apps

Removes two deprecated properties from AudioDeviceInfo, which should
be safe to do at this point, as the object is not currently used by
the apps whitelisted for audio API.

BUG=673392
==========

[tbarzic, 2017-02-16 00:42:55]

tbarzic@chromium.org changed reviewers:
+ jennyz@chromium.org, mnilsson@chromium.org, rdevlin.cronin@chromium.org,
steel@chromium.org

[tbarzic, 2017-02-16 00:42:55]

steel and mnilsson, FYI

https://codereview.chromium.org/2688773002/diff/120001/extensions/common/api/...
File extensions/common/api/audio.idl (left):

https://codereview.chromium.org/2688773002/diff/120001/extensions/common/api/...
extensions/common/api/audio.idl:69: [deprecated="Use |streamType|."] boolean
isInput;
Note that AudioDeviceInfo is not currently in use anywhere, so this should be
safe.

[Devlin, 2017-02-22 00:31:43]

https://codereview.chromium.org/2688773002/diff/120001/extensions/common/api/...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/2688773002/diff/120001/extensions/common/api/...
extensions/common/api/audio.idl:213: // event will not work for apps that are
not part of that white-list.</p>
Why this difference instead of the nodocs we have above?

[tbarzic, 2017-02-22 00:38:37]

https://codereview.chromium.org/2688773002/diff/120001/extensions/common/api/...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/2688773002/diff/120001/extensions/common/api/...
extensions/common/api/audio.idl:213: // event will not work for apps that are
not part of that white-list.</p>
On 2017/02/22 00:31:43, Devlin wrote:
> Why this difference instead of the nodocs we have above?

Main reason is that there will be a clear error message when there is attempt to
use the methods - if the event is used it will fail silently (so it seemed
reasonable to document this).

[Devlin, 2017-02-22 14:13:14]

Do we have a timeline for exactly how long we're going to give the apps to
migrate over?  Should we include it in any of the errors?

https://codereview.chromium.org/2688773002/diff/120001/extensions/browser/api...
File extensions/browser/api/audio/audio_api.cc (right):

https://codereview.chromium.org/2688773002/diff/120001/extensions/browser/api...
extensions/browser/api/audio/audio_api.cc:23: bool
CanUseDeprecatedAudioApi(const Extension* extension) {
Add a TODO to remove this, preferably with a concrete date.

https://codereview.chromium.org/2688773002/diff/120001/extensions/browser/api...
extensions/browser/api/audio/audio_api.cc:133: return RespondNow(Error("Not
allowed."));
Should this be a little more detailed?  E.g. "audio.getInfo is deprecated, use
... instead"

https://codereview.chromium.org/2688773002/diff/120001/extensions/common/api/...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/2688773002/diff/120001/extensions/common/api/...
extensions/common/api/audio.idl:213: // event will not work for apps that are
not part of that white-list.</p>
On 2017/02/22 00:38:37, tbarzic wrote:
> On 2017/02/22 00:31:43, Devlin wrote:
> > Why this difference instead of the nodocs we have above?
> 
> Main reason is that there will be a clear error message when there is attempt
to
> use the methods - if the event is used it will fail silently (so it seemed
> reasonable to document this).

If this is only for whitelisted properties, can we just let them know?  It's
important that we don't brick them, but I don't know if we need to add public
documentation for them.  In-development APIs are meant to change. :)

https://codereview.chromium.org/2688773002/diff/120001/extensions/test/data/a...
File extensions/test/data/api_test/audio/test.js (left):

https://codereview.chromium.org/2688773002/diff/120001/extensions/test/data/a...
extensions/test/data/api_test/audio/test.js:322: function
deprecatedSetActiveDevicesTest() {
These were all just cut-and-pasted, right?

[tbarzic, 2017-02-22 19:35:19]

https://codereview.chromium.org/2688773002/diff/120001/extensions/browser/api...
File extensions/browser/api/audio/audio_api.cc (right):

https://codereview.chromium.org/2688773002/diff/120001/extensions/browser/api...
extensions/browser/api/audio/audio_api.cc:23: bool
CanUseDeprecatedAudioApi(const Extension* extension) {
On 2017/02/22 14:13:14, Devlin wrote:
> Add a TODO to remove this, preferably with a concrete date.

Done.

I've pinged mnilsson about a reasonable time frame for removing this, and will
update the comment with the response.

https://codereview.chromium.org/2688773002/diff/120001/extensions/browser/api...
extensions/browser/api/audio/audio_api.cc:133: return RespondNow(Error("Not
allowed."));
On 2017/02/22 14:13:14, Devlin wrote:
> Should this be a little more detailed?  E.g. "audio.getInfo is deprecated, use
> ... instead"

Done.

https://codereview.chromium.org/2688773002/diff/120001/extensions/common/api/...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/2688773002/diff/120001/extensions/common/api/...
extensions/common/api/audio.idl:213: // event will not work for apps that are
not part of that white-list.</p>
On 2017/02/22 14:13:14, Devlin wrote:
> On 2017/02/22 00:38:37, tbarzic wrote:
> > On 2017/02/22 00:31:43, Devlin wrote:
> > > Why this difference instead of the nodocs we have above?
> > 
> > Main reason is that there will be a clear error message when there is
attempt
> to
> > use the methods - if the event is used it will fail silently (so it seemed
> > reasonable to document this).
> 
> If this is only for whitelisted properties, can we just let them know?  It's
> important that we don't brick them, but I don't know if we need to add public
> documentation for them.  In-development APIs are meant to change. :)

OK, nodocing this.

https://codereview.chromium.org/2688773002/diff/120001/extensions/test/data/a...
File extensions/test/data/api_test/audio/test.js (left):

https://codereview.chromium.org/2688773002/diff/120001/extensions/test/data/a...
extensions/test/data/api_test/audio/test.js:322: function
deprecatedSetActiveDevicesTest() {
On 2017/02/22 14:13:14, Devlin wrote:
> These were all just cut-and-pasted, right?

yes, to audio/deprecated_api/test.js

[Devlin, 2017-02-23 23:17:42]

largely lg, but I'd like a definitive time frame for removing the deprecated
function handling before submitting.

[jennyz, 2017-03-01 01:10:46]

lgtm

[tbarzic, 2017-03-01 17:36:24]

https://codereview.chromium.org/2688773002/diff/120001/extensions/browser/api...
File extensions/browser/api/audio/audio_api.cc (right):

https://codereview.chromium.org/2688773002/diff/120001/extensions/browser/api...
extensions/browser/api/audio/audio_api.cc:23: bool
CanUseDeprecatedAudioApi(const Extension* extension) {
On 2017/02/22 19:35:18, tbarzic wrote:
> On 2017/02/22 14:13:14, Devlin wrote:
> > Add a TODO to remove this, preferably with a concrete date.
> 
> Done.
> 
> I've pinged mnilsson about a reasonable time frame for removing this, and will
> update the comment with the response.

I've added a comment to target removal of this for M-60

[Devlin, 2017-03-02 01:46:22]

lgtm

[tbarzic, 2017-03-02 01:47:46]

The CQ bit was checked by tbarzic@chromium.org

[tbarzic, 2017-03-02 01:47:47]

The patchset sent to the CQ was uploaded after l-g-t-m from jennyz@chromium.org
Link to the patchset: https://codereview.chromium.org/2688773002/#ps180001
(title: ".")

[commit-bot: I haz the power, 2017-03-02 01:48:24]

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-03-02 05:43:57]

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

[commit-bot: I haz the power, 2017-03-02 05:43:58]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tbarzic, 2017-03-02 05:53:33]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2017-03-02 05:54:10]

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-03-02 07:38:20]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1488434013250870,
"parent_rev": "f88569b6a113509e7e7245ca297221701d07d4a3", "commit_rev":
"7c6ba299357a5780218eb7a7fd00a35b9cbecc96"}

[commit-bot: I haz the power, 2017-03-02 07:39:02]

Description was changed from

==========
Restrict deprecated parts of audio API usage to whitelisted apps

Removes two deprecated properties from AudioDeviceInfo, which should
be safe to do at this point, as the object is not currently used by
the apps whitelisted for audio API.

BUG=673392
==========

to

==========
Restrict deprecated parts of audio API usage to whitelisted apps

Removes two deprecated properties from AudioDeviceInfo, which should
be safe to do at this point, as the object is not currently used by
the apps whitelisted for audio API.

BUG=673392

Review-Url: https://codereview.chromium.org/2688773002
Cr-Commit-Position: refs/heads/master@{#454204}
Committed:
https://chromium.googlesource.com/chromium/src/+/7c6ba299357a5780218eb7a7fd00...
==========

[commit-bot: I haz the power, 2017-03-02 07:39:03]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/7c6ba299357a5780218eb7a7fd00...