Rename ExtensionInstallPromptClient::Prompt to InstallPromptData [chromium/src : main]

0 views

Skip to first unread message

Miyoung Shin (Gerrit)

unread,

Jun 16, 2026, 1:59:43 PM (9 days ago) Jun 16

to kokoro, android-bu...@system.gserviceaccount.com, Devlin Cronin, Chromium LUCI CQ, chromium...@chromium.org, srahim...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Devlin Cronin

Miyoung Shin added 1 comment

File extensions/browser/extension_install_prompt_client.h

Line 121, Patchset 7:

                                 Observer* observer,
                                 bool requires_parent_permission) = 0;

Devlin Cronin . resolved

I'd like to avoid having requires_parent_permission in the generic "ShowInstallDialog" function, since it's very targeted. (Probably ditto for Observer)
Two options:
1) Expose a SetRequiresParentPermission() method here that passes through to the Prompt in ExtensionInstallPrompt. This is a bit tricky, since the Prompt isn't constructed at this point. To fix that, we could a) take in a Prompt::Type in CreateInstallPrompt in ExtensionsBrowserClient and b) create the Prompt as part of that construction, so that it's ready when we then use other methods on it.
2) From a very quick skim, it _looks_ like we might be able to hoist all of ExtensionInstallPrompt::Prompt to //extensions (with a better name, maybe InstallPromptData or InstallPromptParams)? That might be desirable, and maybe then we could pass the constructed prompt as a parameter to CreateInstallPrompt(), and not worry about the pass-through methods. That also has some nice architectural benefits, since then the //extensions layer is responsible for handling the "metadata", and just delegates out the display / view to the embedder.

Option 1) is easier, 2) might be better long-term. WDYT?

Miyoung Shin

Thanks for the suggestions.

I updated the patch following option 2. I moved the prompt metadata out into a standalone `InstallPromptData` class.
Could you take another look at the updated patch and let me know what you think?

Open in Gerrit

Related details

Attention is currently required from:

Devlin Cronin

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Devlin Cronin (Gerrit)

unread,

4:15 PM (5 hours ago) 4:15 PM

to Miyoung Shin, kokoro, android-bu...@system.gserviceaccount.com, Devlin Cronin, Chromium LUCI CQ, chromium...@chromium.org, srahim...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Miyoung Shin

Devlin Cronin added 3 comments

Patchset-level comments

File-level comment, Patchset 19 (Latest):

Devlin Cronin . resolved

Thank you, Miyoung, and sorry for the delay here! Two high-level comments that will make this easier to review.

File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc

Line 625, Patchset 19 (Latest): install_prompt_ = ExtensionsBrowserClient::Get()->CreateInstallPrompt(

Devlin Cronin . unresolved

Can I ask you to split this into two CLs?
1) Move + rename InstallPromptData. Becomes extremely mechanical.
2) Updates to webstorePrivate, introducing new methods on ExtensionBrowserClient, etc. Not quite as mechanical, but much smaller.

File extensions/browser/install_prompt_data.h

Line 58, Patchset 19 (Latest):

  // Interface for observing events on the prompt.
  class Observer : public base::CheckedObserver {
   public:
    // Called right before the dialog is about to show.
    virtual void OnDialogOpened() = 0;

    // Called when the user clicks accept on the dialog.
    virtual void OnDialogAccepted() = 0;

    // Called when the user clicks cancel on the dialog, presses 'x' or escape.
    virtual void OnDialogCanceled() = 0;
  };

Devlin Cronin . unresolved

Can we leave this on ExtensionInstallPromptClient? Ideally, InstallPromptData is a strictly data-storing class, rather than having functionality and behavior. We're not there yet (because we still have e.g. OnDialogOpened, etc), but I think I'd like to move in that direction.