Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1311783007:
refactor to Introduce AUDIO_CAPTURE and VIDEO_CAPTURE permissions. (Closed)
|
Created:
5 years, 4 months ago by guoweis_left_chromium Modified:
5 years, 3 months ago Reviewers:
rkaplow, mlamouri (slow - plz ping), michaelbai, kinuko, raymes, palmer, tommi (sloooow) - chröme, xhwang CC:
Aaron Boodman, abarth-chromium, android-webview-reviews_chromium.org, asvitkine+watch_chromium.org, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mlamouri+watch-permissions_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, wjia+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce AUDIO_CAPTURE and VIDEO_CAPTURE permissions.
This CL adds the permissions to the Permission Mojo Service and PermissionType enum. It is also adding a PermissionContext to handle them.
There are 2 CLs depending on this one.
https://codereview.chromium.org/1314923007
https://codereview.chromium.org/1318173002
BUG=520101
Committed: https://crrev.com/c409888759da9a9e8bdd1089790dd52f1b681b54
Cr-Commit-Position: refs/heads/master@{#346776}
Patch Set 1 : #
Total comments: 18
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 35
Patch Set 5 : #
Total comments: 10
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : #
Total comments: 14
Patch Set 9 : #Patch Set 10 : #
Total comments: 5
Patch Set 11 : #Patch Set 12 : #
Total comments: 1
Messages
Total messages: 59 (15 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by guoweis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1311783007/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311783007/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
PTAL.
Thanks for splitting this up :) It would be good if we could add a small test for the new PermissionContext which would test GetPermissionStatus. https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... File chrome/browser/media/media_stream_device_permission_context.cc (right): https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.cc:21: bool MediaStreamDevicePermissionContext::IsRestrictedToSecureOrigins() const { Please keep functions in the same order as the header file https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.cc:31: CHECK(0) << "RequestPermission is not implemented"; We would typically use NOTREACHED() here. (and below) https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.cc:52: // TODO(guoweis): why are we using embedding_origin? You tell me: why are we using embedding origin :)? https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.cc:62: DCHECK(policy == POLICY_NOT_SET); DCHECK_EQ https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.cc:66: requesting_origin, embedding_origin, content_settings_type(), This should use requesting_origin, requesting_origin to match the previous behavior. https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... File chrome/browser/media/media_stream_device_permission_context.h (right): https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.h:11: // Common class which handles mic and camera handles the mic and camera permissions. https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.h:12: // permission. MediaStreamMicPermissionContextFactory and nit: fill 80chars
https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... File chrome/browser/media/media_stream_device_permission_context.cc (right): https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.cc:52: // TODO(guoweis): why are we using embedding_origin? On 2015/08/26 at 03:07:36, raymes wrote: > You tell me: why are we using embedding origin :)? If embedder.com has an iframe to website.com and website.com gets a permission, it might be different than website.com getting it while it's the top frame. https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.cc:66: requesting_origin, embedding_origin, content_settings_type(), On 2015/08/26 at 03:07:36, raymes wrote: > This should use requesting_origin, requesting_origin to match the previous behavior. Could you just call PermissionContextBase::GetPermissionStatus() ? https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:121: ContentSettingsType content_settings_type() const; ContentSettingsType is an implementation details of chrome/browser/permissions/. There is an ongoing task which is about removing all association between PermissionType and ContentSetting outside of the internal c/b/permissions code.
PTAL. https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... File chrome/browser/media/media_stream_device_permission_context.cc (right): https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.cc:21: bool MediaStreamDevicePermissionContext::IsRestrictedToSecureOrigins() const { On 2015/08/26 03:07:36, raymes wrote: > Please keep functions in the same order as the header file Done. https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.cc:31: CHECK(0) << "RequestPermission is not implemented"; On 2015/08/26 03:07:36, raymes wrote: > We would typically use NOTREACHED() here. (and below) Done. https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.cc:52: // TODO(guoweis): why are we using embedding_origin? On 2015/08/26 13:49:34, Mounir Lamouri wrote: > On 2015/08/26 at 03:07:36, raymes wrote: > > You tell me: why are we using embedding origin :)? > > If http://embedder.com has an iframe to http://website.com and http://website.com gets a permission, > it might be different than http://website.com getting it while it's the top frame. Done. https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.cc:62: DCHECK(policy == POLICY_NOT_SET); On 2015/08/26 03:07:36, raymes wrote: > DCHECK_EQ Done. https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.cc:66: requesting_origin, embedding_origin, content_settings_type(), On 2015/08/26 13:49:34, Mounir Lamouri wrote: > On 2015/08/26 at 03:07:36, raymes wrote: > > This should use requesting_origin, requesting_origin to match the previous > behavior. > > Could you just call PermissionContextBase::GetPermissionStatus() ? Done. https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... File chrome/browser/media/media_stream_device_permission_context.h (right): https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.h:12: // permission. MediaStreamMicPermissionContextFactory and On 2015/08/26 03:07:36, raymes wrote: > nit: fill 80chars Done. https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:121: ContentSettingsType content_settings_type() const; On 2015/08/26 13:49:34, Mounir Lamouri wrote: > ContentSettingsType is an implementation details of chrome/browser/permissions/. > There is an ongoing task which is about removing all association between > PermissionType and ContentSetting outside of the internal c/b/permissions code. Done.
lgtm with the changes below Thanks! https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... File chrome/browser/media/media_stream_device_permission_context.cc (right): https://codereview.chromium.org/1311783007/diff/80001/chrome/browser/media/me... chrome/browser/media/media_stream_device_permission_context.cc:66: requesting_origin, embedding_origin, content_settings_type(), This will break the current behavior for the media permission, which is to consider only the requesting origin. I would suggest keeping this as GetPermissionStatus(requesting_origin, requesting_origin) in this CL. If we want to change the scoping, we should do it as a separate change. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.cc (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:15: permission_type_(permission_type) { I'd suggest naming this content_type_ (to distinguish from PermissionType). https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:16: CHECK(permission_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC || Should be DCHECK - use CHECK sparingly https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:52: return CONTENT_SETTING_BLOCK; nit: include components/content_settings/core/common/content_settings.h https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:57: DCHECK_EQ(policy, POLICY_NOT_SET); nit: the arguments are in the wrong order https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:82: } Mounir: we actually can't do this yet. The reason is that this will be used for Flash too which currently does allow http origins to request audio/video. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.h (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.h:43: DISALLOW_COPY_AND_ASSIGN(MediaStreamDevicePermissionContext); include base/macros.h https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context_unittest.cc (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context_unittest.cc:84: } There's a bunch of tests in MediaStreamDevicesControllerTest which should probably be converted to test this file (e.g. setting content settings and checking the result, setting policy and checking the result. I don't want to make you do that change just yet but please add a TODO saying that we should add more tests and they can be based off the tests in that file.
lgtm for the small portion of content changes (deferring real review to permission owners)
lgtm with more tests. Also, could you change your CL description to better match the usual style: $TITLE $LONG_DESCRIPTION BUG=XYZ Like: """ Introduce AUDIO_RECORDING and VIDEO_RECORDING permissions. This CL adds the permissions to the Permission Mojo Service and PermissionType enum. It is also adding a PermissionContext to handle them. The permissions are being used in $LINK_TO_CL_USING_THEM BUG=520101 """ https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.cc (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:17: permission_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA); nit: you might want to check the member, not the argument. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:28: NOTREACHED() << "RequestPermission is not implemented"; This would only fail in DEBUG. Could you resolve the callback so a release build wouldn't break. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:45: NOTREACHED(); NOTREACHED() probably not needed, we are checking this in the ctor already. Alternatively, you can do: if (permission_type_ == MEDIASTREAM_MIC) { // ... } else { DCHECK_EQ(permission_type_, MEDIASTREAM_CAMERA); // ... } So at least, you only have two paths. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:65: return setting; nit: you might want to do: return setting == CONTENT_SETTING_DEFAULT ? CONTENT_SETTING_ASK : setting; https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:82: } On 2015/08/27 at 05:54:18, raymes wrote: > Mounir: we actually can't do this yet. The reason is that this will be used for Flash too which currently does allow http origins to request audio/video. I don't think we should try to use PermissionContext for Flash based permissions. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context_unittest.cc (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context_unittest.cc:83: TestInsecureQueryingUrl(CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA); Can you check that it is working for secure origins? You might want to test more situations too like relations between embedder and requesting frame.
android_webview LGTM
Mostly nits. I may be missing some contexts, but I do have a question about naming. Mounir Lamouri / raymes: Let me know what you think about the naming question. https://codereview.chromium.org/1311783007/diff/140001/android_webview/browse... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/1311783007/diff/140001/android_webview/browse... android_webview/browser/aw_permission_manager.cc:211: case PermissionType::AUDIO_RECORDING: It seems to me that AUDIO_RECORDING == MIC VIDEO_RECORDING == CAMERA Why do we use two different sets of names here? It'd be easier to read/remember if we use consistent names across layers. Also, all other permission types are using consistent names. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_camera_permission_context_factory.h (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_camera_permission_context_factory.h:13: nit: no need to have an empty line here https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.cc (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:15: permission_type_(permission_type) { On 2015/08/27 05:54:18, raymes wrote: > I'd suggest naming this content_type_ (to distinguish from PermissionType). +1, or just strictly follow the type name: content_settings_type_, which will leave no room for ambiguity. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:40: urls_policy_name = prefs::kAudioCaptureAllowedUrls; err, here we are using another set of names: AudioCapture VideoCapture Is there any reason we can't make the names consistent for, say, AudioCapture, AudioRecording and Mic? https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.h (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.h:20: nit: // PermissionContextBase: https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context_unittest.cc (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context_unittest.cc:27: const ContentSettingsType permission_type) s/explicit// https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_mic_permission_context_factory.h (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_mic_permission_context_factory.h:13: ditto
working on adding one more test case https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_camera_permission_context_factory.h (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_camera_permission_context_factory.h:13: On 2015/08/27 19:25:17, xhwang wrote: > nit: no need to have an empty line here Done. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.cc (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:15: permission_type_(permission_type) { On 2015/08/27 19:25:17, xhwang wrote: > On 2015/08/27 05:54:18, raymes wrote: > > I'd suggest naming this content_type_ (to distinguish from PermissionType). > > +1, or just strictly follow the type name: content_settings_type_, which will > leave no room for ambiguity. Done. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:16: CHECK(permission_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC || On 2015/08/27 05:54:18, raymes wrote: > Should be DCHECK - use CHECK sparingly Done. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:17: permission_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA); On 2015/08/27 10:03:40, Mounir Lamouri wrote: > nit: you might want to check the member, not the argument. Done. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:28: NOTREACHED() << "RequestPermission is not implemented"; On 2015/08/27 10:03:40, Mounir Lamouri wrote: > This would only fail in DEBUG. Could you resolve the callback so a release build > wouldn't break. Done. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:45: NOTREACHED(); On 2015/08/27 10:03:39, Mounir Lamouri wrote: > NOTREACHED() probably not needed, we are checking this in the ctor already. > Alternatively, you can do: > if (permission_type_ == MEDIASTREAM_MIC) { > // ... > } else { > DCHECK_EQ(permission_type_, MEDIASTREAM_CAMERA); > // ... > } > > So at least, you only have two paths. Done. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:52: return CONTENT_SETTING_BLOCK; On 2015/08/27 05:54:18, raymes wrote: > nit: include components/content_settings/core/common/content_settings.h Done. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:57: DCHECK_EQ(policy, POLICY_NOT_SET); On 2015/08/27 05:54:18, raymes wrote: > nit: the arguments are in the wrong order Done. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:65: return setting; On 2015/08/27 10:03:39, Mounir Lamouri wrote: > nit: you might want to do: > return setting == CONTENT_SETTING_DEFAULT ? CONTENT_SETTING_ASK : setting; Done. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:82: } On 2015/08/27 10:03:40, Mounir Lamouri wrote: > On 2015/08/27 at 05:54:18, raymes wrote: > > Mounir: we actually can't do this yet. The reason is that this will be used > for Flash too which currently does allow http origins to request audio/video. > > I don't think we should try to use PermissionContext for Flash based > permissions. Are we saying to keep media_permission the same as it is without depending on PermissionContext added here? https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.h (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.h:20: On 2015/08/27 19:25:17, xhwang wrote: > nit: > > // PermissionContextBase: Done. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.h:43: DISALLOW_COPY_AND_ASSIGN(MediaStreamDevicePermissionContext); On 2015/08/27 05:54:18, raymes wrote: > include base/macros.h Done. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context_unittest.cc (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context_unittest.cc:27: const ContentSettingsType permission_type) On 2015/08/27 19:25:17, xhwang wrote: > s/explicit// Done. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context_unittest.cc:84: } On 2015/08/27 05:54:18, raymes wrote: > There's a bunch of tests in MediaStreamDevicesControllerTest which should > probably be converted to test this file (e.g. setting content settings and > checking the result, setting policy and checking the result. I don't want to > make you do that change just yet but please add a TODO saying that we should add > more tests and they can be based off the tests in that file. Done. https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_mic_permission_context_factory.h (right): https://codereview.chromium.org/1311783007/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_mic_permission_context_factory.h:13: On 2015/08/27 19:25:17, xhwang wrote: > ditto Done.
guoweis@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow@chromium.org: Please review changes in histograms.xml
On 2015/08/27 23:19:20, guoweis_chromium wrote: > mailto:rkaplow@chromium.org: Please review changes in histograms.xml Regarding to the naming, I also think we should keep the original one (MEDIASTREAM_MIC and MEDIASTREAM_RECORDING). I'm going to add extra permisson to media/base/media_permission.h which will just spread the inconsistency to more places and become confusing.
https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.cc (right): https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:79: // Changing this once Flash also requires secure origin. I'm not sure what this comment means. Does it mean that you have to return false so that Flash can record media when the SWF is loaded from a non-secure origin? https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.h (right): https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.h:31: const GURL& requesting_origin, Rather than GURL, use url::Origin for origins. This probably applies throughout this class. https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:215: requesting_origin.GetOrigin(), embedding_origin.GetOrigin())); Yeah, unfortunately, GURL::GetOrigin is not the right thing, even though you'd think it would be. You can construct a url::Origin, which is the right thing, from a GURL though.
https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.cc (right): https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:79: // Changing this once Flash also requires secure origin. On 2015/08/28 at 01:44:21, palmer wrote: > I'm not sure what this comment means. Does it mean that you have to return false so that Flash can record media when the SWF is loaded from a non-secure origin? +1. Also, this is should be TODO($user) and point to a bug. I still don't understand why we need the PermissionContext to worry about Flash but let's discuss that in the bug. https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.h (right): https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.h:31: const GURL& requesting_origin, On 2015/08/28 at 01:44:21, palmer wrote: > Rather than GURL, use url::Origin for origins. This probably applies throughout this class. This is inheriting from PermissionContextBase so changing GURL to Origin shouldn't probably not happen in this CL (it will imply a lot of changes). https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context_unittest.cc (right): https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context_unittest.cc:84: TestInsecureQueryingUrl(CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA); You didn't add the tests :(
https://codereview.chromium.org/1311783007/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1311783007/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67244: + <int value="8" label="PERMISSION_AUDIO_RECORIDNG"/> RECORDING
Fixed the typo in histograms.xml. https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.cc (right): https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:79: // Changing this once Flash also requires secure origin. On 2015/08/28 10:56:21, Mounir Lamouri wrote: > On 2015/08/28 at 01:44:21, palmer wrote: > > I'm not sure what this comment means. Does it mean that you have to return > false so that Flash can record media when the SWF is loaded from a non-secure > origin? > > +1. > Also, this is should be TODO($user) and point to a bug. > > I still don't understand why we need the PermissionContext to worry about Flash > but let's discuss that in the bug. My goal here is to refactor without introducing functional change. Hence this is probably best left with someone else to handle this. Raymes, may I delegate this to you? Could you open a new bug to track this discussion? https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.h (right): https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.h:31: const GURL& requesting_origin, On 2015/08/28 10:56:21, Mounir Lamouri wrote: > On 2015/08/28 at 01:44:21, palmer wrote: > > Rather than GURL, use url::Origin for origins. This probably applies > throughout this class. > > This is inheriting from PermissionContextBase so changing GURL to Origin > shouldn't probably not happen in this CL (it will imply a lot of changes). I suspect this is related to 490074 and other related bugs. I'll open a new bug for PermissionContext and link them together. https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context_unittest.cc (right): https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context_unittest.cc:84: TestInsecureQueryingUrl(CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA); On 2015/08/28 10:56:21, Mounir Lamouri wrote: > You didn't add the tests :( Sorry, it was on my TODO list but forgot to put a comment mentioning about this. I'll add a test today with next update of the code. https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/1311783007/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:215: requesting_origin.GetOrigin(), embedding_origin.GetOrigin())); On 2015/08/28 01:44:21, palmer wrote: > Yeah, unfortunately, GURL::GetOrigin is not the right thing, even though you'd > think it would be. You can construct a url::Origin, which is the right thing, > from a GURL though. Will use a bug to track this. https://codereview.chromium.org/1311783007/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1311783007/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67244: + <int value="8" label="PERMISSION_AUDIO_RECORIDNG"/> On 2015/08/28 15:54:44, rkaplow wrote: > RECORDING Fixed the typo.
I think AUDIO_RECORDING and VIDEO_RECORDING are better names than MEDIASTREAM_MIC and MEDIASTREAM_CAM. They are more descriptive and clearer as general browser permissions.
On 2015/08/28 17:00:10, Mounir Lamouri wrote: > I think AUDIO_RECORDING and VIDEO_RECORDING are better names than > MEDIASTREAM_MIC and MEDIASTREAM_CAM. They are more descriptive and clearer as > general browser permissions. Drive-by: Generalizing beyond mic/camera and MEDIASTREAM makes sense. However, I think "recording" has a specific meaning that could be ambiguous here. That is, saving it somewhere. USER_AUDIO or AUDIO_CAPTURE are less ambiguous options IMO.
AUDIO_CAPTURE and VIDEO_CAPTURE sgtm!
On 2015/08/28 17:00:10, Mounir Lamouri wrote: > I think AUDIO_RECORDING and VIDEO_RECORDING are better names than > MEDIASTREAM_MIC and MEDIASTREAM_CAM. They are more descriptive and clearer as > general browser permissions. That makes sense. I agree that these permissions should not be MediaStream specific. But then thinking about this further, why don't we just use "MICROPHONE" and "CAMERA"? Here are some random thoughts to justify this: - User may use camera to capture an image, which isn't "video recording" [1]. Our permission should cover that use case. - On Android, we also have Camera/Microphone as the permission group name [2] - In chrome://settings/content and our permission UI [3], we are showing Camera/Microphone as the name for the settings. [1] http://www.w3.org/TR/html-media-capture/#security [2] https://support.google.com/googleplay/answer/6014972?hl=en [3] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat...
A user can also use a camera to record sound with no pixels. I think using VIDEO instead of PIXEL is fine. Taking a picture is an edge case of taking a video: it's a 1 frame video. The privacy/security concerns are mostly the same.
On 2015/08/28 17:30:48, Mounir Lamouri wrote: > A user can also use a camera to record sound with no pixels. > > I think using VIDEO instead of PIXEL is fine. Taking a picture is an edge case > of taking a video: it's a 1 frame video. The privacy/security concerns are > mostly the same. What we are discussing here are implementation details, not about user facing UI. Today we are already showing users that "<...> wants to access your CAMERA" [1]. I don't think we'll change that UI, as it's pretty standard across platforms, e.g. Chrome, Android, iOS... So this naming discussion is really for our internal implementation, which I'd lean toward consistency/simplicity. That's why I am in favor of names that are consistent with our UI. That being said, for your concern about camera can record audio, I think user knows whether the camera can record sound or not. So if a user choose to allow the app to use the "camera", he/she also allows the sound recording by the camera. That's a natural process and I don't see any issue with that. Also, I don't feel taking picture is an edge use case of a camera (even though you can say a picture a special 1-frame video). For most users "video recording" doesn't naturally mean taking a still picture. [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat...
lgtm
Patchset #8 (id:220001) has been deleted
On 2015/08/30 17:42:45, rkaplow wrote: > lgtm PTAL. For site like http://tinychat.com, it uses flash which doesn't require secure origin. If we block the insecure origin, such use case will be broken.
I fear that url::Origin refactor may have to happen much sooner rather than later. https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.cc (right): https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:40: if (content_settings_type_ == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) { I wonder if this would be more clear/obvious as a switch case? I usually prefer that with enums. const char* policy_name = nullptr; const char* urls_policy_name = nullptr; switch (content_settings_type_) { case CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC: policy_name = prefs::kAudioCaptureAllowed; urls_policy_name = prefs::kAudioCaptureAllowedUrls; break; case CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA: // ... break; default: NOTREACHED(); } https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:52: if (policy == ALWAYS_DENY) Same here, switching over the enum seems better. https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.h (right): https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.h:34: void ResetPermission(const GURL& requesting_origin, Can you please add a TODO and file a bug, and CC me on it, that we need to refactor this to use url::Origin? Thanks. https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:215: NOTREACHED(); This should be the default: clause of the switch. The entire function is then a switch statement. https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:71: // This will hit the NOTREACHED below. Same here: just put the default case inside the switch with an explicit default: label. (a) it's more straightforward ("The purpose of this function is to map 1 enum to another, and nothing else"); (b) I wonder if the compiler finds it easier to optimize such pure-switch functions. https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:215: requesting_origin.GetOrigin(), embedding_origin.GetOrigin())); I'm still really anxious about this. I believe it may be incorrect, given the documented contract of GetOrigin: 166 // A helper function to return a GURL containing just the scheme, host, 167 // and port from a URL. Equivalent to clearing any username and password, 168 // replacing the path with a slash, and clearing everything after that. If 169 // this URL is not a standard URL, then the result will be an empty, 170 // invalid GURL. If the URL has neither username nor password, this 171 // degenerates to GetWithEmptyPath(). 172 // 173 // It is an error to get the origin of an invalid URL. The result 174 // will be the empty URL. 175 GURL GetOrigin() const; That is, all non-standard URLs will be equivalent, which is a violation of the same origin policy. Note the implementation of the comparators: 182 bool GURL::operator==(const GURL& other) const { 183 return spec_ == other.spec_; 184 } 185 186 bool GURL::operator!=(const GURL& other) const { 187 return spec_ != other.spec_; 188 } 189 190 bool GURL::operator<(const GURL& other) const { 191 return spec_ < other.spec_; 192 } 193 194 bool GURL::operator>(const GURL& other) const { 195 return spec_ > other.spec_; 196 } https://codereview.chromium.org/1311783007/diff/240001/content/browser/permis... File content/browser/permissions/permission_service_impl.cc (right): https://codereview.chromium.org/1311783007/diff/240001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:38: NOTREACHED(); Put this in the default:.
> For site like http://tinychat.com, it uses flash which doesn't require secure > origin. If we block the insecure origin, such use case will be broken. Isn't that a good thing?
guoweis@chromium.org changed reviewers: - xhwang@chromium.org
palmer@, PTAL. https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.cc (right): https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:40: if (content_settings_type_ == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) { On 2015/09/01 18:31:04, palmer wrote: > I wonder if this would be more clear/obvious as a switch case? I usually prefer > that with enums. > > const char* policy_name = nullptr; > const char* urls_policy_name = nullptr; > switch (content_settings_type_) { > case CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC: > policy_name = prefs::kAudioCaptureAllowed; > urls_policy_name = prefs::kAudioCaptureAllowedUrls; > break; > case CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA: > // ... > break; > default: > NOTREACHED(); > } This class could only be used with MEDIASTREAM_MIC and MEDIASTREAM_CAMERA (checked in ctor). Any other values don't make sense here. https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.cc:52: if (policy == ALWAYS_DENY) On 2015/09/01 18:31:04, palmer wrote: > Same here, switching over the enum seems better. Done. https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.h (right): https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.h:34: void ResetPermission(const GURL& requesting_origin, On 2015/09/01 18:31:04, palmer wrote: > Can you please add a TODO and file a bug, and CC me on it, that we need to > refactor this to use url::Origin? Thanks. crbug.com/527149. However, since I don't have security permission, can't add you to the cc. https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:215: NOTREACHED(); On 2015/09/01 18:31:04, palmer wrote: > This should be the default: clause of the switch. The entire function is then a > switch statement. It's intended to be without default so compiler can fail when a new enum is added here. https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:71: // This will hit the NOTREACHED below. On 2015/09/01 18:31:04, palmer wrote: > Same here: just put the default case inside the switch with an explicit default: > label. > > (a) it's more straightforward ("The purpose of this function is to map 1 enum to > another, and nothing else"); (b) I wonder if the compiler finds it easier to > optimize such pure-switch functions. intentionally leave out default to have compiler catch the case when enum is missing. https://codereview.chromium.org/1311783007/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:215: requesting_origin.GetOrigin(), embedding_origin.GetOrigin())); On 2015/09/01 18:31:04, palmer wrote: > I'm still really anxious about this. I believe it may be incorrect, given the > documented contract of GetOrigin: > > 166 // A helper function to return a GURL containing just the scheme, host, > 167 // and port from a URL. Equivalent to clearing any username and password, > 168 // replacing the path with a slash, and clearing everything after that. If > 169 // this URL is not a standard URL, then the result will be an empty, > 170 // invalid GURL. If the URL has neither username nor password, this > 171 // degenerates to GetWithEmptyPath(). > 172 // > 173 // It is an error to get the origin of an invalid URL. The result > 174 // will be the empty URL. > 175 GURL GetOrigin() const; > > That is, all non-standard URLs will be equivalent, which is a violation of the > same origin policy. Note the implementation of the comparators: > > 182 bool GURL::operator==(const GURL& other) const { > 183 return spec_ == other.spec_; > 184 } > 185 > 186 bool GURL::operator!=(const GURL& other) const { > 187 return spec_ != other.spec_; > 188 } > 189 > 190 bool GURL::operator<(const GURL& other) const { > 191 return spec_ < other.spec_; > 192 } > 193 > 194 bool GURL::operator>(const GURL& other) const { > 195 return spec_ > other.spec_; > 196 } crbug.com/527149 filed for this. https://codereview.chromium.org/1311783007/diff/240001/content/browser/permis... File content/browser/permissions/permission_service_impl.cc (right): https://codereview.chromium.org/1311783007/diff/240001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:38: NOTREACHED(); On 2015/09/01 18:31:04, palmer wrote: > Put this in the default:. same here.
guoweis@chromium.org changed reviewers: + ddorwin@chromium.org
ddorwin@chromium.org: Please review changes in chrome/browser/media
guoweis@chromium.org changed reviewers: + tommi@chromium.org - ddorwin@chromium.org
On 2015/09/01 19:45:11, guoweis_chromium wrote: > mailto:ddorwin@chromium.org: Please review changes in chrome/browser/media +tommi@chromium.org
lgtm for media. https://codereview.chromium.org/1311783007/diff/280001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.h (right): https://codereview.chromium.org/1311783007/diff/280001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.h:30: // TODO(tbd): GURL.GetOrigin() shouldn't be used as the origin. Need to is tbd right? https://codereview.chromium.org/1311783007/diff/280001/chrome/browser/media/m... File chrome/browser/media/media_stream_mic_permission_context_factory.h (right): https://codereview.chromium.org/1311783007/diff/280001/chrome/browser/media/m... chrome/browser/media/media_stream_mic_permission_context_factory.h:28: KeyedService* BuildServiceInstanceFor( could these return a scoped_ptr?
lgtm
lgtm
Updated the owner for the TODO. landing this now. https://codereview.chromium.org/1311783007/diff/280001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.h (right): https://codereview.chromium.org/1311783007/diff/280001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.h:30: // TODO(tbd): GURL.GetOrigin() shouldn't be used as the origin. Need to On 2015/09/01 21:29:02, tommi wrote: > is tbd right? changed to xhwang. https://codereview.chromium.org/1311783007/diff/280001/chrome/browser/media/m... File chrome/browser/media/media_stream_mic_permission_context_factory.h (right): https://codereview.chromium.org/1311783007/diff/280001/chrome/browser/media/m... chrome/browser/media/media_stream_mic_permission_context_factory.h:28: KeyedService* BuildServiceInstanceFor( On 2015/09/01 21:29:02, tommi wrote: > could these return a scoped_ptr? This was an override function. Changing the signature will be a much larger change.
The CQ bit was checked by guoweis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, michaelbai@chromium.org, kinuko@chromium.org, raymes@chromium.org, rkaplow@chromium.org, palmer@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1311783007/#ps320001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1311783007/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311783007/320001
Message was sent while issue was closed.
Committed patchset #12 (id:320001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/c409888759da9a9e8bdd1089790dd52f1b681b54 Cr-Commit-Position: refs/heads/master@{#346776}
Message was sent while issue was closed.
https://codereview.chromium.org/1311783007/diff/280001/chrome/browser/media/m... File chrome/browser/media/media_stream_device_permission_context.h (right): https://codereview.chromium.org/1311783007/diff/280001/chrome/browser/media/m... chrome/browser/media/media_stream_device_permission_context.h:30: // TODO(tbd): GURL.GetOrigin() shouldn't be used as the origin. Need to For future reference you are meant to put your own name in a TODO. The name listed isn't the person who will necessarily do the work, but it is the person who is the best to contact about getting more info/resolving the problem.
Message was sent while issue was closed.
On 2015/08/28 17:42:12, xhwang (OOO till 09-28) wrote: > On 2015/08/28 17:30:48, Mounir Lamouri wrote: > > A user can also use a camera to record sound with no pixels. > > > > I think using VIDEO instead of PIXEL is fine. Taking a picture is an edge case > > of taking a video: it's a 1 frame video. The privacy/security concerns are > > mostly the same. > > What we are discussing here are implementation details, not about user facing > UI. Today we are already showing users that "<...> wants to access your CAMERA" > [1]. > > I don't think we'll change that UI, as it's pretty standard across platforms, > e.g. Chrome, Android, iOS... > > So this naming discussion is really for our internal implementation, which I'd > lean toward consistency/simplicity. That's why I am in favor of names that are > consistent with our UI. > > That being said, for your concern about camera can record audio, I think user > knows whether the camera can record sound or not. So if a user choose to allow > the app to use the "camera", he/she also allows the sound recording by the > camera. That's a natural process and I don't see any issue with that. > > Also, I don't feel taking picture is an edge use case of a camera (even though > you can say a picture a special 1-frame video). For most users "video recording" > doesn't naturally mean taking a still picture. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat... What's our decision on the naming? I didn't see replies to my last comment.... Mounir Lamouri / guoweis / ddorwin: What do you think on this?
Message was sent while issue was closed.
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
Message was sent while issue was closed.
Should we also update the CL description to reflect the latest naming? https://codereview.chromium.org/1311783007/diff/320001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1311783007/diff/320001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:211: return "VideoRecording"; Should we s/Recording/Capture?
Message was sent while issue was closed.
On 2015/09/02 22:26:03, xhwang (OOO till 09-28) wrote: > On 2015/08/28 17:42:12, xhwang (OOO till 09-28) wrote: > > On 2015/08/28 17:30:48, Mounir Lamouri wrote: > > > A user can also use a camera to record sound with no pixels. > > > > > > I think using VIDEO instead of PIXEL is fine. Taking a picture is an edge > case > > > of taking a video: it's a 1 frame video. The privacy/security concerns are > > > mostly the same. > > > > What we are discussing here are implementation details, not about user facing > > UI. Today we are already showing users that "<...> wants to access your > CAMERA" > > [1]. > > > > I don't think we'll change that UI, as it's pretty standard across platforms, > > e.g. Chrome, Android, iOS... > > > > So this naming discussion is really for our internal implementation, which I'd > > lean toward consistency/simplicity. That's why I am in favor of names that are > > consistent with our UI. > > > > That being said, for your concern about camera can record audio, I think user > > knows whether the camera can record sound or not. So if a user choose to allow > > the app to use the "camera", he/she also allows the sound recording by the > > camera. That's a natural process and I don't see any issue with that. > > > > Also, I don't feel taking picture is an edge use case of a camera (even though > > you can say a picture a special 1-frame video). For most users "video > recording" > > doesn't naturally mean taking a still picture. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat... > > What's our decision on the naming? I didn't see replies to my last comment.... > > Mounir Lamouri / guoweis / ddorwin: What do you think on this? I thought we have made the conclusion for that and I ended up using AUDIO_CAPTURE and VIDEO_CAPTURE.
Message was sent while issue was closed.
On 2015/09/02 22:28:36, guoweis_chromium wrote: > On 2015/09/02 22:26:03, xhwang (OOO till 09-28) wrote: > > On 2015/08/28 17:42:12, xhwang (OOO till 09-28) wrote: > > > On 2015/08/28 17:30:48, Mounir Lamouri wrote: > > > > A user can also use a camera to record sound with no pixels. > > > > > > > > I think using VIDEO instead of PIXEL is fine. Taking a picture is an edge > > case > > > > of taking a video: it's a 1 frame video. The privacy/security concerns are > > > > mostly the same. > > > > > > What we are discussing here are implementation details, not about user > facing > > > UI. Today we are already showing users that "<...> wants to access your > > CAMERA" > > > [1]. > > > > > > I don't think we'll change that UI, as it's pretty standard across > platforms, > > > e.g. Chrome, Android, iOS... > > > > > > So this naming discussion is really for our internal implementation, which > I'd > > > lean toward consistency/simplicity. That's why I am in favor of names that > are > > > consistent with our UI. > > > > > > That being said, for your concern about camera can record audio, I think > user > > > knows whether the camera can record sound or not. So if a user choose to > allow > > > the app to use the "camera", he/she also allows the sound recording by the > > > camera. That's a natural process and I don't see any issue with that. > > > > > > Also, I don't feel taking picture is an edge use case of a camera (even > though > > > you can say a picture a special 1-frame video). For most users "video > > recording" > > > doesn't naturally mean taking a still picture. > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat... > > > > What's our decision on the naming? I didn't see replies to my last comment.... > > > > Mounir Lamouri / guoweis / ddorwin: What do you think on this? > > I thought we have made the conclusion for that and I ended up using > AUDIO_CAPTURE and VIDEO_CAPTURE. Understood :) _CAPTURE is better than _RECORDING. But I wonder whether we could just use MIC and CAMERA. See my previous comment. Let me know what you think about it.
Message was sent while issue was closed.
I think the name that landed is great. |
