Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
262763004:
Per navigation sticky media permissions. (Closed)
|
Created:
6 years, 7 months ago by tommi (sloooow) - chröme Modified:
6 years, 7 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPer navigation sticky media permissions.
This fixes an issue with getUserMedia where users have to repeatedly allow getUserMedia requests made by the same page.
The approach is to cache granted media access flags with the navigation entry. The access is not granted to other instances of the same page and is not persisted.
BUG=269719, 263333
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268187
Patch Set 1 #
Total comments: 8
Patch Set 2 : Add unit test for separate audio and video tracks #Patch Set 3 : Rebase #
Total comments: 17
Patch Set 4 : Move IsRequestAllowedByNavigationEntry to after IsDefaultMediaAccessBlocked #
Messages
Total messages: 20 (0 generated)
perkj - main reviewer, please review all files. jochen - content/public/common/* - I need an owner, but feel free to review the other files for context.
Adding xians who has been involved in the UI work. (I have only made one small change in this area). https://codereview.chromium.org/262763004/diff/1/chrome/browser/media/chrome_... File chrome/browser/media/chrome_media_stream_infobar_browsertest.cc (right): https://codereview.chromium.org/262763004/diff/1/chrome/browser/media/chrome_... chrome/browser/media/chrome_media_stream_infobar_browsertest.cc:204: Test first gum for audio only. Then gum with audio and video. The second gum should still ask for permissions to at least use the video camera right? https://codereview.chromium.org/262763004/diff/1/chrome/browser/media/media_s... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/262763004/diff/1/chrome/browser/media/media_s... chrome/browser/media/media_stream_devices_controller.cc:64: // lists is empty. If they are, we do not have devices available for the are https://codereview.chromium.org/262763004/diff/1/chrome/browser/media/media_s... chrome/browser/media/media_stream_devices_controller.cc:142: return false; What does this mean for tab or screen capture? Can't be done? or does it simply meant that the permissions dialog is showed? https://codereview.chromium.org/262763004/diff/1/chrome/browser/media/media_s... chrome/browser/media/media_stream_devices_controller.cc:300: content::NavigationEntry* navigation_entry = How is this done for HTTPS? Why is there are difference how HTTP and HTTPS is handled? Document if there is a reason.
content/public lgtm
replying to a few comments and getting to work on adding the unit test for separate audio and video acks. https://codereview.chromium.org/262763004/diff/1/chrome/browser/media/media_s... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/262763004/diff/1/chrome/browser/media/media_s... chrome/browser/media/media_stream_devices_controller.cc:64: // lists is empty. If they are, we do not have devices available for the On 2014/05/02 07:14:37, perkj wrote: > are not sure what you mean.... "either of those lists is empty" should be 'is' since it refers to one list (singular from 'either'). the second sentence refers to lists plural, so 'are' correct there I think. https://codereview.chromium.org/262763004/diff/1/chrome/browser/media/media_s... chrome/browser/media/media_stream_devices_controller.cc:142: return false; On 2014/05/02 07:14:37, perkj wrote: > What does this mean for tab or screen capture? Can't be done? or does it simply > meant that the permissions dialog is showed? It means that a permission dialog will be shown if permissions aren't sticky. This is a bit of a paranoid check and a guard against regressions since checks for tab capture have already been made. The important thing here is that this function must never make a decision for anything but MEDIA_DEVICE_[AUDIO|VIDEO]_CAPTURE. All functionality related to other types must be unaffected. https://codereview.chromium.org/262763004/diff/1/chrome/browser/media/media_s... chrome/browser/media/media_stream_devices_controller.cc:300: content::NavigationEntry* navigation_entry = On 2014/05/02 07:14:37, perkj wrote: > How is this done for HTTPS? Why is there are difference how HTTP and HTTPS is > handled? Document if there is a reason. https is handled where IsSchemeSecure() is checked. This particular piece of code runs for both http and https. We don't care at this point whether the protocol is secure or not since if the permissions have been added to the navigation entry, then that means that it previously went through all the proper checks and we don't have to repeat them here.
Add unit test for separate audio and video tracks
Added unit test. ptal
Rebase
lgtm with questions not related to this change. https://codereview.chromium.org/262763004/diff/1/chrome/browser/media/media_s... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/262763004/diff/1/chrome/browser/media/media_s... chrome/browser/media/media_stream_devices_controller.cc:64: // lists is empty. If they are, we do not have devices available for the On 2014/05/02 12:01:26, tommi wrote: > On 2014/05/02 07:14:37, perkj wrote: > > are > > not sure what you mean.... "either of those lists is empty" should be 'is' since > it refers to one list (singular from 'either'). the second sentence refers to > lists plural, so 'are' correct there I think. Humm, seems like like I need an English lesson. http://www.bbc.co.uk/worldservice/learningenglish/grammar/learnit/learnitv128... https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/chr... File chrome/browser/media/chrome_media_stream_infobar_browsertest.cc (right): https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/chr... chrome/browser/media/chrome_media_stream_infobar_browsertest.cc:84: tab_contents, "stopLocalStream()", &result); Do you know what this uses? Stop the tracks or stop the mediastream? Since MediaStream::Stop is deprecated, we should at least not add new dependencies on it. https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/chr... chrome/browser/media/chrome_media_stream_infobar_browsertest.cc:167: // See http://crbug.com/263333. Can this be enabled?
https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/chr... File chrome/browser/media/chrome_media_stream_infobar_browsertest.cc (right): https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/chr... chrome/browser/media/chrome_media_stream_infobar_browsertest.cc:84: tab_contents, "stopLocalStream()", &result); On 2014/05/05 07:28:25, perkj wrote: > Do you know what this uses? Stop the tracks or stop the mediastream? Since > MediaStream::Stop is deprecated, we should at least not add new dependencies on > it. I think it does the right thing but you can verify: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w... https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/chr... chrome/browser/media/chrome_media_stream_infobar_browsertest.cc:167: // See http://crbug.com/263333. On 2014/05/05 07:28:25, perkj wrote: > Can this be enabled? I thought about it but decided to rather do that on a separate change that specifically addresses bug 263333. I haven't been able to repro the problem but then again I'm not specifically looking to fix it either (although I may have).
Driven-by. https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.cc:115: navigation_entry->SetExtraData(key, permissions); what happen if it sets the same entry to the navigation more than once? https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.cc:302: if (IsRequestAllowedByNavigationEntry(navigation_entry, request_)) { Does this navigation entry has higher priority than the media default setting ? For example, if the entry allows the permission, but users explicitly set the media default setting to "Do not allow sites to access your camera and microphone", the current code will allow the usage of the devices rather than denying. https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.cc:467: content::NavigationEntry* navigation_entry = Deny(false) can be triggered by clicking "X" button, is it intentional to clear the allowed permission?
Thanks Shijing - regarding drive-by comment, you're actually on the reviewer list :) https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.cc:115: navigation_entry->SetExtraData(key, permissions); On 2014/05/05 08:14:36, xians1 wrote: > what happen if it sets the same entry to the navigation more than once? Not sure if I understand the question, but if you think there's a bug in SetExtraData, check out the implementation: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.cc:302: if (IsRequestAllowedByNavigationEntry(navigation_entry, request_)) { On 2014/05/05 08:14:36, xians1 wrote: > Does this navigation entry has higher priority than the media default setting ? Permissions are stored with the navigation entry _after_ passing all checks, so no. > For example, if the entry allows the permission, but users explicitly set the > media default setting to "Do not allow sites to access your camera and > microphone", the current code will allow the usage of the devices rather than > denying. Nope. See MediaStreamDevicesController::Deny below. https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.cc:467: content::NavigationEntry* navigation_entry = On 2014/05/05 08:14:36, xians1 wrote: > Deny(false) can be triggered by clicking "X" button, is it intentional to clear > the allowed permission? Yes, intentional.
https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.cc:115: navigation_entry->SetExtraData(key, permissions); On 2014/05/05 08:28:53, tommi wrote: > On 2014/05/05 08:14:36, xians1 wrote: > > what happen if it sets the same entry to the navigation more than once? > > Not sure if I understand the question, but if you think there's a bug in > SetExtraData, check out the implementation: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... Thanks, I was just trying to understand the behavior, and wondering if the navigation would store different permissions to the same keys. The link answers the question with a "no" since it is a map. https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.cc:302: if (IsRequestAllowedByNavigationEntry(navigation_entry, request_)) { On 2014/05/05 08:28:53, tommi wrote: > On 2014/05/05 08:14:36, xians1 wrote: > > Does this navigation entry has higher priority than the media default setting > ? > > Permissions are stored with the navigation entry _after_ passing all checks, so > no. > > > For example, if the entry allows the permission, but users explicitly set the > > media default setting to "Do not allow sites to access your camera and > > microphone", the current code will allow the usage of the devices rather than > > denying. > > Nope. See MediaStreamDevicesController::Deny below. Then shouldn't these code be moved after IsDefaultMediaAccessBlocked(), which is 10 lines below? https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.cc:467: content::NavigationEntry* navigation_entry = On 2014/05/05 08:28:53, tommi wrote: > On 2014/05/05 08:14:36, xians1 wrote: > > Deny(false) can be triggered by clicking "X" button, is it intentional to > clear > > the allowed permission? > > Yes, intentional. Good.
https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.cc:115: navigation_entry->SetExtraData(key, permissions); On 2014/05/05 08:43:03, xians1 wrote: > On 2014/05/05 08:28:53, tommi wrote: > > On 2014/05/05 08:14:36, xians1 wrote: > > > what happen if it sets the same entry to the navigation more than once? > > > > Not sure if I understand the question, but if you think there's a bug in > > SetExtraData, check out the implementation: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > > Thanks, I was just trying to understand the behavior, and wondering if the > navigation would store different permissions to the same keys. The link answers > the question with a "no" since it is a map. Ah, ok, yeah. I wasn't sure what "entry to the navigation" meant since we're dealing with a navigation entry. https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.cc:302: if (IsRequestAllowedByNavigationEntry(navigation_entry, request_)) { On 2014/05/05 08:43:03, xians1 wrote: > On 2014/05/05 08:28:53, tommi wrote: > > On 2014/05/05 08:14:36, xians1 wrote: > > > Does this navigation entry has higher priority than the media default > setting > > ? > > > > Permissions are stored with the navigation entry _after_ passing all checks, > so > > no. > > > > > For example, if the entry allows the permission, but users explicitly set > the > > > media default setting to "Do not allow sites to access your camera and > > > microphone", the current code will allow the usage of the devices rather > than > > > denying. > > > > Nope. See MediaStreamDevicesController::Deny below. > > Then shouldn't these code be moved after IsDefaultMediaAccessBlocked(), which > is 10 lines below? No, it should be here. The permissions that are checked here will only be here after the whole list of permissions has been checked for the first request and only once all checks have been satisfied, do we actually store the permissions with the navigation entry. So, if we pass this check, it means that we have already passed IsDefaultMediaAccessBlocked and all the other checks.
https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.cc:302: if (IsRequestAllowedByNavigationEntry(navigation_entry, request_)) { On 2014/05/05 08:50:31, tommi wrote: > On 2014/05/05 08:43:03, xians1 wrote: > > On 2014/05/05 08:28:53, tommi wrote: > > > On 2014/05/05 08:14:36, xians1 wrote: > > > > Does this navigation entry has higher priority than the media default > > setting > > > ? > > > > > > Permissions are stored with the navigation entry _after_ passing all checks, > > so > > > no. > > > > > > > For example, if the entry allows the permission, but users explicitly set > > the > > > > media default setting to "Do not allow sites to access your camera and > > > > microphone", the current code will allow the usage of the devices rather > > than > > > > denying. > > > > > > Nope. See MediaStreamDevicesController::Deny below. > > > > Then shouldn't these code be moved after IsDefaultMediaAccessBlocked(), which > > is 10 lines below? > > No, it should be here. The permissions that are checked here will only be here > after the whole list of permissions has been checked for the first request and > only once all checks have been satisfied, do we actually store the permissions > with the navigation entry. > > So, if we pass this check, it means that we have already passed > IsDefaultMediaAccessBlocked and all the other checks. I think I got your point. But My question is about that the default setting is changed after the first request. Let me explain the use case in details: After the users approve the first request, users go to content setting and modify the default setting to "Do not allow sites to access your camera and microphone", the current code will allow the usage of the device since the navigation entry has the exception, even though the default setting is "Do not allow".
https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/262763004/diff/40001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.cc:302: if (IsRequestAllowedByNavigationEntry(navigation_entry, request_)) { On 2014/05/05 09:27:02, xians1 wrote: > On 2014/05/05 08:50:31, tommi wrote: > > On 2014/05/05 08:43:03, xians1 wrote: > > > On 2014/05/05 08:28:53, tommi wrote: > > > > On 2014/05/05 08:14:36, xians1 wrote: > > > > > Does this navigation entry has higher priority than the media default > > > setting > > > > ? > > > > > > > > Permissions are stored with the navigation entry _after_ passing all > checks, > > > so > > > > no. > > > > > > > > > For example, if the entry allows the permission, but users explicitly > set > > > the > > > > > media default setting to "Do not allow sites to access your camera and > > > > > microphone", the current code will allow the usage of the devices rather > > > than > > > > > denying. > > > > > > > > Nope. See MediaStreamDevicesController::Deny below. > > > > > > Then shouldn't these code be moved after IsDefaultMediaAccessBlocked(), > which > > > is 10 lines below? > > > > No, it should be here. The permissions that are checked here will only be > here > > after the whole list of permissions has been checked for the first request and > > only once all checks have been satisfied, do we actually store the permissions > > with the navigation entry. > > > > So, if we pass this check, it means that we have already passed > > IsDefaultMediaAccessBlocked and all the other checks. > > I think I got your point. > But My question is about that the default setting is changed after the first > request. > Let me explain the use case in details: > After the users approve the first request, users go to content setting and > modify the default setting to "Do not allow sites to access your camera and > microphone", the current code will allow the usage of the device since the > navigation entry has the exception, even though the default setting is "Do not > allow". > Ah, I see. Yes you're right. Thanks for catching that. Fixed.
Move IsRequestAllowedByNavigationEntry to after IsDefaultMediaAccessBlocked
On 2014/05/05 10:00:30, tommi wrote: > Move IsRequestAllowedByNavigationEntry to after IsDefaultMediaAccessBlocked lgtm, thanks.
The CQ bit was checked by tommi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommi@chromium.org/262763004/60001
Message was sent while issue was closed.
Change committed as 268187 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
