Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1362803002:
Add logic to resolve permission mismatches in Android M for webrtc. (Closed)
|
Created:
5 years, 3 months ago by Ted C Modified:
4 years, 3 months ago Reviewers:
calamity, Peter Kasting, mlamouri (slow - plz ping), Guido Urdaneta, dzhioev (left Google), perkj_chrome, tommi (sloooow) - chröme, ddorwin CC:
chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, mlamouri+watch-permissions_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd logic to resolve permission mismatches in Android M for webrtc.
BUG=499393
Committed: https://crrev.com/d3cfebe04af22eed762bd9c44f63c1554d424fec
Cr-Commit-Position: refs/heads/master@{#351425}
Patch Set 1 #Patch Set 2 : Add comment in media_stream_devices_controller #
Total comments: 7
Patch Set 3 : Address comments and fix declining #
Total comments: 8
Patch Set 4 : Rebased #Patch Set 5 : Address Mounir's comments #Patch Set 6 : Fix StartPageService #Patch Set 7 : Fix ChromeOS WebUI login usage #Patch Set 8 : Rebased #
Total comments: 2
Patch Set 9 : Rebased #Patch Set 10 : Add blankline #Messages
Total messages: 53 (21 generated)
tedchoc@chromium.org changed reviewers: + mlamouri@chromium.org, tommi@chromium.org
PTAL tommi - media* mlamouri - permissions*
tommi@chromium.org changed reviewers: + perkj@chromium.org
Per - do you mind taking a look?
perkj@chromium.org changed reviewers: + guidou@chromium.org
Sorry, I can't say I know this code. guidou?
lgtm % nits https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:100: if (old_audio_setting_ == CONTENT_SETTING_ALLOW) Shouldn't you use IsAllowedForAudio() and IsAllowedForVideo()? https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_update_infobar_delegate_android.cc (right): https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_update_infobar_delegate_android.cc:79: PermissionUpdatedCallback cb = callback_; Can you use base::ResetAndReturn() here? https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_update_infobar_delegate_android.cc:176: PermissionUpdatedCallback cb = callback_; base::ResetAndReturn()
I ended up needing to add a new method to the controller to force disable the permissions regardless of the original state. Cancelled would use the original values, which in this case would be ALLOWED. I want to send denied to the callback w/o changing the site settings. Also, the existing permissiondenied call only sets denied if the original value was ASK, and in this case it is ALLOW for the site but Chrome itself is missing the required permission. So as my rambling implies, I'd like another quick pass if you don't mind :-) https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:100: if (old_audio_setting_ == CONTENT_SETTING_ALLOW) On 2015/09/23 13:32:14, Guido Urdaneta wrote: > Shouldn't you use IsAllowedForAudio() and IsAllowedForVideo()? Done. https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_update_infobar_delegate_android.cc (right): https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_update_infobar_delegate_android.cc:79: PermissionUpdatedCallback cb = callback_; On 2015/09/23 13:32:14, Guido Urdaneta wrote: > Can you use base::ResetAndReturn() here? Neat! Done
lgtm https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:105: if (IsAllowedForVideo()) style: I believe you need to have { } if the next line wraps https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/pe... File chrome/browser/media/permission_bubble_media_access_handler.cc (right): https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/pe... chrome/browser/media/permission_bubble_media_access_handler.cc:137: if (controller->IsAllowedForAudio() || controller->IsAllowedForVideo()) { Maybe you could remove that and simply check if the |content_settings_types| vector is empty? https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/pe... chrome/browser/media/permission_bubble_media_access_handler.cc:141: if (controller->IsAllowedForVideo()) style: as above https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/pe... chrome/browser/media/permission_bubble_media_access_handler.cc:164: // when we've transitioned to bubbles. (crbug/337458) nit: as long as you move that, could you add https:// in front of the link?
https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:105: if (IsAllowedForVideo()) On 2015/09/24 10:57:02, Mounir Lamouri wrote: > style: I believe you need to have { } if the next line wraps Done. https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/pe... File chrome/browser/media/permission_bubble_media_access_handler.cc (right): https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/pe... chrome/browser/media/permission_bubble_media_access_handler.cc:137: if (controller->IsAllowedForAudio() || controller->IsAllowedForVideo()) { On 2015/09/24 10:57:02, Mounir Lamouri wrote: > Maybe you could remove that and simply check if the |content_settings_types| > vector is empty? Done. https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/pe... chrome/browser/media/permission_bubble_media_access_handler.cc:141: if (controller->IsAllowedForVideo()) On 2015/09/24 10:57:02, Mounir Lamouri wrote: > style: as above Done. https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/pe... chrome/browser/media/permission_bubble_media_access_handler.cc:164: // when we've transitioned to bubbles. (crbug/337458) On 2015/09/24 10:57:02, Mounir Lamouri wrote: > nit: as long as you move that, could you add https:// in front of the link? Done.
On 2015/09/24 22:07:27, Ted C wrote: > https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/me... > File chrome/browser/media/media_stream_devices_controller.cc (right): > > https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/me... > chrome/browser/media/media_stream_devices_controller.cc:105: if > (IsAllowedForVideo()) > On 2015/09/24 10:57:02, Mounir Lamouri wrote: > > style: I believe you need to have { } if the next line wraps > > Done. > > https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/pe... > File chrome/browser/media/permission_bubble_media_access_handler.cc (right): > > https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/pe... > chrome/browser/media/permission_bubble_media_access_handler.cc:137: if > (controller->IsAllowedForAudio() || controller->IsAllowedForVideo()) { > On 2015/09/24 10:57:02, Mounir Lamouri wrote: > > Maybe you could remove that and simply check if the |content_settings_types| > > vector is empty? > > Done. > > https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/pe... > chrome/browser/media/permission_bubble_media_access_handler.cc:141: if > (controller->IsAllowedForVideo()) > On 2015/09/24 10:57:02, Mounir Lamouri wrote: > > style: as above > > Done. > > https://codereview.chromium.org/1362803002/diff/40001/chrome/browser/media/pe... > chrome/browser/media/permission_bubble_media_access_handler.cc:164: // when > we've transitioned to bubbles. (crbug/337458) > On 2015/09/24 10:57:02, Mounir Lamouri wrote: > > nit: as long as you move that, could you add https:// in front of the link? > > Done. @guidou - do you want to take another look?
The CQ bit was checked by tedchoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1362803002/#ps80001 (title: "Address Mounir's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1362803002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1362803002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
tedchoc@chromium.org changed reviewers: + calamity@chromium.org
+calamity for start_page_service.cc
The CQ bit was checked by tedchoc@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/1362803002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1362803002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tedchoc@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/1362803002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1362803002/120001
tedchoc@chromium.org changed reviewers: + dzhioev@chromium.org
+dzhioev for chromeos/login/ui/webui_login_view.cc Now I think I finally found all usages. Sorry for the spam.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
start_page_service.cc lgtm
On 2015/09/26 00:21:01, Ted C wrote: > +dzhioev for chromeos/login/ui/webui_login_view.cc > > Now I think I finally found all usages. Sorry for the spam. chromeos/login/ui/webui_login_view.cc LGTM
The CQ bit was checked by tedchoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1362803002/#ps120001 (title: "Fix ChromeOS WebUI login usage")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1362803002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1362803002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
tedchoc@chromium.org changed reviewers: + ddorwin@chromium.org
tommi|ddorwin -- need media OWNERS stamp
rs lgtm https://codereview.chromium.org/1362803002/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1362803002/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:105: if (IsAllowedForVideo()) { nit: empty line above this one for readability.
https://codereview.chromium.org/1362803002/diff/140001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1362803002/diff/140001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:105: if (IsAllowedForVideo()) { On 2015/09/29 14:49:48, tommi wrote: > nit: empty line above this one for readability. Done.
The CQ bit was checked by tedchoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from calamity@chromium.org, mlamouri@chromium.org, guidou@chromium.org, dzhioev@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1362803002/#ps180001 (title: "Add blankline")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1362803002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1362803002/180001
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tedchoc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1362803002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1362803002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/d3cfebe04af22eed762bd9c44f63c1554d424fec Cr-Commit-Position: refs/heads/master@{#351425}
Message was sent while issue was closed.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_update_infobar_delegate_android.cc (right): https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_update_infobar_delegate_android.cc:79: PermissionUpdatedCallback cb = callback_; On 2015/09/23 19:45:34, Ted C wrote: > On 2015/09/23 13:32:14, Guido Urdaneta wrote: > > Can you use base::ResetAndReturn() here? > > Neat! Done This change got referenced in another review I was doing for some Android infobar code. Why is it important to save off the callback before calling it (i.e. using ResetAndReturn() instead of just a direct Run())? This is subtle, and (1) Other people are copying this without it being clear to me that they need it, and (2) I'm not sure how to _be_ clear about when it's needed Understanding why you were doing this would help me.
Message was sent while issue was closed.
https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_update_infobar_delegate_android.cc (right): https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_update_infobar_delegate_android.cc:79: PermissionUpdatedCallback cb = callback_; On 2016/09/21 21:33:44, Peter Kasting wrote: > On 2015/09/23 19:45:34, Ted C wrote: > > On 2015/09/23 13:32:14, Guido Urdaneta wrote: > > > Can you use base::ResetAndReturn() here? > > > > Neat! Done > > This change got referenced in another review I was doing for some Android > infobar code. > > Why is it important to save off the callback before calling it (i.e. using > ResetAndReturn() instead of just a direct Run())? This is subtle, and > > (1) Other people are copying this without it being clear to me that they need > it, and > (2) I'm not sure how to _be_ clear about when it's needed > > Understanding why you were doing this would help me. To be clear: I understand why you want to reset the callback, so it doesn't run in the destructor. What I _don't_ understand is why the order needs to be "reset first, then run" instead of just: callback_.Run(); callback_.Reset(); Can |callback_| destroy |this|? But if it could, then the RemoveSelf() call wouldn't be safe either. So I don't understand why the hoop-jumping.
Message was sent while issue was closed.
On 2016/09/21 21:37:08, Peter Kasting wrote: > https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/permissi... > File chrome/browser/permissions/permission_update_infobar_delegate_android.cc > (right): > > https://codereview.chromium.org/1362803002/diff/20001/chrome/browser/permissi... > chrome/browser/permissions/permission_update_infobar_delegate_android.cc:79: > PermissionUpdatedCallback cb = callback_; > On 2016/09/21 21:33:44, Peter Kasting wrote: > > On 2015/09/23 19:45:34, Ted C wrote: > > > On 2015/09/23 13:32:14, Guido Urdaneta wrote: > > > > Can you use base::ResetAndReturn() here? > > > > > > Neat! Done > > > > This change got referenced in another review I was doing for some Android > > infobar code. > > > > Why is it important to save off the callback before calling it (i.e. using > > ResetAndReturn() instead of just a direct Run())? This is subtle, and > > > > (1) Other people are copying this without it being clear to me that they need > > it, and > > (2) I'm not sure how to _be_ clear about when it's needed > > > > Understanding why you were doing this would help me. > > To be clear: I understand why you want to reset the callback, so it doesn't run > in the destructor. What I _don't_ understand is why the order needs to be > "reset first, then run" instead of just: > > callback_.Run(); > callback_.Reset(); > > Can |callback_| destroy |this|? But if it could, then the RemoveSelf() call > wouldn't be safe either. So I don't understand why the hoop-jumping. I don't recall to be honest. I wouldn't be surprised if I just copied the existing callback semantics in: https://codereview.chromium.org/1362803002/diff/180001/chrome/browser/media/m... If anything, I like that ResetAndReturn is on a single line. But if using it picks up unintended behavior baggage, then that isn't desired either. But then again, I haven't looked at this code in a year, so I also can't guarantee that under some circumstances this was needed to avoid a double callback trigger (although in my brief glance it didn't seem possible). |
