Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696703006:
Move media permission checking logic for ChromeOS login pages (Closed)
|
Created:
3 years, 10 months ago by raymes Modified:
3 years, 10 months ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove media permission checking logic for ChromeOS login pages
Currently the permission checking logic for ChromeOS login pages (which is used
to determine whether to allow webcam access for SAML login pages) is contained
in WebUILoginView and not with the other permission-checking logic for media
permissions. This has several impacts, one of which is to do with correctness
(see below) but the other is that it makes it hard to refactor the media
permissions code. The flow of the media permissions code is:
1) Do some device checks
2) Do some perission checks
3) Show a prompt if needed
4) Store permission decisions
5) Allow/deny access and update some state based on success
Currently the custom permission checks in WebUILoginView fit in somewhere
between 2 and 3. It means that we need to exit the existing flow and
re-enter it at the right point (4), which means that we need to expose a
complex API for permissions. By moving this logic into (2) itself, we can
refactor the permissions code to have a simpler, more sensible API.
This CL results in a slight behavioral changes, which more than likely fix
an existing bug. Currently, the permission will be granted in the login view,
if the user had previously manually granted permissioni to that origin in a
regular browsing session. Now, the only URLs that will be granted access are
those listed in the kLoginVideoCaptureAllowedUrls pref.
BUG=596786
Review-Url: https://codereview.chromium.org/2696703006
Cr-Commit-Position: refs/heads/master@{#452300}
Committed: https://chromium.googlesource.com/chromium/src/+/6b9217880b3814ec933c74beb183375ec840cfaa
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Move media permission checking logic for ChromeOS login pages #Patch Set 4 : Move media permission checking logic for ChromeOS login pages #Patch Set 5 : Move media permission checking logic for ChromeOS login pages #Patch Set 6 : Move media permission checking logic for ChromeOS login pages #
Total comments: 29
Patch Set 7 : Move media permission checking logic for ChromeOS login pages #Patch Set 8 : Move media permission checking logic for ChromeOS login pages #
Total comments: 2
Patch Set 9 : Move media permission checking logic for ChromeOS login pages #
Messages
Total messages: 40 (24 generated)
Description was changed from ========== [NOT TO LAND] Test BUG= ========== to ========== Move media permission checking logic for ChromeOS login pages Currently the permission checking logic for ChromeOS login pages (which is used to determine whether to allow webcam access for SAML login pages) is contained in WebUILoginView and not with the other permission-checking logic for media permissions. This has several impacts, one of which is to do with correctness (see below) but the other is that it makes it hard to refactor the media permissions code. The flow of the media permissions code is: 1) Do some device checks 2) Do some perission checks 3) Show a prompt if needed 4) Store permission decisions 5) Allow/deny access and update some state based on success Currently the custom permission checks in WebUILoginView fit in somewhere between 2 and 3. It means that we need to exit the existing flow and re-enter it at the right point (4), which means that we need to expose a complex API for permissions. By moving this logic into (2) itself, we can refactor the permissions code to have a simpler, more sensible API. This CL results in a slight behavioral changes, which more than likely fix an existing bug. Currently, the permission will be granted in the login view, if the user had previously manually granted permissioni to that origin in a regular browsing session. Now, the only URLs that will be granted access are those listed in the kLoginVideoCaptureAllowedUrls pref. BUG=596786 ==========
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Description was changed from ========== Move media permission checking logic for ChromeOS login pages Currently the permission checking logic for ChromeOS login pages (which is used to determine whether to allow webcam access for SAML login pages) is contained in WebUILoginView and not with the other permission-checking logic for media permissions. This has several impacts, one of which is to do with correctness (see below) but the other is that it makes it hard to refactor the media permissions code. The flow of the media permissions code is: 1) Do some device checks 2) Do some perission checks 3) Show a prompt if needed 4) Store permission decisions 5) Allow/deny access and update some state based on success Currently the custom permission checks in WebUILoginView fit in somewhere between 2 and 3. It means that we need to exit the existing flow and re-enter it at the right point (4), which means that we need to expose a complex API for permissions. By moving this logic into (2) itself, we can refactor the permissions code to have a simpler, more sensible API. This CL results in a slight behavioral changes, which more than likely fix an existing bug. Currently, the permission will be granted in the login view, if the user had previously manually granted permissioni to that origin in a regular browsing session. Now, the only URLs that will be granted access are those listed in the kLoginVideoCaptureAllowedUrls pref. BUG=596786 ========== to ========== Move media permission checking logic for ChromeOS login pages Currently the permission checking logic for ChromeOS login pages (which is used to determine whether to allow webcam access for SAML login pages) is contained in WebUILoginView and not with the other permission-checking logic for media permissions. This has several impacts, one of which is to do with correctness (see below) but the other is that it makes it hard to refactor the media permissions code. The flow of the media permissions code is: 1) Do some device checks 2) Do some perission checks 3) Show a prompt if needed 4) Store permission decisions 5) Allow/deny access and update some state based on success Currently the custom permission checks in WebUILoginView fit in somewhere between 2 and 3. It means that we need to exit the existing flow and re-enter it at the right point (4), which means that we need to expose a complex API for permissions. By moving this logic into (2) itself, we can refactor the permissions code to have a simpler, more sensible API. This CL results in a slight behavioral changes, which more than likely fix an existing bug. Currently, the permission will be granted in the login view, if the user had previously manually granted permissioni to that origin in a regular browsing session. Now, the only URLs that will be granted access are those listed in the kLoginVideoCaptureAllowedUrls pref. BUG=596786 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
raymes@chromium.org changed reviewers: + achuith@chromium.org, sergeyu@chromium.org, timloh@chromium.org
timloh: for general review achuith: for chrome/browser/chromeos/login sergeyu: for chrome/browser/media/webrtc
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/saml/saml_browsertest.cc:1431: IN_PROC_BROWSER_TEST_F(SAMLPolicyTest, TestLoginMediaPermission) { Please add a comment describing what this test is attempting to accomplish. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/saml/saml_browsertest.cc:1434: GURL url1("https://google.com"); const url1, url2, url3, and permission vars. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/ui/webui_login_view.h (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/ui/webui_login_view.h:60: static bool IsWebUILoginView(const content::WebContentsDelegate* delegate); Let's not do this here. You can get WebUILoginView webcontents by calling: LoginDisplayHost::default_host()->GetWebUILoginView()->GetWebContents(); For eg. https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/screens/er... If you want to keep this method (I would prefer not), you could add it in login/helper.cc: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/helper.cc?... The implementation should look like: bool IsWebUILoginView(const content::WebContents* web_contents) { return GetLoginWebContents() == web_contents; } But you're only using this in one place so I think you can just inline this logic in media_permission.cc https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_permission.cc (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:52: (void)web_contents_; This seems pretty sketchy. Are there any other places in the codebase where we do this? https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:102: ContentSettingsPattern pattern = nit const https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:113: return CONTENT_SETTING_BLOCK; Wonder if there's a way to avoid the 4x repetition of these two lines. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_permission.h (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.h:20: } // namespace content drop comment https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.h:51: content::WebContents* web_contents_; nit const: content::WebContents* const web_contents_;
Thanks for the helpful comments! I haven't updated the CL yet but just a few questions to clarify. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/ui/webui_login_view.h (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/ui/webui_login_view.h:60: static bool IsWebUILoginView(const content::WebContentsDelegate* delegate); I see - so there is only ever one of these around? That would make sense. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_permission.cc (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:52: (void)web_contents_; On 2017/02/16 09:05:38, achuithb wrote: > This seems pretty sketchy. Are there any other places in the codebase where we > do this? +jyasskin (C++ expert): Jeffrey does this seem like a reasonable approach to you? The alternative would be to ifdef out the member in all the places. At some point it's likely that the web_contents will get used on other platforms too.
https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/ui/webui_login_view.h (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/ui/webui_login_view.h:60: static bool IsWebUILoginView(const content::WebContentsDelegate* delegate); On 2017/02/16 10:36:14, raymes wrote: > I see - so there is only ever one of these around? That would make sense. Yup, there's only one
raymes@chromium.org changed reviewers: + jyasskin@chromium.org
Oops, actually +jyasskin this time. Please see the comment directed at you :)
https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_permission.cc (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:52: (void)web_contents_; ALLOW_UNUSED_LOCAL()? https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:98: CHECK(raw_list_value->GetAsList(&list_value)); It's best to avoid side-effects in CHECK() statements. If someone decides to change this to DCHECK then release builds would stop working. The following would be better: if (!raw_list_value->GetAsList(&list_value))) { LOG(FATAL) << "Boo"; } https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:115: #endif // defined(OS_CHROMEOS) https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_permission.h (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.h:20: } // namespace content On 2017/02/16 09:05:38, achuithb wrote: > drop comment Why? It's required by style guide.
Performed the manual testing of this CL on a Chromebook with a Clever.com account. The camera capturing during SAML login worked fine.
https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_permission.h (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.h:20: } // namespace content On 2017/02/16 19:26:17, Sergey Ulanov wrote: > On 2017/02/16 09:05:38, achuithb wrote: > > drop comment > > Why? It's required by style guide. We drop it for short namespaces since the namespace that the closing brace belongs to is obvious, and it adds unnecessary clutter.
https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_permission.cc (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:52: (void)web_contents_; On 2017/02/16 19:26:17, Sergey Ulanov wrote: > ALLOW_UNUSED_LOCAL()? That seems misleading for a non-local variable, but it does compile to the same thing. I'm not necessarily the best person to judge this: (void)variable is definitely in my vocabulary, but the important question is whether it's in most non-experts' vocabulary. #ifdef'ing will save some memory, of course, but I doubt there are enough MediaPermission objects for that to matter.
Updated the CL - thanks! https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/saml/saml_browsertest.cc:1431: IN_PROC_BROWSER_TEST_F(SAMLPolicyTest, TestLoginMediaPermission) { On 2017/02/16 09:05:38, achuithb wrote: > Please add a comment describing what this test is attempting to accomplish. Done. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/saml/saml_browsertest.cc:1434: GURL url1("https://google.com"); On 2017/02/16 09:05:37, achuithb wrote: > const url1, url2, url3, and permission vars. Done. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/ui/webui_login_view.h (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/ui/webui_login_view.h:60: static bool IsWebUILoginView(const content::WebContentsDelegate* delegate); On 2017/02/16 10:38:49, achuithb wrote: > On 2017/02/16 10:36:14, raymes wrote: > > I see - so there is only ever one of these around? That would make sense. > > Yup, there's only one Done. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/ui/webui_login_view.h:60: static bool IsWebUILoginView(const content::WebContentsDelegate* delegate); On 2017/02/16 10:38:49, achuithb wrote: > On 2017/02/16 10:36:14, raymes wrote: > > I see - so there is only ever one of these around? That would make sense. > > Yup, there's only one Done. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_permission.cc (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:52: (void)web_contents_; On 2017/02/17 17:34:32, Jeffrey Yasskin wrote: > On 2017/02/16 19:26:17, Sergey Ulanov wrote: > > ALLOW_UNUSED_LOCAL()? > > That seems misleading for a non-local variable, but it does compile to the same > thing. > > I'm not necessarily the best person to judge this: (void)variable is definitely > in my vocabulary, but the important question is whether it's in most > non-experts' vocabulary. > > #ifdef'ing will save some memory, of course, but I doubt there are enough > MediaPermission objects for that to matter. I'd like to stick with this for now as I think it's the most sensible way forward. I think I've made it clear what is happening by the comment. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:98: CHECK(raw_list_value->GetAsList(&list_value)); On 2017/02/16 19:26:17, Sergey Ulanov wrote: > It's best to avoid side-effects in CHECK() statements. If someone decides to > change this to DCHECK then release builds would stop working. > The following would be better: > if (!raw_list_value->GetAsList(&list_value))) { > LOG(FATAL) << "Boo"; > } Agreed - I just copied this code over from the other file. Done. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:102: ContentSettingsPattern pattern = On 2017/02/16 09:05:38, achuithb wrote: > nit const Done. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:113: return CONTENT_SETTING_BLOCK; On 2017/02/16 09:05:38, achuithb wrote: > Wonder if there's a way to avoid the 4x repetition of these two lines. I don't think there's a simple way right now but it may be simpler after some subsequent refactorings. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:115: #endif On 2017/02/16 19:26:17, Sergey Ulanov wrote: > // defined(OS_CHROMEOS) Done. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_permission.h (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.h:20: } // namespace content On 2017/02/16 20:02:12, achuithb wrote: > On 2017/02/16 19:26:17, Sergey Ulanov wrote: > > On 2017/02/16 09:05:38, achuithb wrote: > > > drop comment > > > > Why? It's required by style guide. > > We drop it for short namespaces since the namespace that the closing brace > belongs to is obvious, and it adds unnecessary clutter. I've seen both and I don't feel strongly. I've removed it for now though generally I think it's better to stick to the style guide when it's provided. https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.h:51: content::WebContents* web_contents_; On 2017/02/16 09:05:38, achuithb wrote: > nit const: > content::WebContents* const web_contents_; Done.
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
lgtm https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_permission.cc (right): https://codereview.chromium.org/2696703006/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:52: (void)web_contents_; On 2017/02/21 01:29:55, raymes wrote: > On 2017/02/17 17:34:32, Jeffrey Yasskin wrote: > > On 2017/02/16 19:26:17, Sergey Ulanov wrote: > > > ALLOW_UNUSED_LOCAL()? > > > > That seems misleading for a non-local variable, but it does compile to the > same > > thing. > > > > I'm not necessarily the best person to judge this: (void)variable is > definitely > > in my vocabulary, but the important question is whether it's in most > > non-experts' vocabulary. > > > > #ifdef'ing will save some memory, of course, but I doubt there are enough > > MediaPermission objects for that to matter. > > I'd like to stick with this for now as I think it's the most sensible way > forward. I think I've made it clear what is happening by the comment. Acknowledged. https://codereview.chromium.org/2696703006/diff/140001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_permission.cc (right): https://codereview.chromium.org/2696703006/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:84: bool is_list = raw_list_value->GetAsList(&list_value); nit const
https://codereview.chromium.org/2696703006/diff/140001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_permission.cc (right): https://codereview.chromium.org/2696703006/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_permission.cc:84: bool is_list = raw_list_value->GetAsList(&list_value); On 2017/02/21 13:54:37, achuithb wrote: > nit const Done.
lgtm
The CQ bit was checked by raymes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, timloh@chromium.org Link to the patchset: https://codereview.chromium.org/2696703006/#ps160001 (title: "Move media permission checking logic for ChromeOS login pages")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487804158268330,
"parent_rev": "2ad35b86c33fe62c3be7c9368a009716d9892324", "commit_rev":
"6b9217880b3814ec933c74beb183375ec840cfaa"}
Message was sent while issue was closed.
Description was changed from ========== Move media permission checking logic for ChromeOS login pages Currently the permission checking logic for ChromeOS login pages (which is used to determine whether to allow webcam access for SAML login pages) is contained in WebUILoginView and not with the other permission-checking logic for media permissions. This has several impacts, one of which is to do with correctness (see below) but the other is that it makes it hard to refactor the media permissions code. The flow of the media permissions code is: 1) Do some device checks 2) Do some perission checks 3) Show a prompt if needed 4) Store permission decisions 5) Allow/deny access and update some state based on success Currently the custom permission checks in WebUILoginView fit in somewhere between 2 and 3. It means that we need to exit the existing flow and re-enter it at the right point (4), which means that we need to expose a complex API for permissions. By moving this logic into (2) itself, we can refactor the permissions code to have a simpler, more sensible API. This CL results in a slight behavioral changes, which more than likely fix an existing bug. Currently, the permission will be granted in the login view, if the user had previously manually granted permissioni to that origin in a regular browsing session. Now, the only URLs that will be granted access are those listed in the kLoginVideoCaptureAllowedUrls pref. BUG=596786 ========== to ========== Move media permission checking logic for ChromeOS login pages Currently the permission checking logic for ChromeOS login pages (which is used to determine whether to allow webcam access for SAML login pages) is contained in WebUILoginView and not with the other permission-checking logic for media permissions. This has several impacts, one of which is to do with correctness (see below) but the other is that it makes it hard to refactor the media permissions code. The flow of the media permissions code is: 1) Do some device checks 2) Do some perission checks 3) Show a prompt if needed 4) Store permission decisions 5) Allow/deny access and update some state based on success Currently the custom permission checks in WebUILoginView fit in somewhere between 2 and 3. It means that we need to exit the existing flow and re-enter it at the right point (4), which means that we need to expose a complex API for permissions. By moving this logic into (2) itself, we can refactor the permissions code to have a simpler, more sensible API. This CL results in a slight behavioral changes, which more than likely fix an existing bug. Currently, the permission will be granted in the login view, if the user had previously manually granted permissioni to that origin in a regular browsing session. Now, the only URLs that will be granted access are those listed in the kLoginVideoCaptureAllowedUrls pref. BUG=596786 Review-Url: https://codereview.chromium.org/2696703006 Cr-Commit-Position: refs/heads/master@{#452300} Committed: https://chromium.googlesource.com/chromium/src/+/6b9217880b3814ec933c74beb183... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/6b9217880b3814ec933c74beb183... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
