[Desktop Android] Add platform-agnostic logic in c/b/ui/extensions/. [chromium/src : main]

0 views

Skip to first unread message

Eva Su (Gerrit)

unread,

7:17 PM (4 hours ago) 7:17 PM

to Emilia Paz, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Emilia Paz

Eva Su added 1 comment

File chrome/browser/ui/extensions/extension_post_install_dialog.cc

Line 80, Patchset 5: // TODO(crbug.com/450296898): Add a sync or sign in promo in the footer if it

Eva Su . resolved

This will be added in the next CL since there's many dependencies needed for this, and this method is not actually called yet, so we can leave this out for now.

Open in Gerrit

Related details

Attention is currently required from:

Emilia Paz

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

Emilia Paz (Gerrit)

unread,

9:23 PM (2 hours ago) 9:23 PM

to Eva Su, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Eva Su

Emilia Paz added 5 comments

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Emilia Paz . resolved

Thanks Eva! Mainly a comment on the utils file

File chrome/browser/ui/extensions/extension_post_install_dialog.h

Line 19, Patchset 9 (Latest):

// Shows the extension post-install dialog. This function is platform-agnostic.
void ShowExtensionPostInstallDialog(
    Profile* profile,
    content::WebContents* web_contents,
    std::unique_ptr<ExtensionPostInstallDialogModel> model);

Emilia Paz . unresolved

nit-optional: this could be on `chrome/browser/ui/extensions/extensions_dialogs.h`
Although, that's when dialogs where in different places. Having their own header also make sense. No strong preference

Line 9, Patchset 9 (Latest):

Emilia Paz . unresolved

nit: `static_assert(BUILDFLAG(ENABLE_EXTENSIONS_CORE));`

File chrome/browser/ui/extensions/extension_post_install_dialog_utils.cc

Line 35, Patchset 9 (Latest):

void ConfigurePostInstallDialogModel(
    ui::DialogModel::Builder& dialog_model_builder,
    ExtensionPostInstallDialogModel* model,
    base::RepeatingClosure manage_shortcuts_callback) {
  std::u16string extension_name =
      extensions::util::GetFixupExtensionNameForUIDisplay(
          model->extension_name());
  base::i18n::AdjustStringForLocaleDirection(&extension_name);
  dialog_model_builder
      .SetTitle(l10n_util::GetStringFUTF16(IDS_EXTENSION_INSTALLED_HEADING,
                                           extension_name))
      .SetIcon(
          ui::ImageModel::FromImageSkia(model->MakeIconOfSize(kMaxIconSize)));

  if (model->show_how_to_use()) {
    dialog_model_builder.AddParagraph(
        ui::DialogModelLabel(model->GetHowToUseText()));
  }
  if (model->show_key_binding() && manage_shortcuts_callback) {
    dialog_model_builder.AddParagraph(
        ui::DialogModelLabel::CreateWithReplacement(
            IDS_EXTENSION_INSTALLED_MANAGE_SHORTCUTS,
            ui::DialogModelLabel::CreateLink(
                IDS_EXTENSION_INSTALLED_MANAGE_SHORTCUTS_LINK_TEXT,
                manage_shortcuts_callback)));
  }
  if (model->show_how_to_manage()) {
    dialog_model_builder.AddParagraph(ui::DialogModelLabel(
        l10n_util::GetStringUTF16(IDS_EXTENSION_INSTALLED_MANAGE_INFO)));
  }
}

void OpenExtensionsShortcutsPage(
    base::WeakPtr<content::WebContents> web_contents) {
  if (!web_contents) {
    return;
  }
  const GURL kUrl(base::StrCat({chrome::kChromeUIExtensionsURL,
                                chrome::kExtensionConfigureCommandsSubPage}));
  content::OpenURLParams params(
      kUrl, content::Referrer(), WindowOpenDisposition::NEW_FOREGROUND_TAB,
      ui::PAGE_TRANSITION_LINK, /*is_renderer_initiated=*/false);
  web_contents->OpenURL(params, {});
}

Emilia Paz . unresolved

Why do we have these methods on a utils file? Could we have them on the dialog? This is directly building and handling dialog input

Line 80, Patchset 9 (Latest):

void TriggerPostInstallDialog(
    Profile* profile,
    scoped_refptr<const extensions::Extension> extension,
    const SkBitmap& icon,
    base::OnceCallback<content::WebContents*()> get_web_contents_callback) {
  auto watcher = std::make_unique<ExtensionInstalledWatcher>(profile);
  ExtensionInstalledWatcher* watcher_ptr = watcher.get();
  watcher_ptr->WaitForInstall(
      extension->id(),
      base::BindOnce(
          [](std::unique_ptr<ExtensionInstalledWatcher> watcher,
             scoped_refptr<const extensions::Extension> ext, Profile* prof,
             const SkBitmap& icon_val,
             base::OnceCallback<content::WebContents*()> get_web_contents_cb,
             bool installed) {
            if (!installed) {
              return;
            }
            content::WebContents* web_contents =
                std::move(get_web_contents_cb).Run();
            if (!web_contents) {
              return;
            }
            auto model = std::make_unique<ExtensionPostInstallDialogModel>(
                prof, ext.get(), icon_val);
            extensions::ShowExtensionPostInstallDialog(prof, web_contents,
                                                       std::move(model));
          },
          std::move(watcher), extension, profile, icon,
          std::move(get_web_contents_callback)));
}

Emilia Paz . unresolved

And this one.. we could use an existent class to reduce the amount of 'extensions post install' classes. Maybe on `ExtensionInstallUI`? Although this would be easier to see once it's connected with the caller. Could leave here for now

Open in Gerrit

Related details

Attention is currently required from:

Eva Su

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages