[Desktop Android] Switch extension installed dialog to use DialogModel [chromium/src : main]
Eva Su (Gerrit)
Eva Su voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Emilia Paz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emilia Paz (Gerrit)
Emilia Paz voted and added 7 comments![]( "Open in Gerrit")
Votes added by Emilia Paz
| Code-Review | +1 |
7 comments
Overall lgtm, thanks Eva! Just some small comments. This is a good first step. Some pointers for the work left:
- remove `BubbleDialogModelHost` and only use `ui::DialogModel`
- have the dialog delegate only do 'delegate stuff' and the show is handled separately (see comment)
- maybe rename to PostInstallDialog to avoid confusion? :)
<message name="IDS_EXTENSION_INSTALLED_MANAGE_SHORTCUTS_LINK_PLACEHOLDER" desc="A placeholder for the manage shortcuts link.">
<ph name="MANAGE_SHORTCUTS_LINK">$1<ex>Manage shortcuts</ex></ph>
</message>We can add the link to the existent one (IDS_EXTENSION_INSTALLED_MANAGE_SHORTCUTS), or if not remove that one since it's no longer used
class ExtensionInstalledBubbleView : public ui::DialogModelDelegate {This is technically not a `View` anymore. I think it's ok to leave the name as is for now because we are continue to refactor it. The goal would be to have a `ExtensionPostInstallDialogDelegate` that handles callbacks. The show method should be outside. Similar to https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/extensions/settings_overridden_dialog.cc;l=21-58
#include "ui/views/controls/label.h"I think you can remove this. Check if other includes are not needed
auto wrapper_view = std::make_unique<views::BoxLayoutView>();
wrapper_view->SetBackground(
views::CreateSolidBackground(ui::kColorSysNeutralContainer));
const auto& layout_provider = *views::LayoutProvider::Get();
const gfx::Insets dialog_insets =
layout_provider.GetInsetsMetric(views::InsetsMetric::INSETS_DIALOG);
wrapper_view->SetInsideBorderInsets(
gfx::Insets::VH(dialog_insets.top(), dialog_insets.left()));Couple of things:
- Lets explain why we add color and insets -- feel free to reword:
- ```
- // Add color and insets to mimic the footnote view on the dialog. We cannot
- // add the footnote using ui::DialogModel because it doesn't support
- // complex footers.
- ```
- Where does the color variable came from? I would have used `ui::kColorBubbleFooterBackground` since [`FootnoteContainerView`](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/bubble/footnote_container_view.cc;l=48) uses it.. but that color may be correct too.
- Are the insets needed on top of using `views::BubbleDialogModelHost::FieldType::kMenuItem`? Edit: yeah, after local build it does need both. Comment will be good to explain it then
- Also maybe rename to `CreateSigninPromoFootnoteView` since we are customizing it to look like a footer?
//
// -------------------------
// | Icon | Title (x) |
// | Info |
// | Extra info |
// -------------------------This is not true anymore, right? The info and extra info are under the icon. Since now we are using the model and not adding custom view, I think we can reword this whole comment. Extra info is different than footer
dialog_model_builder.AddCustomField(
std::make_unique<views::BubbleDialogModelHost::CustomView>(
CreateSigninPromoView(
browser->tab_strip_model()->GetActiveWebContents(),
weak_delegate->model()->extension_id()),
views::BubbleDialogModelHost::FieldType::kMenuItem));Add a comment that we add custom field instead of footnote because footnote doesn't support complex view (here or inside CreateSigninPromoView(), depending on wording)
Related details
- Eva Su
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. |
Eva Su (Gerrit)
Eva Su added 7 comments![]( "Open in Gerrit")
Overall lgtm, thanks Eva! Just some small comments. This is a good first step. Some pointers for the work left:
- remove `BubbleDialogModelHost` and only use `ui::DialogModel`
- have the dialog delegate only do 'delegate stuff' and the show is handled separately (see comment)
- maybe rename to PostInstallDialog to avoid confusion? :)
Acknowledged and will do in the next CL, thanks!
<message name="IDS_EXTENSION_INSTALLED_MANAGE_SHORTCUTS_LINK_PLACEHOLDER" desc="A placeholder for the manage shortcuts link.">
<ph name="MANAGE_SHORTCUTS_LINK">$1<ex>Manage shortcuts</ex></ph>
</message>We can add the link to the existent one (IDS_EXTENSION_INSTALLED_MANAGE_SHORTCUTS), or if not remove that one since it's no longer used
Done
class ExtensionInstalledBubbleView : public ui::DialogModelDelegate {This is technically not a `View` anymore. I think it's ok to leave the name as is for now because we are continue to refactor it. The goal would be to have a `ExtensionPostInstallDialogDelegate` that handles callbacks. The show method should be outside. Similar to https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/extensions/settings_overridden_dialog.cc;l=21-58
Got it, yes, I'll be renaming it to `ExtensionPostInstallDialogDelegate` as well as refactoring it to handle callbacks in the next CL.
I think you can remove this. Check if other includes are not needed
Done
auto wrapper_view = std::make_unique<views::BoxLayoutView>();
wrapper_view->SetBackground(
views::CreateSolidBackground(ui::kColorSysNeutralContainer));
const auto& layout_provider = *views::LayoutProvider::Get();
const gfx::Insets dialog_insets =
layout_provider.GetInsetsMetric(views::InsetsMetric::INSETS_DIALOG);
wrapper_view->SetInsideBorderInsets(
gfx::Insets::VH(dialog_insets.top(), dialog_insets.left()));Couple of things:
- Lets explain why we add color and insets -- feel free to reword:
- ```
- // Add color and insets to mimic the footnote view on the dialog. We cannot
- // add the footnote using ui::DialogModel because it doesn't support
- // complex footers.
- ```
- Where does the color variable came from? I would have used `ui::kColorBubbleFooterBackground` since [`FootnoteContainerView`](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/bubble/footnote_container_view.cc;l=48) uses it.. but that color may be correct too.
- Are the insets needed on top of using `views::BubbleDialogModelHost::FieldType::kMenuItem`? Edit: yeah, after local build it does need both. Comment will be good to explain it then
- Also maybe rename to `CreateSigninPromoFootnoteView` since we are customizing it to look like a footer?
Thanks! Added comments, renamed to `CreateSigninPromoFootnoteView`, and updated to use `ui::kColorBubbleFooterBackground` and tested via demo that the background color is still correct.
// -------------------------
// | Icon | Title (x) |
// | Info |
// | Extra info |
// -------------------------This is not true anymore, right? The info and extra info are under the icon. Since now we are using the model and not adding custom view, I think we can reword this whole comment. Extra info is different than footer
Done
dialog_model_builder.AddCustomField(
std::make_unique<views::BubbleDialogModelHost::CustomView>(
CreateSigninPromoView(
browser->tab_strip_model()->GetActiveWebContents(),
weak_delegate->model()->extension_id()),
views::BubbleDialogModelHost::FieldType::kMenuItem));Add a comment that we add custom field instead of footnote because footnote doesn't support complex view (here or inside CreateSigninPromoView(), depending on wording)
- Emilia Paz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emilia Paz (Gerrit)
Emilia Paz voted and added 3 comments![]( "Open in Gerrit")
Votes added by Emilia Paz
| Code-Review | +1 |
3 comments
<message name="IDS_EXTENSION_INSTALLED_MANAGE_SHORTCUTS" desc="Text for the link in the InfoBubble that opens the chrome://extensions page with the Configure Commands UI visible.">super nit: Lets update this comment, it's not an info bubble anymore. And the link content is the current one?
#include "ui/views/bubble/bubble_dialog_delegate_view.h"nit: remove
Related details
- Eva Su
- Tim
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. |
Eva Su (Gerrit)
Eva Su added 2 comments![]( "Open in Gerrit")
<message name="IDS_EXTENSION_INSTALLED_MANAGE_SHORTCUTS" desc="Text for the link in the InfoBubble that opens the chrome://extensions page with the Configure Commands UI visible.">super nit: Lets update this comment, it's not an info bubble anymore. And the link content is the current one?
Done
#include "ui/views/bubble/bubble_dialog_delegate_view.h"Eva Sunit: remove
Done
Related details
- Emilia Paz
- Tim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tim (Gerrit)
Tim voted and added 2 comments![]( "Open in Gerrit")
Votes added by Tim
| Code-Review | +1 |
2 comments
Nice, LGTM! Just a stray include that can be removed.
#include "ui/views/bubble/bubble_dialog_delegate_view.h"- Eva Su
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. |
Eva Su (Gerrit)
Eva Su added 1 comment![]( "Open in Gerrit")
#include "ui/views/bubble/bubble_dialog_delegate_view.h"nit: Looks like we can remove this header now?
Thanks, this was removed in the last commit :)
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |