Require InstallPromptData in ExtensionInstallPrompt constructors [chromium/src : main]

0 views
Skip to first unread message

Miyoung Shin (Gerrit)

unread,
Jul 8, 2026, 4:03:01 AM (14 days ago) Jul 8
to Devlin Cronin, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromium-a...@chromium.org, dtraino...@chromium.org, extension...@chromium.org
Attention needed from Devlin Cronin

Miyoung Shin voted Commit-Queue+1

Commit-Queue+1
Open in Gerrit

Related details

Attention is currently required from:
  • Devlin Cronin
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ic6a4e9449a0bb61ee3cf2b123a79321c46f64e74
Gerrit-Change-Number: 8061505
Gerrit-PatchSet: 2
Gerrit-Owner: Miyoung Shin <myid...@igalia.com>
Gerrit-Reviewer: Devlin Cronin <rdevlin...@chromium.org>
Gerrit-Reviewer: Miyoung Shin <myid...@igalia.com>
Gerrit-Attention: Devlin Cronin <rdevlin...@chromium.org>
Gerrit-Comment-Date: Wed, 08 Jul 2026 08:02:42 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Devlin Cronin (Gerrit)

unread,
Jul 8, 2026, 1:59:08 PM (13 days ago) Jul 8
to Miyoung Shin, Devlin Cronin, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromium-a...@chromium.org, dtraino...@chromium.org, extension...@chromium.org
Attention needed from Miyoung Shin

Devlin Cronin added 2 comments

Patchset-level comments
File-level comment, Patchset 2 (Latest):
Devlin Cronin . resolved

Thanks, Miyoung!

File chrome/browser/extensions/extension_install_prompt.cc
Line 177, Patchset 2 (Latest): if (prompt_->type() == InstallPromptData::UNSET_PROMPT_TYPE) {
Devlin Cronin . unresolved

this and ConfirmReEnable are *only* called from CrxInstaller, which is always passed an unset type. Would it make sense to CHECK_EQ(UNSET_PROMPT_TYPE, prompt_->type()) here to verify that, and add a comment explaining it? (Ditto below)

Separately, I think it'd be great to refactor CrxInstaller to construct the ExtensionInstallPrompt closer to "on-demand", which avoids this -- but that's definitely not something to tackle in this CL.

Open in Gerrit

Related details

Attention is currently required from:
  • Miyoung Shin
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic6a4e9449a0bb61ee3cf2b123a79321c46f64e74
    Gerrit-Change-Number: 8061505
    Gerrit-PatchSet: 2
    Gerrit-Owner: Miyoung Shin <myid...@igalia.com>
    Gerrit-Reviewer: Devlin Cronin <rdevlin...@chromium.org>
    Gerrit-Reviewer: Miyoung Shin <myid...@igalia.com>
    Gerrit-Attention: Miyoung Shin <myid...@igalia.com>
    Gerrit-Comment-Date: Wed, 08 Jul 2026 17:58:54 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Miyoung Shin (Gerrit)

    unread,
    Jul 15, 2026, 8:21:03 AM (6 days ago) Jul 15
    to Devlin Cronin, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromium-a...@chromium.org, dtraino...@chromium.org, extension...@chromium.org
    Attention needed from Devlin Cronin

    Miyoung Shin added 1 comment

    File chrome/browser/extensions/extension_install_prompt.cc
    Line 177, Patchset 2: if (prompt_->type() == InstallPromptData::UNSET_PROMPT_TYPE) {
    Devlin Cronin . resolved

    this and ConfirmReEnable are *only* called from CrxInstaller, which is always passed an unset type. Would it make sense to CHECK_EQ(UNSET_PROMPT_TYPE, prompt_->type()) here to verify that, and add a comment explaining it? (Ditto below)

    Separately, I think it'd be great to refactor CrxInstaller to construct the ExtensionInstallPrompt closer to "on-demand", which avoids this -- but that's definitely not something to tackle in this CL.

    Miyoung Shin

    Done

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Devlin Cronin
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Owners
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Ic6a4e9449a0bb61ee3cf2b123a79321c46f64e74
      Gerrit-Change-Number: 8061505
      Gerrit-PatchSet: 4
      Gerrit-Owner: Miyoung Shin <myid...@igalia.com>
      Gerrit-Reviewer: Devlin Cronin <rdevlin...@chromium.org>
      Gerrit-Reviewer: Miyoung Shin <myid...@igalia.com>
      Gerrit-Attention: Devlin Cronin <rdevlin...@chromium.org>
      Gerrit-Comment-Date: Wed, 15 Jul 2026 12:20:31 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Devlin Cronin <rdevlin...@chromium.org>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Devlin Cronin (Gerrit)

      unread,
      Jul 15, 2026, 2:35:51 PM (6 days ago) Jul 15
      to Miyoung Shin, Devlin Cronin, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromium-a...@chromium.org, dtraino...@chromium.org, extension...@chromium.org
      Attention needed from Miyoung Shin

      Devlin Cronin voted and added 2 comments

      Votes added by Devlin Cronin

      Code-Review+1

      2 comments

      Patchset-level comments
      File-level comment, Patchset 4 (Latest):
      Devlin Cronin . resolved

      LGTM; thanks, Miyoung!

      File chrome/browser/extensions/extension_install_prompt.cc
      Line 127, Patchset 4 (Latest): const ShowDialogCallback& show_dialog_callback) {
      Devlin Cronin . unresolved

      by this point, the prompt should always have a defined type, right? Can we CHECK that?

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Miyoung Shin
      Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Owners
        • requirement satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        • requirement satisfiedReview-Enforcement
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: Ic6a4e9449a0bb61ee3cf2b123a79321c46f64e74
        Gerrit-Change-Number: 8061505
        Gerrit-PatchSet: 4
        Gerrit-Owner: Miyoung Shin <myid...@igalia.com>
        Gerrit-Reviewer: Devlin Cronin <rdevlin...@chromium.org>
        Gerrit-Reviewer: Miyoung Shin <myid...@igalia.com>
        Gerrit-Attention: Miyoung Shin <myid...@igalia.com>
        Gerrit-Comment-Date: Wed, 15 Jul 2026 18:35:38 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: Yes
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Miyoung Shin (Gerrit)

        unread,
        Jul 16, 2026, 9:57:34 PM (5 days ago) Jul 16
        to Avi Drissman, Devlin Cronin, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromium-a...@chromium.org, dtraino...@chromium.org, extension...@chromium.org
        Attention needed from Avi Drissman

        Miyoung Shin added 2 comments

        Patchset-level comments
        File-level comment, Patchset 5 (Latest):
        Miyoung Shin . resolved

        @avi for //chrome/browser/download & //chrome/browser/infobars

        File chrome/browser/extensions/extension_install_prompt.cc
        Line 127, Patchset 4: const ShowDialogCallback& show_dialog_callback) {
        Devlin Cronin . resolved

        by this point, the prompt should always have a defined type, right? Can we CHECK that?

        Miyoung Shin

        Done

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Avi Drissman
        Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement is not satisfiedCode-Owners
          • requirement satisfiedCode-Review
          • requirement satisfiedReview-Enforcement
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: Ic6a4e9449a0bb61ee3cf2b123a79321c46f64e74
          Gerrit-Change-Number: 8061505
          Gerrit-PatchSet: 5
          Gerrit-Owner: Miyoung Shin <myid...@igalia.com>
          Gerrit-Reviewer: Avi Drissman <a...@chromium.org>
          Gerrit-Attention: Avi Drissman <a...@chromium.org>
          Gerrit-Comment-Date: Fri, 17 Jul 2026 01:56:59 +0000
          satisfied_requirement
          unsatisfied_requirement
          open
          diffy

          Avi Drissman (Gerrit)

          unread,
          Jul 17, 2026, 9:25:32 AM (4 days ago) Jul 17
          to Miyoung Shin, Avi Drissman, Devlin Cronin, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromium-a...@chromium.org, dtraino...@chromium.org, extension...@chromium.org
          Attention needed from Miyoung Shin

          Avi Drissman voted Code-Review+1

          Code-Review+1
          Open in Gerrit

          Related details

          Attention is currently required from:
          • Miyoung Shin
          Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Owners
          • requirement satisfiedCode-Review
          • requirement satisfiedReview-Enforcement
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: Ic6a4e9449a0bb61ee3cf2b123a79321c46f64e74
          Gerrit-Change-Number: 8061505
          Gerrit-PatchSet: 5
          Gerrit-Owner: Miyoung Shin <myid...@igalia.com>
          Gerrit-Reviewer: Avi Drissman <a...@chromium.org>
          Gerrit-Reviewer: Devlin Cronin <rdevlin...@chromium.org>
          Gerrit-Reviewer: Miyoung Shin <myid...@igalia.com>
          Gerrit-Attention: Miyoung Shin <myid...@igalia.com>
          Gerrit-Comment-Date: Fri, 17 Jul 2026 13:25:23 +0000
          Gerrit-HasComments: No
          Gerrit-Has-Labels: Yes
          satisfied_requirement
          open
          diffy

          Miyoung Shin (Gerrit)

          unread,
          1:55 AM (16 hours ago) 1:55 AM
          to Avi Drissman, Devlin Cronin, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromium-a...@chromium.org, dtraino...@chromium.org, extension...@chromium.org

          Miyoung Shin voted Commit-Queue+2

          Commit-Queue+2
          Open in Gerrit

          Related details

          Attention set is empty
          Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Owners
          • requirement satisfiedCode-Review
          • requirement satisfiedReview-Enforcement
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: Ic6a4e9449a0bb61ee3cf2b123a79321c46f64e74
          Gerrit-Change-Number: 8061505
          Gerrit-PatchSet: 5
          Gerrit-Owner: Miyoung Shin <myid...@igalia.com>
          Gerrit-Reviewer: Avi Drissman <a...@chromium.org>
          Gerrit-Reviewer: Devlin Cronin <rdevlin...@chromium.org>
          Gerrit-Reviewer: Miyoung Shin <myid...@igalia.com>
          Gerrit-Comment-Date: Tue, 21 Jul 2026 05:55:21 +0000
          Gerrit-HasComments: No
          Gerrit-Has-Labels: Yes
          satisfied_requirement
          open
          diffy

          Chromium LUCI CQ (Gerrit)

          unread,
          2:37 AM (15 hours ago) 2:37 AM
          to Miyoung Shin, Avi Drissman, Devlin Cronin, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromium-a...@chromium.org, dtraino...@chromium.org, extension...@chromium.org

          Chromium LUCI CQ submitted the change

          Change information

          Commit message:
          Require InstallPromptData in ExtensionInstallPrompt constructors

          Previously, ExtensionInstallPrompt accepted a nullable InstallPromptData
          in its constructors and lazily created one in ConfirmInstall() and
          ConfirmReEnable() when needed.

          This could lead to confusing behavior where an ExtensionInstallPrompt was
          constructed with an InstallPromptData instance, but later calls to other
          ShowDialog() methods ignored the provided data.

          This change makes InstallPromptData mandatory for all
          ExtensionInstallPrompt constructors. For cases where the prompt type is
          determined later (e.g. ConfirmInstall() and ConfirmReEnable()), it uses
          UNSET_PROMPT_TYPE when creating a new InstallPromptData instance.
          Bug: 358567092
          Change-Id: Ic6a4e9449a0bb61ee3cf2b123a79321c46f64e74
          Commit-Queue: Miyoung Shin <myid...@igalia.com>
          Reviewed-by: Avi Drissman <a...@chromium.org>
          Reviewed-by: Devlin Cronin <rdevlin...@chromium.org>
          Cr-Commit-Position: refs/heads/main@{#1665208}
          Files:
          • M chrome/browser/download/download_crx_util.cc
          • M chrome/browser/extensions/api/developer_private/developer_private_functions.cc
          • M chrome/browser/extensions/api/management/chrome_management_api_delegate.cc
          • M chrome/browser/extensions/api/permissions/permissions_api.cc
          • M chrome/browser/extensions/crx_installer_browsertest.cc
          • M chrome/browser/extensions/extension_browsertest.cc
          • M chrome/browser/extensions/extension_install_prompt.cc
          • M chrome/browser/extensions/extension_install_prompt.h
          • M chrome/browser/extensions/extension_install_prompt_browsertest.cc
          • M chrome/browser/extensions/extension_install_prompt_unittest.cc
          • M chrome/browser/extensions/external_install_error_desktop.cc
          • M chrome/browser/extensions/navigation_extension_enabler.cc
          • M chrome/browser/extensions/webstore_install_with_prompt.cc
          • M chrome/browser/extensions/webstore_install_with_prompt.h
          • M chrome/browser/extensions/webstore_standalone_installer.cc
          • M chrome/browser/extensions/webstore_standalone_installer.h
          • M chrome/browser/infobars/infobars_browsertest.cc
          • M chrome/browser/ui/extensions/extension_enable_flow.cc
          • M chrome/browser/ui/extensions/extension_enable_flow.h
          • M chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
          • M chrome/browser/ui/views/extensions/extension_install_dialog_view_supervised_browsertest.cc
          • M extensions/browser/install_prompt_data.cc
          • M extensions/browser/install_prompt_data.h
          Change size: M
          Delta: 23 files changed, 137 insertions(+), 99 deletions(-)
          Branch: refs/heads/main
          Submit Requirements:
          • requirement satisfiedCode-Review: +1 by Avi Drissman, +1 by Devlin Cronin
          Open in Gerrit
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: merged
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: Ic6a4e9449a0bb61ee3cf2b123a79321c46f64e74
          Gerrit-Change-Number: 8061505
          Gerrit-PatchSet: 6
          Gerrit-Owner: Miyoung Shin <myid...@igalia.com>
          Gerrit-Reviewer: Avi Drissman <a...@chromium.org>
          Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
          Gerrit-Reviewer: Devlin Cronin <rdevlin...@chromium.org>
          Gerrit-Reviewer: Miyoung Shin <myid...@igalia.com>
          open
          diffy
          satisfied_requirement
          Reply all
          Reply to author
          Forward
          0 new messages