Expose microphone content setting to extensions.

Expose microphone content setting to extensions.

BUG=452828
Committed: https://crrev.com/1069b7c12046e5e4bd6b607b4543db3c5cfab036
Cr-Commit-Position: refs/heads/master@{#314306}

[msramek, 2015-01-28 10:55:38]

Patchset #1 (id:1) has been deleted

[msramek, 2015-01-30 11:57:01]

msramek@chromium.org changed reviewers:
+ kalman@chromium.org, mkwst@chromium.org

[msramek, 2015-01-30 11:57:02]

Hi,

here comes another simple addition towards crbug.com/447978. Please take a look.

Thanks,
Martin

https://codereview.chromium.org/880223002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/content_settings.json (right):

https://codereview.chromium.org/880223002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/content_settings.json:240: "mediaStreamMic": {
I kept the internal name here, just renamed from dash-separated to camelCase to
fit the JS style.

However, the "media stream" prefix is a result of the fact that microphone and
camera used to be a single content setting "media stream", which is now
deprecated. Do we want to keep the name for consistency, or rename it to e.g.
"microphone" (as extension developers needn't care about the original deprecated
setting)?

[Mike West, 2015-01-30 12:05:58]

mkwst@chromium.org changed required reviewers:
+ kalman@chromium.org

[Mike West, 2015-01-30 12:05:59]

LGTM

https://codereview.chromium.org/880223002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/content_settings.json (right):

https://codereview.chromium.org/880223002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/content_settings.json:240: "mediaStreamMic": {
On 2015/01/30 11:57:02, msramek wrote:
> I kept the internal name here, just renamed from dash-separated to camelCase
to
> fit the JS style.
> 
> However, the "media stream" prefix is a result of the fact that microphone and
> camera used to be a single content setting "media stream", which is now
> deprecated. Do we want to keep the name for consistency, or rename it to e.g.
> "microphone" (as extension developers needn't care about the original
deprecated
> setting)?

Renaming for the JS interface seems pretty reasonable.

[not at google - send to devlin, 2015-01-30 20:11:52]

https://codereview.chromium.org/880223002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/content_settings/content_settings_helpers.cc
(right):

https://codereview.chromium.org/880223002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/content_settings/content_settings_helpers.cc:31:
"register-protocol-handler",
why are there 3 changes here (in particular RPH seems unrelated to media)

[Mike West, 2015-01-31 09:51:59]

https://codereview.chromium.org/880223002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/content_settings/content_settings_helpers.cc
(right):

https://codereview.chromium.org/880223002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/content_settings/content_settings_helpers.cc:31:
"register-protocol-handler",
On 2015/01/30 20:11:52, kalman wrote:
> why are there 3 changes here (in particular RPH seems unrelated to media)

This array needs to match the ContentSettingsType enum
(https://code.google.com/p/chromium/codesearch#chromium/src/components/content...).
I'd much prefer for us to be a bit cleverer about this (assert sameness at a
minimum (see the TODO below (and note that the enum is in a component, so direct
assertion is right out for the moment (but, really, so is the cast below..
hooray for layering violations!)))), but for the moment, this is how things
work.

[not at google - send to devlin, 2015-02-02 17:52:57]

https://codereview.chromium.org/880223002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/content_settings/content_settings_helpers.cc
(right):

https://codereview.chromium.org/880223002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/content_settings/content_settings_helpers.cc:31:
"register-protocol-handler",
On 2015/01/31 09:51:59, Mike West wrote:
> On 2015/01/30 20:11:52, kalman wrote:
> > why are there 3 changes here (in particular RPH seems unrelated to media)
> 
> This array needs to match the ContentSettingsType enum
>
(https://code.google.com/p/chromium/codesearch#chromium/src/components/content...).
> I'd much prefer for us to be a bit cleverer about this (assert sameness at a
> minimum (see the TODO below (and note that the enum is in a component, so
direct
> assertion is right out for the moment (but, really, so is the cast below..
> hooray for layering violations!)))), but for the moment, this is how things
> work.

Okie I'm a little confused, it doesn't match the enum though? RPH comes after
the media stream values from what I can tell.

[msramek, 2015-02-02 18:50:52]

https://codereview.chromium.org/880223002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/content_settings/content_settings_helpers.cc
(right):

https://codereview.chromium.org/880223002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/content_settings/content_settings_helpers.cc:31:
"register-protocol-handler",
On 2015/02/02 17:52:57, kalman wrote:
> On 2015/01/31 09:51:59, Mike West wrote:
> > On 2015/01/30 20:11:52, kalman wrote:
> > > why are there 3 changes here (in particular RPH seems unrelated to media)
> > 
> > This array needs to match the ContentSettingsType enum
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/components/content...).
> > I'd much prefer for us to be a bit cleverer about this (assert sameness at a
> > minimum (see the TODO below (and note that the enum is in a component, so
> direct
> > assertion is right out for the moment (but, really, so is the cast below..
> > hooray for layering violations!)))), but for the moment, this is how things
> > work.
> 
> Okie I'm a little confused, it doesn't match the enum though? RPH comes after
> the media stream values from what I can tell.

Oh! Thanks, good catch :-) (I would notice it when I got to RPH, but your
confusion prevented my future confusion :-) ).

I didn't want to just invent these strings, so I followed the mapping in
|kContentSettingsTypeGroupNames| and wrote these strings in the same order as
there. The order is ALMOST the same as in |ContentSettingsType|, except one
difference that I didn't notice. The 11th entry is

MIXED_SCRIPT in |ContentSettingsType|
PROTOCOL_HANDLERS in |kContentSettingsTypeGroupNames|

And since this 11th entry was unused so far (it's the only one of them not yet
occurring in content_settings.json), it didn't matter what is its value and
tests passed anyway.

So I'm removing the reference to |kContentSettingsTypeGroupNames| from the TODO,
since that was a misstep. For consistency, it would be nice to synchronize these
strings with |kTypeNames| in content_settings_utils.cc, but it's not necessary
for this to work.

[not at google - send to devlin, 2015-02-02 19:12:40]

lgtm

[msramek, 2015-02-03 09:03:54]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2015-02-03 09:04:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/880223002/80001

[commit-bot: I haz the power, 2015-02-03 10:26:34]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-02-03 10:28:42]

Patchset 4 (id:??) landed as
https://crrev.com/1069b7c12046e5e4bd6b607b4543db3c5cfab036
Cr-Commit-Position: refs/heads/master@{#314306}

[msramek, 2015-02-18 11:24:03]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/925303003/ by msramek@chromium.org.

The reason for reverting is: Setting the camera and microphone default content
settings currently cannot be shown in the UI and will not work exactly the way
an app developer might expect it to (due to interference with the MEDIASTREAM
content setting).

Reverting this patch until the UI is ready.
.