Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808003002:
Start wiring up modal dialog experiment in PermissionRequestManager codepath (Closed)
|
Created:
3 years, 8 months ago by Timothy Loh Modified:
3 years, 6 months ago Reviewers:
raymes CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionStart wiring up modal dialog experiment in PermissionRequestManager codepath
This patch starts adding support for the modal dialog experiment when
the PermissionRequestManager is enabled on Android. To keep things
simple we re-use the existing PermissionDialogDelegate class and allow
it to now operate on a PermissionPromptAndroid.
There are several pieces that still need to be implemented in subsequent
patches. The string used in the dialog is currently the wrong string
(we use the fragment, e.g. "Know your location" instead of a full
sentence "permission.site wants to use your device's location.".
The link text (learn more) for EME needs to be hooked up, as well as
making the toggle experiment work, and supporting grouped camera +
microphone permissions.
BUG=606138
Review-Url: https://codereview.chromium.org/2808003002
Cr-Commit-Position: refs/heads/master@{#475422}
Committed: https://chromium.googlesource.com/chromium/src/+/ee625a76bb992853cd9a73625f25deeecfeb0d83
Patch Set 1 #Patch Set 2 : upd comment #
Total comments: 16
Patch Set 3 : rebase / address comments / etc #Patch Set 4 : rebase more #
Total comments: 4
Patch Set 5 : address comments #Patch Set 6 : fix bad rebase #
Dependent Patchsets: Messages
Total messages: 25 (16 generated)
The CQ bit was checked by timloh@chromium.org to run a CQ dry run
Description was changed from ========== Start wiring up modal dialog experiment in PermissionRequestManager codepath This patch starts adding support for the modal dialog experiment when the PermissionRequestManager is enabled on Android. To keep things simple we re-use the existing PermissionDialogDelegate class and allow it to now operate on a PermissionPromptAndroid. There are several pieces that still need to be implemented in subsequent patches. The string used in the dialog is currently the wrong string (we use the fragment, e.g. "Know your location" instead of a full sentence "permission.site wants to use your device's location.". Navigating while a dialog is open (e.g. a JS timer, see bug 699851) is still broken, but since the PermissionPromptAndroid is destroyed on navigation this will cause a crash. BUG=606138 ========== to ========== Start wiring up modal dialog experiment in PermissionRequestManager codepath This patch starts adding support for the modal dialog experiment when the PermissionRequestManager is enabled on Android. To keep things simple we re-use the existing PermissionDialogDelegate class and allow it to now operate on a PermissionPromptAndroid. There are several pieces that still need to be implemented in subsequent patches. The string used in the dialog is currently the wrong string (we use the fragment, e.g. "Know your location" instead of a full sentence "permission.site wants to use your device's location.". Navigating while a dialog is open (e.g. a JS timer, see bug 699851) is still broken, but since the PermissionPromptAndroid is destroyed on navigation this will cause a crash. Other outstanding issues are hooking up the link text (learn more) for EME and making toggle experiment work. BUG=606138 ==========
Description was changed from ========== Start wiring up modal dialog experiment in PermissionRequestManager codepath This patch starts adding support for the modal dialog experiment when the PermissionRequestManager is enabled on Android. To keep things simple we re-use the existing PermissionDialogDelegate class and allow it to now operate on a PermissionPromptAndroid. There are several pieces that still need to be implemented in subsequent patches. The string used in the dialog is currently the wrong string (we use the fragment, e.g. "Know your location" instead of a full sentence "permission.site wants to use your device's location.". Navigating while a dialog is open (e.g. a JS timer, see bug 699851) is still broken, but since the PermissionPromptAndroid is destroyed on navigation this will cause a crash. Other outstanding issues are hooking up the link text (learn more) for EME and making toggle experiment work. BUG=606138 ========== to ========== Start wiring up modal dialog experiment in PermissionRequestManager codepath This patch starts adding support for the modal dialog experiment when the PermissionRequestManager is enabled on Android. To keep things simple we re-use the existing PermissionDialogDelegate class and allow it to now operate on a PermissionPromptAndroid. There are several pieces that still need to be implemented in subsequent patches. The string used in the dialog is currently the wrong string (we use the fragment, e.g. "Know your location" instead of a full sentence "permission.site wants to use your device's location.". Navigating while a dialog is open (e.g. a JS timer, see bug 699851) is still broken, but since the PermissionPromptAndroid is destroyed on navigation this will cause a crash. Other outstanding issues are hooking up the link text (learn more) for EME, making the toggle experiment work, and supporting grouped camera + microphone permissions. BUG=606138 ==========
timloh@chromium.org changed reviewers: + raymes@chromium.org
The CQ bit was checked by timloh@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.
https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_dialog_delegate.cc (right): https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:45: PermissionPromptAndroid* permission_prompt) { Is it worth DCHECKing that the PermissionRequestManager is enabled in this case? https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:142: primaryButtonText, secondaryButtonText, Does this change the button labels for the old codepath? https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:177: permission_prompt_->Accept(); nit: Should we add TODOs here and below to hook up the persistence toggle? https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:245: } nit: should we just merge these 2 constructors together and just pass null for the unneeded param? https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_dialog_delegate.h (right): https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.h:37: // for using a PermissionInfoBarDelegate. nit: can you add a bug link here? https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.h:39: // TODO(timloh): Make this a WebContentsObserver so we can remove this if the nit: can you clarify what "this" means here https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.h:46: PermissionPromptAndroid* permission_prompt); nit: could you add more documentation on when each of these is used? https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.h:97: PermissionPromptAndroid* permission_prompt_; nit: could you make a comment on the lifetime of this? When is it safe to use this pointer?
Description was changed from ========== Start wiring up modal dialog experiment in PermissionRequestManager codepath This patch starts adding support for the modal dialog experiment when the PermissionRequestManager is enabled on Android. To keep things simple we re-use the existing PermissionDialogDelegate class and allow it to now operate on a PermissionPromptAndroid. There are several pieces that still need to be implemented in subsequent patches. The string used in the dialog is currently the wrong string (we use the fragment, e.g. "Know your location" instead of a full sentence "permission.site wants to use your device's location.". Navigating while a dialog is open (e.g. a JS timer, see bug 699851) is still broken, but since the PermissionPromptAndroid is destroyed on navigation this will cause a crash. Other outstanding issues are hooking up the link text (learn more) for EME, making the toggle experiment work, and supporting grouped camera + microphone permissions. BUG=606138 ========== to ========== Start wiring up modal dialog experiment in PermissionRequestManager codepath This patch starts adding support for the modal dialog experiment when the PermissionRequestManager is enabled on Android. To keep things simple we re-use the existing PermissionDialogDelegate class and allow it to now operate on a PermissionPromptAndroid. There are several pieces that still need to be implemented in subsequent patches. The string used in the dialog is currently the wrong string (we use the fragment, e.g. "Know your location" instead of a full sentence "permission.site wants to use your device's location.". The link text (learn more) for EME needs to be hooked up, as well as making the toggle experiment work, and supporting grouped camera + microphone permissions. BUG=606138 ==========
Rebased on top of https://codereview.chromium.org/2899973002/ so we don't uaf if navigating while a modal is active. https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_dialog_delegate.cc (right): https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:45: PermissionPromptAndroid* permission_prompt) { On 2017/04/11 02:50:35, raymes wrote: > Is it worth DCHECKing that the PermissionRequestManager is enabled in this case? I'd prefer to not add the dependency. https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:142: primaryButtonText, secondaryButtonText, On 2017/04/11 02:50:35, raymes wrote: > Does this change the button labels for the old codepath? I just inlined the logic of GetButtonLabel so the values could be re-used below. https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:177: permission_prompt_->Accept(); On 2017/04/11 02:50:35, raymes wrote: > nit: Should we add TODOs here and below to hook up the persistence toggle? Don't think it's needed. https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:245: } On 2017/04/11 02:50:35, raymes wrote: > nit: should we just merge these 2 constructors together and just pass null for > the unneeded param? Sure, done. https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_dialog_delegate.h (right): https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.h:37: // for using a PermissionInfoBarDelegate. On 2017/04/11 02:50:35, raymes wrote: > nit: can you add a bug link here? Done. https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.h:39: // TODO(timloh): Make this a WebContentsObserver so we can remove this if the On 2017/04/11 02:50:35, raymes wrote: > nit: can you clarify what "this" means here Done. https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.h:46: PermissionPromptAndroid* permission_prompt); On 2017/04/11 02:50:35, raymes wrote: > nit: could you add more documentation on when each of these is used? Done. https://codereview.chromium.org/2808003002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.h:97: PermissionPromptAndroid* permission_prompt_; On 2017/04/11 02:50:35, raymes wrote: > nit: could you make a comment on the lifetime of this? When is it safe to use > this pointer? Done.
lgtm https://codereview.chromium.org/2808003002/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_dialog_delegate.cc (right): https://codereview.chromium.org/2808003002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:56: new PermissionDialogDelegate(tab, nullptr, permission_prompt); nit: /*infobar_delegate=*/nullptr https://codereview.chromium.org/2808003002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:104: new PermissionDialogDelegate(tab, std::move(infobar_delegate), nullptr); nit: /*permission_prompt=*/nullptr
https://codereview.chromium.org/2808003002/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_dialog_delegate.cc (right): https://codereview.chromium.org/2808003002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:56: new PermissionDialogDelegate(tab, nullptr, permission_prompt); On 2017/05/29 04:31:39, raymes wrote: > nit: /*infobar_delegate=*/nullptr Done. https://codereview.chromium.org/2808003002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:104: new PermissionDialogDelegate(tab, std::move(infobar_delegate), nullptr); On 2017/05/29 04:31:39, raymes wrote: > nit: /*permission_prompt=*/nullptr Done.
The CQ bit was checked by timloh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2808003002/#ps80001 (title: "address comments")
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
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 timloh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2808003002/#ps100001 (title: "fix bad rebase")
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": 100001, "attempt_start_ts": 1496115568501190,
"parent_rev": "eb83ceebd189762e06fb281ef9b11cd98b51aa43", "commit_rev":
"ee625a76bb992853cd9a73625f25deeecfeb0d83"}
Message was sent while issue was closed.
Description was changed from ========== Start wiring up modal dialog experiment in PermissionRequestManager codepath This patch starts adding support for the modal dialog experiment when the PermissionRequestManager is enabled on Android. To keep things simple we re-use the existing PermissionDialogDelegate class and allow it to now operate on a PermissionPromptAndroid. There are several pieces that still need to be implemented in subsequent patches. The string used in the dialog is currently the wrong string (we use the fragment, e.g. "Know your location" instead of a full sentence "permission.site wants to use your device's location.". The link text (learn more) for EME needs to be hooked up, as well as making the toggle experiment work, and supporting grouped camera + microphone permissions. BUG=606138 ========== to ========== Start wiring up modal dialog experiment in PermissionRequestManager codepath This patch starts adding support for the modal dialog experiment when the PermissionRequestManager is enabled on Android. To keep things simple we re-use the existing PermissionDialogDelegate class and allow it to now operate on a PermissionPromptAndroid. There are several pieces that still need to be implemented in subsequent patches. The string used in the dialog is currently the wrong string (we use the fragment, e.g. "Know your location" instead of a full sentence "permission.site wants to use your device's location.". The link text (learn more) for EME needs to be hooked up, as well as making the toggle experiment work, and supporting grouped camera + microphone permissions. BUG=606138 Review-Url: https://codereview.chromium.org/2808003002 Cr-Commit-Position: refs/heads/master@{#475422} Committed: https://chromium.googlesource.com/chromium/src/+/ee625a76bb992853cd9a73625f25... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/ee625a76bb992853cd9a73625f25... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
