Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1846033002:
Switch GlobalErrorBubbleView to a BubbleDialogDelegate. (Closed)
|
Created:
4 years, 8 months ago by Evan Stade Modified:
4 years, 8 months ago CC:
chromium-reviews, tfarina, Finnur Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch GlobalErrorBubbleView to a BubbleDialogDelegate.
Simplify code, fix margins.
BUG=585312
Committed: https://crrev.com/e372a77d7d302b4e1dbf55900094925e677afddb
Cr-Commit-Position: refs/heads/master@{#385241}
Patch Set 1 #
Total comments: 1
Patch Set 2 : fix button action #
Total comments: 4
Patch Set 3 : with dbg code #Patch Set 4 : preserve elevation stuff #Patch Set 5 : remove one last dbg line #Patch Set 6 : . #
Total comments: 7
Patch Set 7 : remove reset #Patch Set 8 : add comment #Patch Set 9 : rebase #
Messages
Total messages: 36 (14 generated)
estade@chromium.org changed reviewers: + msw@chromium.org
https://screenshot.googleplex.com/cGkLJ4jONNa.png https://codereview.chromium.org/1846033002/diff/1/chrome/browser/ui/views/glo... File chrome/browser/ui/views/global_error_bubble_view.cc (right): https://codereview.chromium.org/1846033002/diff/1/chrome/browser/ui/views/glo... chrome/browser/ui/views/global_error_bubble_view.cc:36: const int kMaxBubbleViewWidth = 362; in the screenshot, this makes less difference than you would expect because the width of the bubble was already being expanded by the size of the button strip.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846033002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846033002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
https://codereview.chromium.org/1846033002/diff/20001/chrome/browser/ui/globa... File chrome/browser/ui/global_error/global_error.h (left): https://codereview.chromium.org/1846033002/diff/20001/chrome/browser/ui/globa... chrome/browser/ui/global_error/global_error.h:84: virtual bool ShouldAddElevationIconToAcceptButton(); Isn't this used by chrome/browser/safe_browsing/srt_global_error_win.cc and chrome/browser/recovery/recovery_install_global_error.cc ? https://codereview.chromium.org/1846033002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/global_error_bubble_view.cc (right): https://codereview.chromium.org/1846033002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/global_error_bubble_view.cc:155: return true; Why doesn't this call Cancel / Accept, like DialogDelegate::Close? (should it be NOTREACHED() to mimic the old ButtonPressed behavior?)
responding to comments but not yet updated https://codereview.chromium.org/1846033002/diff/20001/chrome/browser/ui/globa... File chrome/browser/ui/global_error/global_error.h (left): https://codereview.chromium.org/1846033002/diff/20001/chrome/browser/ui/globa... chrome/browser/ui/global_error/global_error.h:84: virtual bool ShouldAddElevationIconToAcceptButton(); On 2016/03/31 22:37:18, msw wrote: > Isn't this used by chrome/browser/safe_browsing/srt_global_error_win.cc and > chrome/browser/recovery/recovery_install_global_error.cc ? ah crud. I guess it slipped my mind that code search doesn't cover windows only stuff. But it looks like there's an accessor for the DCV's OK button so it shouldn't be too hard to add this back. https://codereview.chromium.org/1846033002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/global_error_bubble_view.cc (right): https://codereview.chromium.org/1846033002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/global_error_bubble_view.cc:155: return true; On 2016/03/31 22:37:18, msw wrote: > Why doesn't this call Cancel / Accept, like DialogDelegate::Close? > (should it be NOTREACHED() to mimic the old ButtonPressed behavior?) hmm, I don't think so. ButtonPressed had a NOTREACHED() because there were no other buttons it was listening to. Close() can be called in response to any number of events (such as browser shutdown or the close button being pressed), so it can't NOTREACHED(). Before, if the bubble were closed for some reason without one of the accept/cancel buttons being pressed (e.g. the close button is pressed), it would just skip ahead to error_->BubbleViewDidClose(), and that's being preserved.
estade@chromium.org changed reviewers: + sky@chromium.org
Alright, I had to rearrange some stuff in DCV/DialogDelegate to allow greater flexibility in creating the buttons. With some debug hacks I've verified this works. +sky for ui/views/window OWNERS.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846033002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846033002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
https://codereview.chromium.org/1846033002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/global_error_bubble_view.cc (right): https://codereview.chromium.org/1846033002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/global_error_bubble_view.cc:66: // |elevation_icon_setter_| references |accept_button_|, so make sure it is nit: there is no |accept_button_| member, update comment. https://codereview.chromium.org/1846033002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/global_error_bubble_view.cc:169: return true; I think this is slightly non-obvious and deserves a comment, like: "Do not Accept or Cancel when closed, as most dialogs do, because..."? I think global error bubbles were meant not to have an (x) close button, because we wanted to encourage users to take some explicit action on them, hence the NOTREACHED() in the old GlobalErrorBubbleView::ButtonPressed that I previously mentioned, but my memory here is fuzzy. +CC Finnur, in case he remembers more.
https://codereview.chromium.org/1846033002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/global_error_bubble_view.cc (right): https://codereview.chromium.org/1846033002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/global_error_bubble_view.cc:66: // |elevation_icon_setter_| references |accept_button_|, so make sure it is On 2016/04/04 17:13:18, msw wrote: > nit: there is no |accept_button_| member, update comment. verified this is no longer needed, removed. https://codereview.chromium.org/1846033002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/global_error_bubble_view.cc:169: return true; On 2016/04/04 17:13:18, msw wrote: > I think this is slightly non-obvious and deserves a comment, like: > "Do not Accept or Cancel when closed, as most dialogs do, because..."? I don't know how to complete this sentence, I'm just maintaining the status quo, which was hitherto undocumented. > I think global error bubbles were meant not to have an (x) close button, because > we wanted to encourage users to take some explicit action on them, hence the > NOTREACHED() in the old GlobalErrorBubbleView::ButtonPressed Even if you had a close button and you pressed it, it would not have gone through ButtonPressed because |this| was not registered as a listener on that button. The implementation of GlobalErrorBubbleView::ShouldShowCloseButton() above makes it seem like some of them are intended to have close buttons.
https://codereview.chromium.org/1846033002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/global_error_bubble_view.cc (right): https://codereview.chromium.org/1846033002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/global_error_bubble_view.cc:169: return true; On 2016/04/04 22:59:07, Evan Stade wrote: > On 2016/04/04 17:13:18, msw wrote: > > I think this is slightly non-obvious and deserves a comment, like: > > "Do not Accept or Cancel when closed, as most dialogs do, because..."? > > I don't know how to complete this sentence, I'm just maintaining the status quo, > which was hitherto undocumented. > > > I think global error bubbles were meant not to have an (x) close button, > because > > we wanted to encourage users to take some explicit action on them, hence the > > NOTREACHED() in the old GlobalErrorBubbleView::ButtonPressed > > Even if you had a close button and you pressed it, it would not have gone > through ButtonPressed because |this| was not registered as a listener on that > button. > > The implementation of GlobalErrorBubbleView::ShouldShowCloseButton() above makes > it seem like some of them are intended to have close buttons. You could say "so that closing via dismissal isn't treated as an explicit accept or cancel response." or even just "to maintain legacy behavior". I just think overriding Close() to skip calling Accept/Cancel merits a comment.
https://codereview.chromium.org/1846033002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/global_error_bubble_view.cc (right): https://codereview.chromium.org/1846033002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/global_error_bubble_view.cc:169: return true; On 2016/04/05 00:25:26, msw wrote: > On 2016/04/04 22:59:07, Evan Stade wrote: > > On 2016/04/04 17:13:18, msw wrote: > > > I think this is slightly non-obvious and deserves a comment, like: > > > "Do not Accept or Cancel when closed, as most dialogs do, because..."? > > > > I don't know how to complete this sentence, I'm just maintaining the status > quo, > > which was hitherto undocumented. > > > > > I think global error bubbles were meant not to have an (x) close button, > > because > > > we wanted to encourage users to take some explicit action on them, hence the > > > NOTREACHED() in the old GlobalErrorBubbleView::ButtonPressed > > > > Even if you had a close button and you pressed it, it would not have gone > > through ButtonPressed because |this| was not registered as a listener on that > > button. > > > > The implementation of GlobalErrorBubbleView::ShouldShowCloseButton() above > makes > > it seem like some of them are intended to have close buttons. > > You could say "so that closing via dismissal isn't treated as an explicit accept > or cancel response." or even just "to maintain legacy behavior". I just think > overriding Close() to skip calling Accept/Cancel merits a comment. Done.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846033002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846033002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
finnur@chromium.org changed reviewers: + finnur@chromium.org
https://codereview.chromium.org/1846033002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/global_error_bubble_view.cc (right): https://codereview.chromium.org/1846033002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/global_error_bubble_view.cc:169: return true; Late to the party... Although I did some bubble work for extensions at the same time, I don't remember being involved in the GlobalErrorBubbleView so I don't have any recollections on that front. I do seem to recall that it was Yoyo who implemented it (who left some time afterwards), but I'm not sure.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846033002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846033002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1846033002/#ps160001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846033002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846033002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Switch GlobalErrorBubbleView to a BubbleDialogDelegate. Simplify code, fix margins. BUG=585312 ========== to ========== Switch GlobalErrorBubbleView to a BubbleDialogDelegate. Simplify code, fix margins. BUG=585312 Committed: https://crrev.com/e372a77d7d302b4e1dbf55900094925e677afddb Cr-Commit-Position: refs/heads/master@{#385241} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e372a77d7d302b4e1dbf55900094925e677afddb Cr-Commit-Position: refs/heads/master@{#385241} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
