Split Camera and Mic into two permissions on the Site Details page (under Site Settings).

Split Camera and Mic into two permissions on the Site Details page (under Site Settings).

BUG=477678
Committed: https://crrev.com/b2280c4e66b154c89b918befba3b07a1b3e84ec3
Cr-Commit-Position: refs/heads/master@{#327032}

[Finnur, 2015-04-16 14:57:40]

finnur@chromium.org changed reviewers:
+ newt@chromium.org

[newt (away), 2015-04-16 17:11:20]

This looks good as a first step towards splitting these permissions apart, but
I'm confused if this is the *only* step, because the permissions are still
grouped together as a single site settings page. What's the plan there?

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/xml/single_website_preferences.xml (right):

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/res...
chrome/android/java/res/xml/single_website_preferences.xml:28:
android:key="video_capture_permission_list" />
nit: let's use consistent naming for these permissions, e.g. "mic" and "camera",
not "voice" and "video"

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java
(right):

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:31:
return R.drawable.permission_media;
rename "permission_media.png" to "permission_camera.png"

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:89:
return R.string.video_permission_title;
consistent naming, as above

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:347:
return INVALID_CAMERA_OR_MICROPHONE_ACCESS;
as soon as we allow users to toggle these individually, we'll run into
situations where one is "BLOCK" and the other is "ALLOW", in which case this
method will return INVALID_CAMERA_OR_MICROPHONE_ACCESS.

What's the plan here? If we're splitting up camera and mic into two separate
permissions, seems like we should do it fully: i.e. split the "camera or
microphone" site settings page into two pages.

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java
(right):

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:205:
return website.site().getVoiceCapturePermission() != ContentSetting.ALLOW
Why this change?

[Finnur, 2015-04-22 16:01:27]

> What's the plan there?

Something like this?

[Finnur, 2015-04-22 16:01:38]

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/xml/single_website_preferences.xml (right):

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/res...
chrome/android/java/res/xml/single_website_preferences.xml:28:
android:key="video_capture_permission_list" />
Yeah, I agree. In addition to being consistent, I have -- on a few of occasion
-- while cleaning this up confused the two words because they start with the
same letter (voice and video). Using Camera and Mic should prevent such mistakes
in the future.

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java
(right):

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:31:
return R.drawable.permission_media;
On 2015/04/16 17:11:20, newt wrote:
> rename "permission_media.png" to "permission_camera.png"

Done.

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:89:
return R.string.video_permission_title;
On 2015/04/16 17:11:20, newt wrote:
> consistent naming, as above

Done.

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/1084423002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:347:
return INVALID_CAMERA_OR_MICROPHONE_ACCESS;
No, lines 335 through 346 handle the case where only one permission is set and
we just report the one that is allowed.
Line 347 is just a catch-all if neither is set.

But thinking about this some more, now that we've fully separated the categories
there's no need for this function or the icon we show in the WebsitePreferences
page. We've removed all other explanatory icons and it is time to do the same
here.

[newt (away), 2015-04-22 17:35:55]

looks good. all small comments.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/xml/single_website_preferences.xml (right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/res...
chrome/android/java/res/xml/single_website_preferences.xml:28:
android:key="video_capture_permission_list" />
"camera_permission_list", similarly below

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/CameraCaptureInfo.java
(right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/CameraCaptureInfo.java:8:
* TODOf doc
TODO

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/CameraCaptureInfo.java:10:
public class CameraCaptureInfo extends PermissionInfo {
for consistency with other permissions, I'd just call this "CameraInfo". you're
not use "camera capture" anywhere else it seems.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/CameraCaptureInfo.java:16:
return WebsitePreferenceBridge.nativeGetVideoCaptureSettingForOrigin(
s/VideoCapture/Camera

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/MicrophoneCaptureInfo.java
(right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/MicrophoneCaptureInfo.java:8:
* TODOf doc
TODO

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/MicrophoneCaptureInfo.java:10:
public class MicrophoneCaptureInfo extends PermissionInfo {
"MicrophoneInfo"?

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/MicrophoneCaptureInfo.java:16:
return WebsitePreferenceBridge.nativeGetVoiceCaptureSettingForOrigin(
s/VoiceCapture/Microphone

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:55:
public static final String PREF_CAMERA_CAPTURE_PERMISSION =
"video_capture_permission_list";
s/video/camera, likewise for "voice"

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:318:
public void setCameraCaptureInfo(CameraCaptureInfo info) {
"setCameraInfo", and similarly for related methods

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:367:
* Configure video capture setting for this site.
s/video/camera

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java
(right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:206:
return website.site().getCameraCapturePermission() != ContentSetting.BLOCK;
shouldn't this be "==" instead of "!=" ? same below.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:594:
} else if (mFilter.showMicrophoneSites(mCategoryFilter)) {
don't you need to add an if condition for "camera"?

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:508: <message
name="IDS_WEBSITE_SETTINGS_CATEGORY_ASK_BEFORE_ACCESSING_CAMERA_MIC"
desc="Summary text explaining that permission will be requested before accessing
the camera and microphone.">
we can delete this string, right?

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:568: <message
name="IDS_WEBSITE_SETTINGS_USE_CAMERA_OR_MIC" desc="The category filter
'Camera/Mic' for content settings.">
delete this?

https://codereview.chromium.org/1084423002/diff/60001/chrome/browser/android/...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:283: static void
GetVoiceAndVideoCaptureOrigins(JNIEnv* env,
delete this?

[Finnur, 2015-04-24 13:52:00]

All addressed, I believe (and some additional cleanup).

PTAL.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/xml/single_website_preferences.xml (right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/res...
chrome/android/java/res/xml/single_website_preferences.xml:28:
android:key="video_capture_permission_list" />
On 2015/04/22 17:35:54, newt wrote:
> "camera_permission_list", similarly below

Done.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/CameraCaptureInfo.java
(right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/CameraCaptureInfo.java:8:
* TODOf doc
On 2015/04/22 17:35:54, newt wrote:
> TODO

Done.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/CameraCaptureInfo.java:10:
public class CameraCaptureInfo extends PermissionInfo {
On 2015/04/22 17:35:54, newt wrote:
> for consistency with other permissions, I'd just call this "CameraInfo".
you're
> not use "camera capture" anywhere else it seems.

Done.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/CameraCaptureInfo.java:16:
return WebsitePreferenceBridge.nativeGetVideoCaptureSettingForOrigin(
On 2015/04/22 17:35:54, newt wrote:
> s/VideoCapture/Camera

Done.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/MicrophoneCaptureInfo.java
(right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/MicrophoneCaptureInfo.java:8:
* TODOf doc
On 2015/04/22 17:35:55, newt wrote:
> TODO

Done.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/MicrophoneCaptureInfo.java:10:
public class MicrophoneCaptureInfo extends PermissionInfo {
On 2015/04/22 17:35:55, newt wrote:
> "MicrophoneInfo"?

Done.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/MicrophoneCaptureInfo.java:16:
return WebsitePreferenceBridge.nativeGetVoiceCaptureSettingForOrigin(
On 2015/04/22 17:35:55, newt wrote:
> s/VoiceCapture/Microphone

Done.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:55:
public static final String PREF_CAMERA_CAPTURE_PERMISSION =
"video_capture_permission_list";
On 2015/04/22 17:35:55, newt wrote:
> s/video/camera, likewise for "voice"

Done.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:318:
public void setCameraCaptureInfo(CameraCaptureInfo info) {
On 2015/04/22 17:35:55, newt wrote:
> "setCameraInfo", and similarly for related methods

Done.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:367:
* Configure video capture setting for this site.
On 2015/04/22 17:35:55, newt wrote:
> s/video/camera

Done.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java
(right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:206:
return website.site().getCameraCapturePermission() != ContentSetting.BLOCK;
Woops. Yes. :)

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:594:
} else if (mFilter.showMicrophoneSites(mCategoryFilter)) {
Good catch!

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:508: <message
name="IDS_WEBSITE_SETTINGS_CATEGORY_ASK_BEFORE_ACCESSING_CAMERA_MIC"
desc="Summary text explaining that permission will be requested before accessing
the camera and microphone.">
On 2015/04/22 17:35:55, newt wrote:
> we can delete this string, right?

Done.

https://codereview.chromium.org/1084423002/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:568: <message
name="IDS_WEBSITE_SETTINGS_USE_CAMERA_OR_MIC" desc="The category filter
'Camera/Mic' for content settings.">
On 2015/04/22 17:35:55, newt wrote:
> delete this?

Done.

https://codereview.chromium.org/1084423002/diff/60001/chrome/browser/android/...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/1084423002/diff/60001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:283: static void
GetVoiceAndVideoCaptureOrigins(JNIEnv* env,
On 2015/04/22 17:35:55, newt wrote:
> delete this?

Done.

[newt (away), 2015-04-24 16:52:29]

lgtm

So, what is the infobar situation if the user visits a page that requests both
camera and microphone? Do we show one or two infobars?

https://codereview.chromium.org/1084423002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/1084423002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:199:
if (merged.getCameraInfo() == null
looks like this would fit on one line. same below.

https://codereview.chromium.org/1084423002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ContentPreferencesTest.java
(right):

https://codereview.chromium.org/1084423002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/ContentPreferencesTest.java:426:
// Launch a page that use geolocation and make sure an infobar shows up.
"geolocation"?

[Finnur, 2015-04-24 19:45:26]

On 2015/04/24 16:52:29, newt wrote:
> lgtm
> 
> So, what is the infobar situation if the user visits a page that requests both
> camera and microphone? Do we show one or two infobars?

Sadly, the infobar scenario for Camera and/or Mic is broken, regardless of this
CL. :/
You can follow along here: http://crbug.com/477652

Thanks for the LGTM. On Monday, I'll address the comments you made before
checking in.

[Finnur, 2015-04-27 12:45:44]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2015-04-27 12:45:44]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1084423002/#ps160001
(title: "Fix test")

[commit-bot: I haz the power, 2015-04-27 12:45:49]

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

[commit-bot: I haz the power, 2015-04-27 13:09:11]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-04-27 13:10:14]

Patchset 9 (id:??) landed as
https://crrev.com/b2280c4e66b154c89b918befba3b07a1b3e84ec3
Cr-Commit-Position: refs/heads/master@{#327032}