[ALPC Extensions]: Add new dialog for final extension installation step [chromium/src : main]
Richard Chui (Gerrit)
Richard Chui added 1 comment![]( "Open in Gerrit")
EXTENSION_PARENT_APPROVAL_PROMPT = 15,@emiliapaz: I am not sure if this is the correct approach. Basically, the dialog should function exactly like a normal install prompt, but just with custom text. I considered just using `INSTALL_PROMPT` and overriding it for all the new strings, but not sure if that will interfere with some of the existing functionality (i.e. like in `GetAcceptButtonLabel()` where it may expect a certain UI already.
Related details
- Emilia Paz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emilia Paz (Gerrit)
Emilia Paz added 9 comments![]( "Open in Gerrit")
Thanks Richard! Direction is good. Left some comments. We are also going to need some test coverage, specially:
(1) when OnParentApprovalDone receives SupervisedExtensionApprovalResult::kApproved, it correctly initializes an
ExtensionInstallPrompt with the new EXTENSION_PARENT_APPROVAL_PROMPT type and displays it.
(2) when OnParentApprovalDone receives kCanceled or kFailed, it immediately runs the done_callback_ with the result and does not proceed to show the second dialog.
EXTENSION_PARENT_APPROVAL_PROMPT = 15,@emiliapaz: I am not sure if this is the correct approach. Basically, the dialog should function exactly like a normal install prompt, but just with custom text. I considered just using `INSTALL_PROMPT` and overriding it for all the new strings, but not sure if that will interfere with some of the existing functionality (i.e. like in `GetAcceptButtonLabel()` where it may expect a certain UI already.
I think a new type is okay to avoid overriding information
// Called when the parent access flow on Android desktop completes to show the
// browser parent approval permission dialog.
void OnParentApprovalDone(const Extension& extension,
base::WeakPtr<content::WebContents> contents,
const gfx::ImageSkia& icon,
SupervisedExtensionApprovalResult result);If this is only for android lets wrap it around a BUILDFLAG
std::optional<base::WeakPtr<content::WebContents>> contents,note - not for this cl: `contents` can only be nullopt for ChromeOS, as other platforms check for `contents.value()`. Is this intended? can a dialog be shown without web contents?
base::Unretained(this), std::cref(extension), contents.value(), icon);This may be a potential use-after-free. This callback is passed to
ExtensionParentApproval, which triggers an asynchronous Android UI flow. If this SupervisedUserExtensionsDelegateImpl instance is destroyed before the parent completes the approval, executing this callback will crash. I believe we want top use a `WeakPtr` here
base::Unretained(this), std::cref(extension), contents.value(), icon);The extension could get uninstalled while the dialog is opened, and the reference here would no longer be valid. We should use a `scoped_refptr` or pass the extension id, and retrieve the extension in the callback
&SupervisedUserExtensionsDelegateImpl::OnParentApprovalDone,
base::Unretained(this), std::cref(extension), contents.value(), icon);What if we pass `done_callback_` to `OnParentApprovalDone`? This could prevent issues where `done_callback_` is called twice, and also follows the same pattern of passing `done_callback` to the dialog callback on other platforms (previous lines)
if (result != SupervisedExtensionApprovalResult::kApproved) {
std::move(done_callback_).Run(result);
return;
}nit - optional: lets explain that the next dialog is only shown if the parental approval dialog was accepted
And with that.. are both dialogs names "parent approval"? It's a bit confusing. It would be better if they had clear different names
case ExtensionInstallPrompt::Result::
ACCEPTED_WITH_WITHHELD_PERMISSIONS:Parent approval dialog doesn't support "withhold permissions". Lets add a check instead, similar to https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/extensions/api/permissions/permissions_api.cc;l=490-492;drc=72eb090be2c9a401d7f03a5b5f91c9e216e8bba3
Related details
- Richard Chui
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emilia Paz (Gerrit)
Emilia Paz added 1 comment![]( "Open in Gerrit")
nit: if possible, lets add a screenshot of the dialog. Helps whem looking back on the CL from codesearch :)
Related details
- Richard Chui
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |