Use standard icon/title in global error bubble

Addresses the spacing issue between close button and title in global error bubble from issue 449737.

Add an optional title icon in the BubbleFrameView and use the standard title/icon in the GlobalErrorBubbleView.

BUG=449737
Committed: https://crrev.com/d9e93b432128b228d5057b5898fe42e5ba5ab140
Cr-Commit-Position: refs/heads/master@{#317927}

[msw, 2015-02-18 18:48:14]

msw@chromium.org changed reviewers:
+ msw@chromium.org

[msw, 2015-02-18 18:48:15]

https://codereview.chromium.org/933893003/diff/40001/ui/views/widget/widget_d...
File ui/views/widget/widget_delegate.h (right):

https://codereview.chromium.org/933893003/diff/40001/ui/views/widget/widget_d...
ui/views/widget/widget_delegate.h:83: virtual gfx::ImageSkia
GetWindowTitleIcon();
Use GetWindowIcon and ShouldShowWindowIcon below instead.

[xiaoling, 2015-02-18 19:03:02]

https://codereview.chromium.org/933893003/diff/40001/ui/views/widget/widget_d...
File ui/views/widget/widget_delegate.h (right):

https://codereview.chromium.org/933893003/diff/40001/ui/views/widget/widget_d...
ui/views/widget/widget_delegate.h:83: virtual gfx::ImageSkia
GetWindowTitleIcon();
On 2015/02/18 18:48:15, msw wrote:
> Use GetWindowIcon and ShouldShowWindowIcon below instead.

Can I? Excellent if I can. What's the purpose of the WindowIcon? Will I break
something if I overload the meaning of that icon?

[msw, 2015-02-18 19:14:44]

https://codereview.chromium.org/933893003/diff/40001/ui/views/widget/widget_d...
File ui/views/widget/widget_delegate.h (right):

https://codereview.chromium.org/933893003/diff/40001/ui/views/widget/widget_d...
ui/views/widget/widget_delegate.h:83: virtual gfx::ImageSkia
GetWindowTitleIcon();
On 2015/02/18 19:03:02, xiaoling wrote:
> On 2015/02/18 18:48:15, msw wrote:
> > Use GetWindowIcon and ShouldShowWindowIcon below instead.
> 
> Can I? Excellent if I can. What's the purpose of the WindowIcon? Will I break
> something if I overload the meaning of that icon?

You should definitely use those. They're already used for popup windows in
Chrome, perhaps app windows and more. Regardless, they're currently unused by
the BubbleDelegate[View] subclasses, and using them for this purpose (nearly
identical to the other window icons) is entirely reasonable.

[xiaoling, 2015-02-19 02:20:58]

Hi Michael, I updated the issue. Can you please start a formal review?

[msw, 2015-02-19 23:56:14]

Can you try to add some tests?

https://codereview.chromium.org/933893003/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/933893003/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view.cc:171: return true;
You shouldn't need to override this, WidgetDelegateView already returns true.
(unless you want to do something like [D]CHECK(!GetWindowTitle().empty()))

https://codereview.chromium.org/933893003/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view.cc:184:
CHECK(!image.IsEmpty());
Do all global error bubbles really have valid icons? Maybe DCHECK?

https://codereview.chromium.org/933893003/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/global_error_bubble_view.h (right):

https://codereview.chromium.org/933893003/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view.h:33: bool
ShouldShowWindowTitle() const override;
nit: make the declaration and definition order of the overridden functions match
their original declaration in widget_delegate.h (Get* before Should*,
WindowClosing last, etc.) Feel free to fix any out-of-order overrides elsewhere
in the subclass chain.

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (left):

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:209: bounds.set_size(title_size);
nit: the old code updated |bounds| to have the size of the title for layout of
the |titlebar_extra_view_|... You'll need to update |bounds| in the new code to
ensure that's sized and placed correctly.

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:65: : bubble_border_(NULL),
nit: please update these to nullptr.

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:72: title_icon_ = new views::ImageView();
nit: inline this in the initializer list.

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:175: gfx::ImageSkia image;
Move this into UpdateWindowIcon(), but you may need to call that from here,
because views::Widget::Init() only calls UpdateWindowTitle(), not
UpdateWindowIcon(). That's probably a bug that can be addressed separately.

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:197: title_->text().empty() ? 0 :
title_->GetPreferredSize().height();
Maybe you can title_->set_collapse_when_hidden(true); instead.

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:225: gfx::Size
title_label_size(title_->GetPreferredSize());
Wouldn't this have the same defect: non-zero height with empty text?

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:233: bounds.y() + (title_height -
title_icon_size.height()) / 2,
Did you fix vertical centering? Please post screenshots to the bug.
(maybe set the height of the label and the icon to the max of each, and rely on
them centering their contents vertically instead of using this naive centering)

[xiaoling, 2015-02-20 19:43:21]

PTAL. I'll look into how to test. Will send screenshot.

https://codereview.chromium.org/933893003/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/933893003/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view.cc:171: return true;
On 2015/02/19 23:56:14, msw wrote:
> You shouldn't need to override this, WidgetDelegateView already returns true.
> (unless you want to do something like [D]CHECK(!GetWindowTitle().empty()))

Done.

https://codereview.chromium.org/933893003/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view.cc:184:
CHECK(!image.IsEmpty());
On 2015/02/19 23:56:13, msw wrote:
> Do all global error bubbles really have valid icons? Maybe DCHECK?

Done.

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (left):

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:209: bounds.set_size(title_size);
On 2015/02/19 23:56:14, msw wrote:
> nit: the old code updated |bounds| to have the size of the title for layout of
> the |titlebar_extra_view_|... You'll need to update |bounds| in the new code
to
> ensure that's sized and placed correctly.

Ahh, I missed that. Done.

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:65: : bubble_border_(NULL),
On 2015/02/19 23:56:14, msw wrote:
> nit: please update these to nullptr.

Done.

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:72: title_icon_ = new views::ImageView();
On 2015/02/19 23:56:14, msw wrote:
> nit: inline this in the initializer list.

Done.

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:175: gfx::ImageSkia image;
On 2015/02/19 23:56:14, msw wrote:
> Move this into UpdateWindowIcon(), but you may need to call that from here,
> because views::Widget::Init() only calls UpdateWindowTitle(), not
> UpdateWindowIcon(). That's probably a bug that can be addressed separately.

Done. I made views::Widget::Init() calls UpdateWindowIcon() as well. Please let
me know if this is not correct.

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:197: title_->text().empty() ? 0 :
title_->GetPreferredSize().height();
On 2015/02/19 23:56:14, msw wrote:
> Maybe you can title_->set_collapse_when_hidden(true); instead.

Done.

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:225: gfx::Size
title_label_size(title_->GetPreferredSize());
On 2015/02/19 23:56:14, msw wrote:
> Wouldn't this have the same defect: non-zero height with empty text?

Right, I inherited the defect:(

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:233: bounds.y() + (title_height -
title_icon_size.height()) / 2,
On 2015/02/19 23:56:14, msw wrote:
> Did you fix vertical centering? Please post screenshots to the bug.
> (maybe set the height of the label and the icon to the max of each, and rely
on
> them centering their contents vertically instead of using this naive
centering)

I think yes. My naive way seems to have the same result of your suggestion, but
your way results in cleaner code. I don't know that by default the contents are
center v-aligned and I cannot find a way to set v-alignment. Thanks for the tip.

[msw, 2015-02-20 20:03:44]

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/933893003/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:175: gfx::ImageSkia image;
On 2015/02/20 19:43:21, xiaoling wrote:
> On 2015/02/19 23:56:14, msw wrote:
> > Move this into UpdateWindowIcon(), but you may need to call that from here,
> > because views::Widget::Init() only calls UpdateWindowTitle(), not
> > UpdateWindowIcon(). That's probably a bug that can be addressed separately.
> 
> Done. I made views::Widget::Init() calls UpdateWindowIcon() as well. Please
let
> me know if this is not correct.

It should be correct, thanks.

https://codereview.chromium.org/933893003/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/global_error_bubble_view.h (right):

https://codereview.chromium.org/933893003/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view.h:32: void WindowClosing()
override;
Ping! The order here (and in the code file) should be:
  base::string16 GetWindowTitle() const override;
  gfx::ImageSkia GetWindowIcon() override;
  bool ShouldShowWindowIcon() const override;
  void WindowClosing() override;

https://codereview.chromium.org/933893003/diff/140001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/933893003/diff/140001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:184: // Update close button visibility too,
otherwise they are not intialized.
nit: "they are" is incorrect, leave this as "it's" or "it is"

https://codereview.chromium.org/933893003/diff/140001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:185: ResetWindowControls();
I added this line as a workaround for a bug that was recently fixed by
<https://codereview.chromium.org/133073010>. Please remove this line and the
comment above, but be sure that the close (x) button still appears as needed.

https://codereview.chromium.org/933893003/diff/140001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:252: const int extra_width = close_->x() -
title_->bounds().right();
Change this to handle the case with an icon but no title:
  const int extra_width = close_->x() - bounds().right();

[xiaoling, 2015-02-20 20:23:19]

Thank you!

https://codereview.chromium.org/933893003/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/global_error_bubble_view.h (right):

https://codereview.chromium.org/933893003/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view.h:32: void WindowClosing()
override;
On 2015/02/20 20:03:44, msw wrote:
> Ping! The order here (and in the code file) should be:
>   base::string16 GetWindowTitle() const override;
>   gfx::ImageSkia GetWindowIcon() override;
>   bool ShouldShowWindowIcon() const override;
>   void WindowClosing() override;

Done.

https://codereview.chromium.org/933893003/diff/140001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/933893003/diff/140001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:184: // Update close button visibility too,
otherwise they are not intialized.
On 2015/02/20 20:03:44, msw wrote:
> nit: "they are" is incorrect, leave this as "it's" or "it is"

I changed some code and then reverted. But didn't revert the comment :(

https://codereview.chromium.org/933893003/diff/140001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:185: ResetWindowControls();
On 2015/02/20 20:03:44, msw wrote:
> I added this line as a workaround for a bug that was recently fixed by
> <https://codereview.chromium.org/133073010>. Please remove this line and the
> comment above, but be sure that the close (x) button still appears as needed.

Done.

https://codereview.chromium.org/933893003/diff/140001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:252: const int extra_width = close_->x() -
title_->bounds().right();
On 2015/02/20 20:03:44, msw wrote:
> Change this to handle the case with an icon but no title:
>   const int extra_width = close_->x() - bounds().right();

Done.

[msw, 2015-02-20 21:58:44]

lgtm. it'd be nice to have tests in this CL or a followup.

[xiaoling, 2015-02-20 22:31:41]

On 2015/02/20 21:58:44, msw wrote:
> lgtm. it'd be nice to have tests in this CL or a followup.

ty. I'll try to add tests in followup.

[xiaoling, 2015-02-20 23:46:02]

The CQ bit was checked by xiaolingbao@chromium.org

[commit-bot: I haz the power, 2015-02-20 23:47:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/933893003/180001

[commit-bot: I haz the power, 2015-02-21 00:34:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-21 00:34:49]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xiaoling, 2015-02-21 00:37:13]

xiaolingbao@chromium.org changed reviewers:
+ sky@chromium.org

[xiaoling, 2015-02-21 00:38:51]

On 2015/02/21 00:34:49, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

+sky. @sky: Can you please take a look, particularly at widget.cc? I need
owner's lgtm for that file. Thanks!

[sky, 2015-02-21 04:21:24]

LGTM

[xiaoling, 2015-02-24 22:03:01]

Hi,

I uploaded another patch to fix some test failures. Most changes are in file
dialog_delegate_unittest.cc. Also added icon-ptr null-checking in panel.cc and
opaque_browser_frame_view.cc. Can you please take another look? Thank you!

[msw, 2015-02-24 22:19:46]

lgtm with comment nits.

https://codereview.chromium.org/933893003/diff/200001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate_unittest.cc (right):

https://codereview.chromium.org/933893003/diff/200001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:192: // Ensure that the new style's
BubbleFrameView hit-tests as expected
nit: remove "the new style's" and s/when title/when the title/. This should be a
one-liner as "// Ensure that BubbleFrameView hit-tests as expected when the
title is hidden."

https://codereview.chromium.org/933893003/diff/200001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:218: // Ensure that the new style's
BubbleFrameView hit-tests as expected
nit: remove "the new style's" here and below; s/when title is present/when the
title is /. This should be a one-liner as "// Ensure that BubbleFrameView
hit-tests as expected when the title is shown."

[xiaoling, 2015-02-24 22:38:29]

Thanks!

https://codereview.chromium.org/933893003/diff/200001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate_unittest.cc (right):

https://codereview.chromium.org/933893003/diff/200001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:192: // Ensure that the new style's
BubbleFrameView hit-tests as expected
On 2015/02/24 22:19:46, msw wrote:
> nit: remove "the new style's" and s/when title/when the title/. This should be
a
> one-liner as "// Ensure that BubbleFrameView hit-tests as expected when the
> title is hidden."

Done.

https://codereview.chromium.org/933893003/diff/200001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:218: // Ensure that the new style's
BubbleFrameView hit-tests as expected
On 2015/02/24 22:19:46, msw wrote:
> nit: remove "the new style's" here and below; s/when title is present/when the
> title is /. This should be a one-liner as "// Ensure that BubbleFrameView
> hit-tests as expected when the title is shown."

Done.

[xiaoling, 2015-02-24 23:02:07]

The CQ bit was checked by xiaolingbao@chromium.org

[xiaoling, 2015-02-24 23:02:07]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/933893003/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-02-24 23:04:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/933893003/220001

[commit-bot: I haz the power, 2015-02-24 23:21:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-24 23:21:59]

Try jobs failed on following builders:
  ios_rel_device_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[xiaoling, 2015-02-24 23:28:25]

The CQ bit was checked by xiaolingbao@chromium.org

[xiaoling, 2015-02-24 23:28:25]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/933893003/#ps260001
(title: " ")

[commit-bot: I haz the power, 2015-02-24 23:28:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/933893003/260001

[commit-bot: I haz the power, 2015-02-25 00:35:08]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-02-25 00:35:40]

Patchset 14 (id:??) landed as
https://crrev.com/d9e93b432128b228d5057b5898fe42e5ba5ab140
Cr-Commit-Position: refs/heads/master@{#317927}