Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2254763002:
Enable the optional permission prompt persistence toggle on grouped infobars (Closed)
|
Created:
4 years, 4 months ago by dominickn Modified:
4 years, 3 months ago CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, dfalcantara+watch_chromium.org, Sergey Ulanov Base URL:
https://chromium.googlesource.com/chromium/src.git@permission-infobardelegate-clean Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable the optional permission prompt persistence toggle on grouped infobars
This CL adds support for the feature enabling a persistence toggle on
permission prompts to grouped permission infobars. The toggle is only
shown if an enabling feature is active. Grouped bars are used exclusively
for media permissions, so this change allows the toggle to be shown
when a single and multiple media permissions are requested.
New Android UI tests for responding to the permission prompts triggered
by getUserMedia are added to verify the toggle behaviour. The persistence
boolean in media_stream_devices_controller is renamed in preparation for
upstreaming it into PermissionRequest.
BUG=632269
Committed: https://crrev.com/fd008ac9abf59ac99681381b297e00652c285246
Cr-Commit-Position: refs/heads/master@{#418107}
Patch Set 1 #Patch Set 2 : Add test, plumb through mediastream #Patch Set 3 : No toggle on combined infobar #Patch Set 4 : Add metric logging #
Total comments: 8
Patch Set 5 : Addressing comments #Patch Set 6 : Fix uninitialized variable #Patch Set 7 : Add CL dependency; improve media impl #
Total comments: 12
Patch Set 8 : Remove dependency CL #Patch Set 9 : Rebase #Patch Set 10 : Rebase #
Total comments: 6
Patch Set 11 : Address nits #
Total comments: 2
Patch Set 12 : Fix typo #Patch Set 13 : Rebase #Patch Set 14 : DCHECK -> CHECK #Messages
Total messages: 107 (75 generated)
The CQ bit was checked by dominickn@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by dominickn@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. BUG=632269 ========== to ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active and there is only 1 permission displayed in the grouped bar. Grouped bars are used exclusively for media permissions; when single media permissions are requested, a grouped bar class is still used. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. BUG=632269 ==========
dominickn@chromium.org changed reviewers: + dfalcantara@chromium.org, raymes@chromium.org
PTAL, thanks!
The CQ bit was checked by dominickn@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: This issue passed the CQ dry run.
permissions/ looks pretty good. A few comments https://codereview.chromium.org/2254763002/diff/60001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2254763002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:260: MediaStreamDevicesController::GetPermissionTypeForContentSettingsType( We might as well use GetPermissionType from permission_util https://codereview.chromium.org/2254763002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:490: UpdateTabSpecificContentSettings(audio_setting, video_setting); Hmm, I'm worried that updating the tab context is important even if the setting isn't being persisted.. I'm a bit surprised that this doesn't cause something to break. https://codereview.chromium.org/2254763002/diff/60001/chrome/browser/media/me... File chrome/browser/media/media_stream_infobar_delegate_android.cc (right): https://codereview.chromium.org/2254763002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_infobar_delegate_android.cc:129: GetContentSettingType(0)), How come we only deal with the case when one permission is shown? Will that limit the usefulness? What were the complications with multiple?
The CQ bit was checked by dominickn@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...
Thanks! https://codereview.chromium.org/2254763002/diff/60001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2254763002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:260: MediaStreamDevicesController::GetPermissionTypeForContentSettingsType( On 2016/08/18 06:10:00, raymes wrote: > We might as well use GetPermissionType from permission_util Done. https://codereview.chromium.org/2254763002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:490: UpdateTabSpecificContentSettings(audio_setting, video_setting); On 2016/08/18 06:10:00, raymes wrote: > Hmm, I'm worried that updating the tab context is important even if the setting > isn't being persisted.. I'm a bit surprised that this doesn't cause something to > break. I'm not sure I know this code well enough to make changes here, but I've changed it to follow your suggestion. WDYT? https://codereview.chromium.org/2254763002/diff/60001/chrome/browser/media/me... File chrome/browser/media/media_stream_infobar_delegate_android.cc (right): https://codereview.chromium.org/2254763002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_infobar_delegate_android.cc:129: GetContentSettingType(0)), On 2016/08/18 06:10:01, raymes wrote: > How come we only deal with the case when one permission is shown? Will that > limit the usefulness? What were the complications with multiple? As discussed, enabling for multiple.
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_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by dominickn@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: This issue passed the CQ dry run.
lgtm
Description was changed from ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active and there is only 1 permission displayed in the grouped bar. Grouped bars are used exclusively for media permissions; when single media permissions are requested, a grouped bar class is still used. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. BUG=632269 ========== to ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. BUG=632269 ==========
dominickn@chromium.org changed reviewers: + sergeyu@chromium.org
Thanks! +sergeyu: PTAL at chrome/browser/media.
The CQ bit was checked by dominickn@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...
On 2016/08/18 23:29:50, dominickn wrote: > Thanks! > > +sergeyu: PTAL at chrome/browser/media. FYI sergeyu: this CL now depends on crrev.com/2258763002, which I've also added you to.
The CQ bit was checked by dominickn@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: This issue passed the CQ dry run.
lgtm My guess is that mic/camera behave differently than geolocation for the not persisting case. I believe it will ask again for each request rather than caching the decision. It might be worth testing on a site like this: https://webrtc.github.io/samples/src/content/devices/input-output/ https://codereview.chromium.org/2254763002/diff/60001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2254763002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:260: MediaStreamDevicesController::GetPermissionTypeForContentSettingsType( On 2016/08/18 06:38:20, dominickn wrote: > On 2016/08/18 06:10:00, raymes wrote: > > We might as well use GetPermissionType from permission_util > > Done. I think we should just remove this and use PermissionUtil::GetPermissionType directly in the callsites. https://codereview.chromium.org/2254763002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/MediaPermissionsTest.java (right): https://codereview.chromium.org/2254763002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/MediaPermissionsTest.java:188: * switch appears and that permission is granted with it toggled of. nit: off https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:478: if (persist()) I don't see persist/set_persist defined anywhere. https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:480: UpdateTabSpecificContentSettings(audio_setting, video_setting); I'm not sure this should actually be in this block either, though it's not related to your CL. Perhaps kcarattini knows.
dominickn@chromium.org changed reviewers: + kcarattini@chromium.org
Thanks! +kcarattini: do you know the answer to raymes' question? If not I won't change the line there from what I have. https://codereview.chromium.org/2254763002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/MediaPermissionsTest.java (right): https://codereview.chromium.org/2254763002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/MediaPermissionsTest.java:188: * switch appears and that permission is granted with it toggled of. On 2016/08/22 04:37:19, raymes wrote: > nit: off Acknowledged, will fix. https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:478: if (persist()) On 2016/08/22 04:37:19, raymes wrote: > I don't see persist/set_persist defined anywhere. It's defined in PermissionRequest. https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:480: UpdateTabSpecificContentSettings(audio_setting, video_setting); On 2016/08/22 04:37:19, raymes wrote: > I'm not sure this should actually be in this block either, though it's not > related to your CL. Perhaps kcarattini knows. Acknowledged. +kcarattini to look at.
https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:478: if (persist()) On 2016/08/22 05:30:02, dominickn wrote: > On 2016/08/22 04:37:19, raymes wrote: > > I don't see persist/set_persist defined anywhere. > > It's defined in PermissionRequest. Oh..it's in the dependent CL. I feel like it could be better to pass this to PermissionGranted/PermissionDenied since it's a bit harder to reason about the state otherwise. At the very least we should ensure that set_persist is only ever called once.
https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:480: UpdateTabSpecificContentSettings(audio_setting, video_setting); On 2016/08/22 05:30:02, dominickn wrote: > On 2016/08/22 04:37:19, raymes wrote: > > I'm not sure this should actually be in this block either, though it's not > > related to your CL. Perhaps kcarattini knows. > > Acknowledged. +kcarattini to look at. This was done to suppress the Page Action icon from displaying when the kill switch icon. This is consistent with the other permissions, but may not be 100% correct if the function now does other things. That being said, the kill switch is only on if there is a major API vulnerability, so this would not be the normal everyday state.
https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:478: if (persist()) On 2016/08/22 05:35:23, raymes wrote: > On 2016/08/22 05:30:02, dominickn wrote: > > On 2016/08/22 04:37:19, raymes wrote: > > > I don't see persist/set_persist defined anywhere. > > > > It's defined in PermissionRequest. > > Oh..it's in the dependent CL. I feel like it could be better to pass this to > PermissionGranted/PermissionDenied since it's a bit harder to reason about the > state otherwise. At the very least we should ensure that set_persist is only > ever called once. I considered that. The problem would be that PermissionRequest has many subclasses, each of which would need to take the persist boolean as an argument, and ignore it. I talked this over with tsergeant and we agreed that solution was worse because it introduced an otherwise ignored parameter to the download request limiter request, the protocol request, etc. WDYT? I don't really mind either way, but I feel like the way I've done it is slightly nicer. The media stream device controller may set the variable multiple times (see ForceDeniedTemporarily in the dependency CL for this patchset), so I don't think logic to make sure set_persist is set once is appropriate. https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:480: UpdateTabSpecificContentSettings(audio_setting, video_setting); On 2016/08/22 05:44:38, kcarattini wrote: > On 2016/08/22 05:30:02, dominickn wrote: > > On 2016/08/22 04:37:19, raymes wrote: > > > I'm not sure this should actually be in this block either, though it's not > > > related to your CL. Perhaps kcarattini knows. > > > > Acknowledged. +kcarattini to look at. > > This was done to suppress the Page Action icon from displaying when the kill > switch icon. This is consistent with the other permissions, but may not be 100% > correct if the function now does other things. That being said, the kill switch > is only on if there is a major API vulnerability, so this would not be the > normal everyday state. Acknowledged.
https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:478: if (persist()) On 2016/08/22 06:00:15, dominickn wrote: > On 2016/08/22 05:35:23, raymes wrote: > > On 2016/08/22 05:30:02, dominickn wrote: > > > On 2016/08/22 04:37:19, raymes wrote: > > > > I don't see persist/set_persist defined anywhere. > > > > > > It's defined in PermissionRequest. > > > > Oh..it's in the dependent CL. I feel like it could be better to pass this to > > PermissionGranted/PermissionDenied since it's a bit harder to reason about the > > state otherwise. At the very least we should ensure that set_persist is only > > ever called once. > > I considered that. The problem would be that PermissionRequest has many > subclasses, each of which would need to take the persist boolean as an argument, > and ignore it. I talked this over with tsergeant and we agreed that solution was > worse because it introduced an otherwise ignored parameter to the download > request limiter request, the protocol request, etc. > > WDYT? I don't really mind either way, but I feel like the way I've done it is > slightly nicer. I think that's a reasonable argument. My main concern would be that it is meaningless for other subclasses. I haven't looked closely at the dependent CL. I think we could untangle the two by just putting the persist_ variable inside the MediaStreamDevicesController for now and then we could look at the larger picture in the desktop CL. That might help getting this landed quicker. Do you think that's possible and makes sense? > The media stream device controller may set the variable multiple > times (see ForceDeniedTemporarily in the dependency CL for this patchset), so I > don't think logic to make sure set_persist is set once is appropriate. The invariant that I was thinking of is that set_persist should never be called after Accept/Deny or Granted/Denied because it won't do anything. We don't have to add it in this CL but I think it could be a good sanity check.
https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:478: if (persist()) On 2016/08/22 06:59:11, raymes wrote: > On 2016/08/22 06:00:15, dominickn wrote: > > On 2016/08/22 05:35:23, raymes wrote: > > > On 2016/08/22 05:30:02, dominickn wrote: > > > > On 2016/08/22 04:37:19, raymes wrote: > > > > > I don't see persist/set_persist defined anywhere. > > > > > > > > It's defined in PermissionRequest. > > > > > > Oh..it's in the dependent CL. I feel like it could be better to pass this to > > > PermissionGranted/PermissionDenied since it's a bit harder to reason about > the > > > state otherwise. At the very least we should ensure that set_persist is only > > > ever called once. > > > > I considered that. The problem would be that PermissionRequest has many > > subclasses, each of which would need to take the persist boolean as an > argument, > > and ignore it. I talked this over with tsergeant and we agreed that solution > was > > worse because it introduced an otherwise ignored parameter to the download > > request limiter request, the protocol request, etc. > > > > WDYT? I don't really mind either way, but I feel like the way I've done it is > > slightly nicer. > > I think that's a reasonable argument. My main concern would be that it is > meaningless for other subclasses. > > I haven't looked closely at the dependent CL. I think we could untangle the two > by just putting the persist_ variable inside the MediaStreamDevicesController > for now and then we could look at the larger picture in the desktop CL. That > might help getting this landed quicker. Do you think that's possible and makes > sense? MediaStreamDevicesController currently has the persist boolean (it's called persist_permission_changes_). The CL that this depends on upstreams that boolean (and renames it to persist_) into PermissionRequest because PermissionManager operates over a list of PermissionRequest objects; therefore passing persist_ must be somewhere on PermissionRequest or else I don't think we can push the persistence state up from the UI code to the permission manager. For instance, geolocation permissions use PermissionRequestImpl, not MediaStreamDevicesController. Both get routed via PermissionManager through the PermissionRequest base class. The two options were PermissionGranted(bool) and PermissionDenied(bool), which has the drawbacks I previously mentioned, or adding the boolean and a setter to PermissionRequest, which contains the setting more cleanly. > > The media stream device controller may set the variable multiple > > times (see ForceDeniedTemporarily in the dependency CL for this patchset), so > I > > don't think logic to make sure set_persist is set once is appropriate. > > The invariant that I was thinking of is that set_persist should never be called > after Accept/Deny or Granted/Denied because it won't do anything. We don't have > to add it in this CL but I think it could be a good sanity check. Thanks for clarifying this; I'll see what I can do.
On 2016/08/22 07:08:44, dominickn wrote: > https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... > File chrome/browser/media/media_stream_devices_controller.cc (right): > > https://codereview.chromium.org/2254763002/diff/120001/chrome/browser/media/m... > chrome/browser/media/media_stream_devices_controller.cc:478: if (persist()) > On 2016/08/22 06:59:11, raymes wrote: > > On 2016/08/22 06:00:15, dominickn wrote: > > > On 2016/08/22 05:35:23, raymes wrote: > > > > On 2016/08/22 05:30:02, dominickn wrote: > > > > > On 2016/08/22 04:37:19, raymes wrote: > > > > > > I don't see persist/set_persist defined anywhere. > > > > > > > > > > It's defined in PermissionRequest. > > > > > > > > Oh..it's in the dependent CL. I feel like it could be better to pass this > to > > > > PermissionGranted/PermissionDenied since it's a bit harder to reason about > > the > > > > state otherwise. At the very least we should ensure that set_persist is > only > > > > ever called once. > > > > > > I considered that. The problem would be that PermissionRequest has many > > > subclasses, each of which would need to take the persist boolean as an > > argument, > > > and ignore it. I talked this over with tsergeant and we agreed that solution > > was > > > worse because it introduced an otherwise ignored parameter to the download > > > request limiter request, the protocol request, etc. > > > > > > WDYT? I don't really mind either way, but I feel like the way I've done it > is > > > slightly nicer. > > > > I think that's a reasonable argument. My main concern would be that it is > > meaningless for other subclasses. > > > > I haven't looked closely at the dependent CL. I think we could untangle the > two > > by just putting the persist_ variable inside the MediaStreamDevicesController > > for now and then we could look at the larger picture in the desktop CL. That > > might help getting this landed quicker. Do you think that's possible and makes > > sense? > > MediaStreamDevicesController currently has the persist boolean (it's called > persist_permission_changes_). The CL that this depends on upstreams that boolean > (and renames it to persist_) into PermissionRequest because PermissionManager > operates over a list of PermissionRequest objects; therefore passing persist_ > must be somewhere on PermissionRequest or else I don't think we can push the > persistence state up from the UI code to the permission manager. For instance, > geolocation permissions use PermissionRequestImpl, not > MediaStreamDevicesController. Both get routed via PermissionManager through the > PermissionRequest base class. > > The two options were PermissionGranted(bool) and PermissionDenied(bool), which > has the drawbacks I previously mentioned, or adding the boolean and a setter to > PermissionRequest, which contains the setting more cleanly. > But that's only needed for desktop right? Couldn't we just keep persist_ in the MediaStreamDevicesController for now and land this and have it working for Android? Sorry if I'm missing something obvious! > > > The media stream device controller may set the variable multiple > > > times (see ForceDeniedTemporarily in the dependency CL for this patchset), > so > > I > > > don't think logic to make sure set_persist is set once is appropriate. > > > > The invariant that I was thinking of is that set_persist should never be > called > > after Accept/Deny or Granted/Denied because it won't do anything. We don't > have > > to add it in this CL but I think it could be a good sanity check. > > Thanks for clarifying this; I'll see what I can do.
Both media reviewers seem to be OOO, so I don't think this is going to make branch point now. It looks like I'll have to get it reviewed after branch and merge it.
dominickn@chromium.org changed reviewers: + xhwang@chromium.org - sergeyu@chromium.org
Description was changed from ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. BUG=632269 ========== to ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 ==========
The CQ bit was checked by dominickn@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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dominickn@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dominickn@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...
+xhwang - can you look at chrome/browser/media? Both sergeyu and tommi are OOO.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rs lgtm with nits https://codereview.chromium.org/2254763002/diff/180001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.h (right): https://codereview.chromium.org/2254763002/diff/180001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.h:46: // ContentSettingsType. |type| must be a media stream type. s/|type|/|content_type|/ ? https://codereview.chromium.org/2254763002/diff/180001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.h:144: bool persist_; nit: I found the old name is easier to understand. Same for the setter/getter function names. Also you use |persist_permission| for the local variables. It'd be nice if we can have a consistent more meaningful name everywhere. https://codereview.chromium.org/2254763002/diff/180001/chrome/browser/media/m... File chrome/browser/media/media_stream_infobar_delegate_android.cc (right): https://codereview.chromium.org/2254763002/diff/180001/chrome/browser/media/m... chrome/browser/media/media_stream_infobar_delegate_android.cc:114: persist_permission); Can you have a helper function? This code is a bit hard to read. void SomeName(position, persist_permission) { PermissionUmaUtil::PermissionPromptAcceptedWithPersistenceToggle( controller_->GetPermissionTypeForContentSettingsType( GetContentSettingType(kGroupedInfobarAudioPosition)), persist_permission); }
The CQ bit was checked by dominickn@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...
Thanks! https://codereview.chromium.org/2254763002/diff/60001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2254763002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:260: MediaStreamDevicesController::GetPermissionTypeForContentSettingsType( On 2016/08/22 04:37:19, raymes wrote: > On 2016/08/18 06:38:20, dominickn wrote: > > On 2016/08/18 06:10:00, raymes wrote: > > > We might as well use GetPermissionType from permission_util > > > > Done. > > I think we should just remove this and use PermissionUtil::GetPermissionType > directly in the callsites. GetPermissionType's signature is a little awkward; wrapping it in this method makes it quite a bit nicer. https://codereview.chromium.org/2254763002/diff/180001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.h (right): https://codereview.chromium.org/2254763002/diff/180001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.h:46: // ContentSettingsType. |type| must be a media stream type. On 2016/08/23 18:03:58, xhwang (slow) wrote: > s/|type|/|content_type|/ ? Done. https://codereview.chromium.org/2254763002/diff/180001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.h:144: bool persist_; On 2016/08/23 18:03:58, xhwang (slow) wrote: > nit: I found the old name is easier to understand. Same for the setter/getter > function names. Also you use |persist_permission| for the local variables. It'd > be nice if we can have a consistent more meaningful name everywhere. This boolean is called |persist| throughout the permissions stack, which is why I renamed it like this. The follow up CL will actually remove the variable from this location and upstream it to the parent PermissionRequest class. Given the context of PermissionRequest, I think the |persist| naming makes sense and will be consistent. :) https://codereview.chromium.org/2254763002/diff/180001/chrome/browser/media/m... File chrome/browser/media/media_stream_infobar_delegate_android.cc (right): https://codereview.chromium.org/2254763002/diff/180001/chrome/browser/media/m... chrome/browser/media/media_stream_infobar_delegate_android.cc:114: persist_permission); On 2016/08/23 18:03:58, xhwang (slow) wrote: > Can you have a helper function? This code is a bit hard to read. > > void SomeName(position, persist_permission) { > PermissionUmaUtil::PermissionPromptAcceptedWithPersistenceToggle( > controller_->GetPermissionTypeForContentSettingsType( > GetContentSettingType(kGroupedInfobarAudioPosition)), > persist_permission); > } Done.
LGTM % typo https://codereview.chromium.org/2254763002/diff/200001/chrome/browser/media/m... File chrome/browser/media/media_stream_infobar_delegate_android.h (right): https://codereview.chromium.org/2254763002/diff/200001/chrome/browser/media/m... chrome/browser/media/media_stream_infobar_delegate_android.h:38: void RecordPermissionAcceptedUma(int psition, bool persist); position
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dominickn@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...
Oops, thanks. https://codereview.chromium.org/2254763002/diff/200001/chrome/browser/media/m... File chrome/browser/media/media_stream_infobar_delegate_android.h (right): https://codereview.chromium.org/2254763002/diff/200001/chrome/browser/media/m... chrome/browser/media/media_stream_infobar_delegate_android.h:38: void RecordPermissionAcceptedUma(int psition, bool persist); On 2016/08/23 20:27:43, xhwang (slow) wrote: > position Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org, raymes@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2254763002/#ps220001 (title: "Fix typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 ========== to ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 ========== to ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 Committed: https://crrev.com/fea9c39c10d391e8b03b77b39b889c54a96a9b5c Cr-Commit-Position: refs/heads/master@{#413970} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/fea9c39c10d391e8b03b77b39b889c54a96a9b5c Cr-Commit-Position: refs/heads/master@{#413970}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/2277733002/ by bauerb@chromium.org. The reason for reverting is: Breaks on Android testers (https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test...) BUG=640580.
Message was sent while issue was closed.
Description was changed from ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 Committed: https://crrev.com/fea9c39c10d391e8b03b77b39b889c54a96a9b5c Cr-Commit-Position: refs/heads/master@{#413970} ========== to ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 Committed: https://crrev.com/fea9c39c10d391e8b03b77b39b889c54a96a9b5c Cr-Commit-Position: refs/heads/master@{#413970} ==========
On 2016/08/24 11:48:27, Bernhard (OOO until Sep 7) wrote: > A revert of this CL (patchset #12 id:220001) has been created in > https://codereview.chromium.org/2277733002/ by mailto:bauerb@chromium.org. > > The reason for reverting is: Breaks on Android testers > (https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test...) > > BUG=640580. It looks like this bounced because of a bug in Android swarming versus non swarming (crbug.com/641477). I am going to try landing it again, possibly with the offending tests disabled until the bug can be sorted out.
The CQ bit was checked by dominickn@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: This issue passed the CQ dry run.
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, raymes@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2254763002/#ps240001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 Committed: https://crrev.com/fea9c39c10d391e8b03b77b39b889c54a96a9b5c Cr-Commit-Position: refs/heads/master@{#413970} ========== to ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 Committed: https://crrev.com/fea9c39c10d391e8b03b77b39b889c54a96a9b5c Cr-Commit-Position: refs/heads/master@{#413970} ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 Committed: https://crrev.com/fea9c39c10d391e8b03b77b39b889c54a96a9b5c Cr-Commit-Position: refs/heads/master@{#413970} ========== to ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 Committed: https://crrev.com/fea9c39c10d391e8b03b77b39b889c54a96a9b5c Committed: https://crrev.com/c23b2a5192ee77dd842b3e79575d586743364b32 Cr-Original-Commit-Position: refs/heads/master@{#413970} Cr-Commit-Position: refs/heads/master@{#417870} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/c23b2a5192ee77dd842b3e79575d586743364b32 Cr-Commit-Position: refs/heads/master@{#417870}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2332663002/ by dominickn@chromium.org. The reason for reverting is: This still breaks the Android Tests bot. https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test....
Message was sent while issue was closed.
Description was changed from ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 Committed: https://crrev.com/fea9c39c10d391e8b03b77b39b889c54a96a9b5c Committed: https://crrev.com/c23b2a5192ee77dd842b3e79575d586743364b32 Cr-Original-Commit-Position: refs/heads/master@{#413970} Cr-Commit-Position: refs/heads/master@{#417870} ========== to ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 ==========
The CQ bit was checked by dominickn@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: This issue passed the CQ dry run.
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, raymes@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2254763002/#ps260001 (title: "DCHECK -> CHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 ========== to ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 ========== to ========== Enable the optional permission prompt persistence toggle on grouped infobars This CL adds support for the feature enabling a persistence toggle on permission prompts to grouped permission infobars. The toggle is only shown if an enabling feature is active. Grouped bars are used exclusively for media permissions, so this change allows the toggle to be shown when a single and multiple media permissions are requested. New Android UI tests for responding to the permission prompts triggered by getUserMedia are added to verify the toggle behaviour. The persistence boolean in media_stream_devices_controller is renamed in preparation for upstreaming it into PermissionRequest. BUG=632269 Committed: https://crrev.com/fd008ac9abf59ac99681381b297e00652c285246 Cr-Commit-Position: refs/heads/master@{#418107} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/fd008ac9abf59ac99681381b297e00652c285246 Cr-Commit-Position: refs/heads/master@{#418107} |
