Split the Media settings UI into separate microphone and camera sections.

Split the Media settings UI into separate microphone and camera sections.

This is a rather straightforward UI change. What is left to do in a follow-up CL is to show a separate Flash link for the microphone (camera) sections if only the microphone (camera) settings differ from Flash. In the current implementation, we show a link if either of them differs from Flash, and we show it in both sections.

BUG=509962
Committed: https://crrev.com/0d64db588db45a8ea2178122182d22fbedf3f64a
Cr-Commit-Position: refs/heads/master@{#340934}

[msramek, 2015-07-16 17:04:09]

Patchset #1 (id:1) has been deleted

[msramek, 2015-07-16 17:04:15]

Patchset #1 (id:20001) has been deleted

[msramek, 2015-07-16 17:27:48]

msramek@chromium.org changed reviewers:
+ bauerb@chromium.org, raymes@chromium.org

[msramek, 2015-07-16 17:27:48]

msramek@chromium.org changed required reviewers:
+ bauerb@chromium.org

[msramek, 2015-07-16 17:27:49]

Hi Bernhard and Raymes,

could you please have a look at this?

I'm splitting the media settings UI into camera and microphone sections. Despite
this being a visual change, I don't think it should be done behind a flag, as
removing MEDIASTREAM entails a lot of refactoring in ContentSettingsHandler, and
I don't want to keep both the refactored and non-refactored code at the same
time. I hope you agree :) If not, it can be done with a proper flag as well, it
will just make ContentSettingsHandler quite a mess.

Some things might change yet, but I think it makes sense to start a preliminary
code review already.

Thanks!
Martin

[msramek, 2015-07-16 20:02:57]

https://codereview.chromium.org/1210173012/diff/60001/chrome/test/data/policy...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/1210173012/diff/60001/chrome/test/data/policy...
chrome/test/data/policy/policy_test_cases.json:1083:
"DefaultMediaStreamSetting.OverriddenByAudioCaptureAllowed": {
The DefaultMediaStreamSetting policy is deprecated, but I'm not sure if it's
left around for legacy reasons... I'll find out.

Nevertheless, content_settings::PolicyProvider currently handles it as the same
value applied to both camera and mic, so in the UI this works out of the box.

I added a test for whether disabling audio/video overrides potential
DefaultMediaStreamSetting=ASK (it should).

[raymes, 2015-07-17 00:39:40]

lg but defer to Bernhard who may know the code better than me :)

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.h (right):

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.h:101: // Compares the
media default settings with flash and updates the flash links
nit: links -> link's

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.h:126: // exceptions
and updates the flash links visibility accordingly.
link -> link's

[Bernhard Bauer, 2015-07-17 08:37:12]

LGTM with nits:

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/resource...
File chrome/browser/resources/options/content_settings.html (right):

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/content_settings.html:398: <h3
i18n-content="mediaStreamMicTabLabel">Microphone</h3>
Remove the contents here.

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/content_settings.html:439: <h3
i18n-content="mediaStreamCameraTabLabel">Camera</h3>
Remove the contents here.

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/resource...
File chrome/browser/resources/options/content_settings.js (right):

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/content_settings.js:24: 'location': 'location',
Could you sort this list alphabetically?

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:655: // camera if
camera settings differ, and analogically for microphone.
Nit: "analogously"

[msramek, 2015-07-17 11:59:50]

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/resource...
File chrome/browser/resources/options/content_settings.html (right):

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/content_settings.html:398: <h3
i18n-content="mediaStreamMicTabLabel">Microphone</h3>
On 2015/07/17 08:37:11, Bernhard Bauer wrote:
> Remove the contents here.

Done.

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/content_settings.html:439: <h3
i18n-content="mediaStreamCameraTabLabel">Camera</h3>
On 2015/07/17 08:37:11, Bernhard Bauer wrote:
> Remove the contents here.

Done.

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/resource...
File chrome/browser/resources/options/content_settings.js (right):

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/content_settings.js:24: 'location': 'location',
On 2015/07/17 08:37:11, Bernhard Bauer wrote:
> Could you sort this list alphabetically?

Done.

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:655: // camera if
camera settings differ, and analogically for microphone.
On 2015/07/17 08:37:11, Bernhard Bauer wrote:
> Nit: "analogously"

I wrote "analogously", then I checked a dictionary and I would have sworn that
"analogically" made more sense. It doesn't, but... yesterday it somehow did :)

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.h (right):

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.h:101: // Compares the
media default settings with flash and updates the flash links
On 2015/07/17 00:39:40, raymes wrote:
> nit: links -> link's

Actually, there are now two of them (one for mic, one for camera).

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.h:126: // exceptions
and updates the flash links visibility accordingly.
On 2015/07/17 00:39:40, raymes wrote:
> link -> link's

Ditto.

[Bernhard Bauer, 2015-07-17 13:29:01]

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.h (right):

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.h:101: // Compares the
media default settings with flash and updates the flash links
On 2015/07/17 11:59:50, msramek wrote:
> On 2015/07/17 00:39:40, raymes wrote:
> > nit: links -> link's
> 
> Actually, there are now two of them (one for mic, one for camera).

links' then :)

(Or just "visibility of the links")

[msramek, 2015-07-17 14:01:24]

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.h (right):

https://codereview.chromium.org/1210173012/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.h:101: // Compares the
media default settings with flash and updates the flash links
On 2015/07/17 13:29:01, Bernhard Bauer wrote:
> On 2015/07/17 11:59:50, msramek wrote:
> > On 2015/07/17 00:39:40, raymes wrote:
> > > nit: links -> link's
> > 
> > Actually, there are now two of them (one for mic, one for camera).
> 
> links' then :)
> 
> (Or just "visibility of the links")

Yep, I already have "links'" :)

[msramek, 2015-07-23 18:59:28]

I discussed the UI with rolfe@ and incorporated her requests.

https://codereview.chromium.org/1210173012/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/1210173012/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8187: -      <message
name="IDS_MEDIA_STREAM_TAB_LABEL" desc="Label for media device tab on Content
Settings dialog">
I reverted IDS_MEDIA_SELECTED_CAMERA_LABEL and IDS_MEDIA_SELECTED_MIC_LABEL from
"Select: " back to "Camera: ", "Microphone: ".

This CL no longer uses labels for comboboxes, but as I learned in the meantime,
ContentSettingBubbleModel does.

https://codereview.chromium.org/1210173012/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/options/content_settings.css (right):

https://codereview.chromium.org/1210173012/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/content_settings.css:87:
select.media-device-control:empty {
UI request: Hide device selection comboboxes if they're empty.

https://codereview.chromium.org/1210173012/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/options/content_settings.html (right):

https://codereview.chromium.org/1210173012/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/content_settings.html:400: <select
id="media-select-mic" class="weakrtl media-device-control"
UI request: Remove the device selection labels.

[msramek, 2015-07-29 14:12:00]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-07-29 14:12:00]

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

[commit-bot: I haz the power, 2015-07-29 14:12:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1210173012/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1210173012/120001

[commit-bot: I haz the power, 2015-07-29 14:50:03]

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

[commit-bot: I haz the power, 2015-07-29 14:50:04]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[msramek, 2015-07-29 15:25:09]

Ah, forgot to test the patch with UI updates. After removing the labels from
comboboxes, I forgot to remove the "aria-labelledby" attributes pointing to
them. And we have tests for that :)

Trying again now.

[msramek, 2015-07-29 15:25:12]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-07-29 15:25:13]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1210173012/#ps140001
(title: "Removed the reference to non-existent label.")

[commit-bot: I haz the power, 2015-07-29 15:25:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1210173012/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1210173012/140001

[commit-bot: I haz the power, 2015-07-29 16:56:02]

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

[commit-bot: I haz the power, 2015-07-29 16:56:03]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msramek, 2015-07-29 17:33:51]

On 2015/07/29 15:25:09, msramek wrote:
> Ah, forgot to test the patch with UI updates. After removing the labels from
> comboboxes, I forgot to remove the "aria-labelledby" attributes pointing to
> them. And we have tests for that :)
> 
> Trying again now.

And this was not the correct solution. For accessibility reasons, every select
must have a label. Therefore, I reintroduced the labels removed in Patchset 4,
but with the "hidden" attribute. They are not visible, as per UI specification,
but for visually impaired users, the label is still accessible by a screen
reader.

I will check with the accessibility folks whether this solution is OK, and
possibly patch it or revert it to the original state in a follow-up CL. In the
meantime, another attempt at landing.

[msramek, 2015-07-29 17:33:58]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-07-29 17:33:59]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1210173012/#ps160001
(title: "Reintroduced the label, but as hidden.")

[commit-bot: I haz the power, 2015-07-29 17:35:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1210173012/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1210173012/160001

[commit-bot: I haz the power, 2015-07-29 18:53:35]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-07-29 18:54:14]

Patchset 7 (id:??) landed as
https://crrev.com/0d64db588db45a8ea2178122182d22fbedf3f64a
Cr-Commit-Position: refs/heads/master@{#340934}