Overrides of DialogDelegate::UpdateButton() should always invoke super::UpdateButton() (or the overrides shouldn't exist at all) |
|||||
Issue descriptionChrome Version : 58.0.3018.3 Currently DialogClientView duplicates button configuration work done in DialogDelegate::UpdateButton() (e.g. for setting the button title). DialogClientView should just allocate/create the button by type rather than setting any properties on it. There aren't many overrides of DialogDelegate::UpdateButton - maybe it can just be made non-virtual. If a client calls DialogClientView::UpdateDialogButtons() then it can just update DialogClientView::ok_button(), DialogClientView::cancel_button() directly afterwards; before a call to Layout(). One override of UpdateButton() doesn't always invoke super::UpdateButton - GlobalErrorBubbleView in https://cs.chromium.org/chromium/src/chrome/browser/ui/views/global_error_bubble_view.cc?l=138 .. which is a recent thing (since r449727 - 3 weeks ago), This refactor will be simpler when DialogClientView doesn't need to pick a button *type* but merely allocate it. That is, once MdTextButton is the only button type used in dialogs -- currently it may also be a views::LabelButton or a views::BlueButton. This would also allow the call to MdTextButton::SetProminent() to move to DialogDelegate::UpdateButton().
,
Mar 1 2017
+CC kylixrd, who has a special love of "virtual override must call superclass explicitly or Bad Things Happen" designs.
,
Mar 1 2017
Is the intention here to refactor things to avoid the override? Or would you prefer for us to fix the GlobalErrorBubbleView regression? (Sounds like we should do that anyway, if only as a short-term fix?)
,
Mar 1 2017
I'm not sure whether GlobalErrorBubbleView has regressed (not in function anyway). Looking at the code, I *think* it lucks out on having any code path where something bad would happen. GlobalErrorBubbleView::GetDialogButtons() only returns something other than NONE when there is |error_|. But then I can't explain why the change to the ::UpdateButton() override in r449727 was necessary (UpdateButton() shouldn't be called at all when the buttons are NONE). So I think it's fine to wait for the refactor that slims down DialogClientView::UpdateDialogButtons(). Depending on the approach, that may need care to ensure GlobalErrorBubbleView is updated at the same time, but I suspect we can find a way to "enforce" this better. E.g. Something like OnButtonUpdated(..) and a non-virtual UpdateButton().
,
Mar 6 2018
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 6 2018
This is probably best addressed once MdTextButton is the only button type used in Dialogs.
,
Sep 13
Archiving old bugs that haven't been actively assigned in over 180 days. If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks! |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by tapted@chromium.org
, Mar 1 2017