Message center re-organized

Message center re-organized

The message center has been re-organized to get rid of unused classes
since we don't need the rounded corners and the arrow. This redesign
affects Linux (Aura) and Windows builds

BUG=249482, 259577
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212007

[dewittj, 2013-06-28 01:13:18]

partial review comments

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/notificati...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/notificati...
chrome/browser/notifications/message_center_settings_controller.cc:79: // the
default profile is not loaded.
I suspect you should actually use GetLastUsedProfileAllowedByPolicy.

Actually, though, we might want to merge this into m29 separately.  Could you
make a separate patch including just this change?

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_frame_view.cc
(right):

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_frame_view.cc:12: const
SkColor kBorderColor = SkColorSetARGB(0xFF, 0xC7, 0xCA, 0xCE);
Where did this color come from?

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_frame_view.cc:33: return
gfx::Rect(client_bounds.size());
This might be why you don't see the border on Windows.

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.cc
(right):

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:101:
min_width_ = max_width_ = kNotificationWidth;
Why not just have 'width_'?

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:369:
max_height_ -= std::abs(mouse_click_point.y() - corner.y());
#include <complex>

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.h
(right):

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:99:
SkColor color() const { return color_; }
Can this be private?

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:100:
void set_color(SkColor color) {
Is this method used anywhere?

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:105:
bool close_on_esc() const { return close_on_esc_; }
private

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:106:
void set_close_on_esc(bool close_on_esc) { close_on_esc_ = close_on_esc; }
remove

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:108:
bool close_on_deactivate() const { return close_on_deactivate_; }
private

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:109:
void set_close_on_deactivate(bool close) { close_on_deactivate_ = close; }
remove

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:113:
virtual void AnimationEnded(const ui::Animation* animation) OVERRIDE;
Do you ever start an animation?  If not, remove these.

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:139:
bool color_explicitly_set_;
I suspect this is unnecessarily copied.

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/web_notification_tray.h (right):

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.h:39: public
views::WidgetObserver,
This doesn't appear to be used in this class.

https://codereview.chromium.org/18003003/diff/10001/ui/message_center/views/m...
File ui/message_center/views/message_center_view.h (right):

https://codereview.chromium.org/18003003/diff/10001/ui/message_center/views/m...
ui/message_center/views/message_center_view.h:78: void Init(int max_height);
This method doesn't seem necessary.

[sidharthms, 2013-07-02 18:09:51]

I've made some changes. Please take a look

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/notificati...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/notificati...
chrome/browser/notifications/message_center_settings_controller.cc:79: // the
default profile is not loaded.
On 2013/06/28 01:13:18, dewittj wrote:
> I suspect you should actually use GetLastUsedProfileAllowedByPolicy.
> 
> Actually, though, we might want to merge this into m29 separately.  Could you
> make a separate patch including just this change?

Done.

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_frame_view.cc
(right):

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_frame_view.cc:12: const
SkColor kBorderColor = SkColorSetARGB(0xFF, 0xC7, 0xCA, 0xCE);
On 2013/06/28 01:13:18, dewittj wrote:
> Where did this color come from?

Done. Moved to MessageCenterStyle

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_frame_view.cc:33: return
gfx::Rect(client_bounds.size());
On 2013/06/28 01:13:18, dewittj wrote:
> This might be why you don't see the border on Windows.

Done.

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.cc
(right):

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:101:
min_width_ = max_width_ = kNotificationWidth;
On 2013/06/28 01:13:18, dewittj wrote:
> Why not just have 'width_'?

Done.

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:369:
max_height_ -= std::abs(mouse_click_point.y() - corner.y());
On 2013/06/28 01:13:18, dewittj wrote:
> #include <complex>

Done.

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.h
(right):

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:99:
SkColor color() const { return color_; }
On 2013/06/28 01:13:18, dewittj wrote:
> Can this be private?

Done. Actually removed

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:100:
void set_color(SkColor color) {
On 2013/06/28 01:13:18, dewittj wrote:
> Is this method used anywhere?

Done.

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:105:
bool close_on_esc() const { return close_on_esc_; }
On 2013/06/28 01:13:18, dewittj wrote:
> private

Done.

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:106:
void set_close_on_esc(bool close_on_esc) { close_on_esc_ = close_on_esc; }
On 2013/06/28 01:13:18, dewittj wrote:
> remove

Done.

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:108:
bool close_on_deactivate() const { return close_on_deactivate_; }
On 2013/06/28 01:13:18, dewittj wrote:
> private

Done.

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:109:
void set_close_on_deactivate(bool close) { close_on_deactivate_ = close; }
On 2013/06/28 01:13:18, dewittj wrote:
> remove

Done.

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:113:
virtual void AnimationEnded(const ui::Animation* animation) OVERRIDE;
On 2013/06/28 01:13:18, dewittj wrote:
> Do you ever start an animation?  If not, remove these.

Done.

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:139:
bool color_explicitly_set_;
On 2013/06/28 01:13:18, dewittj wrote:
> I suspect this is unnecessarily copied.

Done.

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/web_notification_tray.h (right):

https://codereview.chromium.org/18003003/diff/10001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.h:39: public
views::WidgetObserver,
On 2013/06/28 01:13:18, dewittj wrote:
> This doesn't appear to be used in this class.

Done.

https://codereview.chromium.org/18003003/diff/10001/ui/message_center/views/m...
File ui/message_center/views/message_center_view.h (right):

https://codereview.chromium.org/18003003/diff/10001/ui/message_center/views/m...
ui/message_center/views/message_center_view.h:78: void Init(int max_height);
On 2013/06/28 01:13:18, dewittj wrote:
> This method doesn't seem necessary.

Done. Refactored

[dewittj, 2013-07-02 18:43:32]

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.cc
(right):

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:38:
pos_info.bubble_alignment & ALIGNMENT_TOP /* Show buttons on top if
I might just change this constructor to accept bubble_alignment, instead of
casting a bitwise and into a boolean.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:51:
UpdateColorsFromTheme(GetNativeTheme());
Should this be called on Windows?

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:59:
UpdateNotifications();
This method is at least misnamed (since it also shows the widget and does some
other widgety stuff) and may be better off inlined here (since nothing else
calls it.)

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:109:
size.set_width(preferred_width_);
This is smaller than the preferred size above.  Should it just return
GetPreferredSize()?

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:119:
const ui::Accelerator& accelerator) {
Not sure, do we need to pass these through to the message center view?

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:168:
GetWidget()->SetBounds(GetBubbleBounds());
Seems like it would make more sense to set the bounds before you show it to
prevent artifacts.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:169:
GetWidget()->GetRootView()->SchedulePaint();
Does it not paint the widget when you show it?

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.h
(right):

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:22:
class Rect;
I suspect that you can remove this and should add #include ui/gfx/rect.h
instead, since you need the full definition of Rect, not just Rect* in this
file.  Also #include ui/gfx/point.h.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:30:
class StatusIcon;
I don't think this is necessary.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:39:
ALIGNMENT_NONE = 16
It is more idiomatic to describe these as 1 << x rather than the decimal
equivalent.  See
https://code.google.com/p/chromium/codesearch#search/&q=%22%3C%3C%204,%22%20f...
for examples.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:46:
Alignment bubble_alignment;
Please rename this since this class is not related to bubbles.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:52: //
center. But since we don't know the size until
This comment is not complete!

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:59:
class MessageCenterWidgetDelegate : public views::WidgetDelegate,
Please provide a class comment.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:64: //
align itself.
Don't refer to bubble here either.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:69:
PositionInfo pos_info);
probably should pass this in as a const PositionInfo&.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:91: void
InitWidget();
Move this to private.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:95: //
the bubble contents.
Don't refer to bubble here either.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:98: //
Update the bubble color from |theme|, unless it was explicitly set.
Don't refer to bubble here either.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:102:
MessageCenterFrameView* GetBubbleFrameView() const;
I don't think anyone calls this function.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:103:
gfx::Rect GetBubbleBounds();
Move to private.  Also, rename since this isn't really a bubble anymore.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:111: //
The background color of the bubble; and flag for when it's explicitly set.
Don't refer to bubble here either.  Remove reference to flag when it's
explicitly set.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/web_notification_tray.h (right):

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.h:93:
MessageCenterWidgetDelegate* message_center_delegate_;
This is troublesome since the widget owns the messagecenterwidgetdelegate.  So
your pointer could become stale after closing the widget, leading to
use-after-free type bugs.  So somehow you will have to notify the
webnotificationtray that the widgetdelegate has been deleted.

https://codereview.chromium.org/18003003/diff/21001/ui/message_center/views/m...
File ui/message_center/views/message_center_view.cc (right):

https://codereview.chromium.org/18003003/diff/21001/ui/message_center/views/m...
ui/message_center/views/message_center_view.cc:1147: if (animation ==
settings_transition_animation_.get()) {
Why remove the DCHECK?

https://codereview.chromium.org/18003003/diff/21001/ui/message_center/views/m...
ui/message_center/views/message_center_view.cc:1161: if (animation ==
settings_transition_animation_.get()) {
why remove the DCHECK?

https://codereview.chromium.org/18003003/diff/21001/ui/views/bubble/bubble_fr...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/18003003/diff/21001/ui/views/bubble/bubble_fr...
ui/views/bubble/bubble_frame_view.cc:83:
client_bounds.Inset(bubble_border_->GetInsets());
Why are you changing bubble_frame_view?

[sidharthms, 2013-07-02 21:01:16]

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.cc
(right):

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:38:
pos_info.bubble_alignment & ALIGNMENT_TOP /* Show buttons on top if
But MessageCenterView is also used in other places, right? They don't use our
alignment enum.

Also the "buttons_on_top" parameter currently has a default value. Is that okay?

On 2013/07/02 18:43:32, dewittj wrote:
> I might just change this constructor to accept bubble_alignment, instead of
> casting a bitwise and into a boolean.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:51:
UpdateColorsFromTheme(GetNativeTheme());
Eh, wouldn't it be better to use native themes?

On 2013/07/02 18:43:32, dewittj wrote:
> Should this be called on Windows?

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:59:
UpdateNotifications();
On 2013/07/02 18:43:32, dewittj wrote:
> This method is at least misnamed (since it also shows the widget and does some
> other widgety stuff) and may be better off inlined here (since nothing else
> calls it.)

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:109:
size.set_width(preferred_width_);
On 2013/07/02 18:43:32, dewittj wrote:
> This is smaller than the preferred size above.  Should it just return
> GetPreferredSize()?

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:119:
const ui::Accelerator& accelerator) {
MessageCenterView doesn't seem to handle these...

On 2013/07/02 18:43:32, dewittj wrote:
> Not sure, do we need to pass these through to the message center view?

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:168:
GetWidget()->SetBounds(GetBubbleBounds());
On 2013/07/02 18:43:32, dewittj wrote:
> Seems like it would make more sense to set the bounds before you show it to
> prevent artifacts.

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:169:
GetWidget()->GetRootView()->SchedulePaint();
On 2013/07/02 18:43:32, dewittj wrote:
> Does it not paint the widget when you show it?

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.h
(right):

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:22:
class Rect;
On 2013/07/02 18:43:32, dewittj wrote:
> I suspect that you can remove this and should add #include ui/gfx/rect.h
> instead, since you need the full definition of Rect, not just Rect* in this
> file.  Also #include ui/gfx/point.h.

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:30:
class StatusIcon;
On 2013/07/02 18:43:32, dewittj wrote:
> I don't think this is necessary.

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:39:
ALIGNMENT_NONE = 16
On 2013/07/02 18:43:32, dewittj wrote:
> It is more idiomatic to describe these as 1 << x rather than the decimal
> equivalent.  See
>
https://code.google.com/p/chromium/codesearch#search/&q=%2522%253C%253C%25204...
> for examples.

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:46:
Alignment bubble_alignment;
On 2013/07/02 18:43:32, dewittj wrote:
> Please rename this since this class is not related to bubbles.

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:52: //
center. But since we don't know the size until
On 2013/07/02 18:43:32, dewittj wrote:
> This comment is not complete! 

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:59:
class MessageCenterWidgetDelegate : public views::WidgetDelegate,
On 2013/07/02 18:43:32, dewittj wrote:
> Please provide a class comment.

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:64: //
align itself.
On 2013/07/02 18:43:32, dewittj wrote:
> Don't refer to bubble here either.

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:69:
PositionInfo pos_info);
So you mean the private instance (pos_info_) should also be a constant ref? I
thought that might create scope issues (since the pos_info originally passed
would have gone out of scope).

On 2013/07/02 18:43:32, dewittj wrote:
> probably should pass this in as a const PositionInfo&.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:91: void
InitWidget();
On 2013/07/02 18:43:32, dewittj wrote:
> Move this to private.

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:95: //
the bubble contents.
On 2013/07/02 18:43:32, dewittj wrote:
> Don't refer to bubble here either.

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:98: //
Update the bubble color from |theme|, unless it was explicitly set.
On 2013/07/02 18:43:32, dewittj wrote:
> Don't refer to bubble here either.

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:102:
MessageCenterFrameView* GetBubbleFrameView() const;
On 2013/07/02 18:43:32, dewittj wrote:
> I don't think anyone calls this function.

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:103:
gfx::Rect GetBubbleBounds();
On 2013/07/02 18:43:32, dewittj wrote:
> Move to private.  Also, rename since this isn't really a bubble anymore.

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:111: //
The background color of the bubble; and flag for when it's explicitly set.
On 2013/07/02 18:43:32, dewittj wrote:
> Don't refer to bubble here either.  Remove reference to flag when it's
> explicitly set.

Done.

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/web_notification_tray.h (right):

https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.h:93:
MessageCenterWidgetDelegate* message_center_delegate_;
The MessageCenterWidget shouldn't ever be closing by itself. It should only be
closed by the WebNotificationTray by calling CloseWidget. So I have changed it
so that WebNotificationTray set's the delegate pointer to NULL when the widget
closes. Would that be okay?

On 2013/07/02 18:43:32, dewittj wrote:
> This is troublesome since the widget owns the messagecenterwidgetdelegate.  So
> your pointer could become stale after closing the widget, leading to
> use-after-free type bugs.  So somehow you will have to notify the
> webnotificationtray that the widgetdelegate has been deleted.

https://codereview.chromium.org/18003003/diff/21001/ui/message_center/views/m...
File ui/message_center/views/message_center_view.cc (right):

https://codereview.chromium.org/18003003/diff/21001/ui/message_center/views/m...
ui/message_center/views/message_center_view.cc:1147: if (animation ==
settings_transition_animation_.get()) {
On 2013/07/02 18:43:32, dewittj wrote:
> Why remove the DCHECK?

Done.

https://codereview.chromium.org/18003003/diff/21001/ui/message_center/views/m...
ui/message_center/views/message_center_view.cc:1161: if (animation ==
settings_transition_animation_.get()) {
On 2013/07/02 18:43:32, dewittj wrote:
> why remove the DCHECK?

Done.

https://codereview.chromium.org/18003003/diff/21001/ui/views/bubble/bubble_fr...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/18003003/diff/21001/ui/views/bubble/bubble_fr...
ui/views/bubble/bubble_frame_view.cc:83:
client_bounds.Inset(bubble_border_->GetInsets());
On 2013/07/02 18:43:32, dewittj wrote:
> Why are you changing bubble_frame_view?

Done. This was not meant to be part of the change.

[dewittj, 2013-07-02 23:03:12]

On 2013/07/02 21:01:16, sidharthms wrote:
>
https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
> File chrome/browser/ui/views/message_center/message_center_widget_delegate.cc
> (right):
> 
>
https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
> chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:38:
> pos_info.bubble_alignment & ALIGNMENT_TOP /* Show buttons on top if
> But MessageCenterView is also used in other places, right? They don't use our
> alignment enum.

Okay, that's understandable.

> Also the "buttons_on_top" parameter currently has a default value. Is that
okay?

I would not use a default value.

> 
> On 2013/07/02 18:43:32, dewittj wrote:
> > I might just change this constructor to accept bubble_alignment, instead of
> > casting a bitwise and into a boolean.
> 
>
https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
> chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:51:
> UpdateColorsFromTheme(GetNativeTheme());
> Eh, wouldn't it be better to use native themes?

We have very specific colors from UX design for the message center.  Unless we
get explicit advice to use the theme (or it was already there in some code for
message center) I would use the colors they have prescribed.


>
https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
> chrome/browser/ui/views/message_center/message_center_widget_delegate.h:69:
> PositionInfo pos_info);
> So you mean the private instance (pos_info_) should also be a constant ref? I
> thought that might create scope issues (since the pos_info originally passed
> would have gone out of scope).

I mean that the parameter should be a const ref, and pos_info_ should be a copy
of that const ref.

> 
> On 2013/07/02 18:43:32, dewittj wrote:
> > probably should pass this in as a const PositionInfo&.
> 

>
https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
> File chrome/browser/ui/views/message_center/web_notification_tray.h (right):
> 
>
https://codereview.chromium.org/18003003/diff/21001/chrome/browser/ui/views/m...
> chrome/browser/ui/views/message_center/web_notification_tray.h:93:
> MessageCenterWidgetDelegate* message_center_delegate_;
> The MessageCenterWidget shouldn't ever be closing by itself. It should only be
> closed by the WebNotificationTray by calling CloseWidget. So I have changed it
> so that WebNotificationTray set's the delegate pointer to NULL when the widget
> closes. Would that be okay?

I'm worried that there could be a case where the widget is closed manually by
the user or the OS.  Could you just implement OnWidgetClosing() in
WidgetObserver and null out the pointer there?

> 
> On 2013/07/02 18:43:32, dewittj wrote:
> > This is troublesome since the widget owns the messagecenterwidgetdelegate. 
So
> > your pointer could become stale after closing the widget, leading to
> > use-after-free type bugs.  So somehow you will have to notify the
> > webnotificationtray that the widgetdelegate has been deleted.
>

[sidharthms, 2013-07-13 01:11:11]

sky@ please review the files in "ui/views/" and the chrome_browser_ui.gypi file

[msw, 2013-07-13 01:21:36]

Just a drive-by comment.

https://codereview.chromium.org/18003003/diff/89001/ui/views/shadow_border.cc
File ui/views/shadow_border.cc (right):

https://codereview.chromium.org/18003003/diff/89001/ui/views/shadow_border.cc...
ui/views/shadow_border.cc:27: SkPaint paint;
Re-painting a shadow looper on every paint call may yield poor performance. This
border's appearance should probably be shipped in image assets (like other
border assets, e.g. ui/resources/default_100_percent/common/window_*), or they
should be drawn with skia once and cached to bitmap(s) for repeated bit blitting
(I hope Chrome will start doing that more often instead of shipping image
assets; for custom color support, binary sizes, etc.).

[sidharthms, 2013-07-15 03:03:02]

I've added caching to the shadow borders. Please check if it's done right.

On 2013/07/13 01:21:36, msw wrote:
> Just a drive-by comment.
> 
> https://codereview.chromium.org/18003003/diff/89001/ui/views/shadow_border.cc
> File ui/views/shadow_border.cc (right):
> 
>
https://codereview.chromium.org/18003003/diff/89001/ui/views/shadow_border.cc...
> ui/views/shadow_border.cc:27: SkPaint paint;
> Re-painting a shadow looper on every paint call may yield poor performance.
This
> border's appearance should probably be shipped in image assets (like other
> border assets, e.g. ui/resources/default_100_percent/common/window_*), or they
> should be drawn with skia once and cached to bitmap(s) for repeated bit
blitting
> (I hope Chrome will start doing that more often instead of shipping image
> assets; for custom color support, binary sizes, etc.).

[msw, 2013-07-15 17:43:03]

On 2013/07/15 03:03:02, sidharthms wrote:
> I've added caching to the shadow borders. Please check if it's done right.
> 
>
https://codereview.chromium.org/18003003/diff/89001/ui/views/shadow_border.cc...
> > ui/views/shadow_border.cc:27: SkPaint paint;
> > Re-painting a shadow looper on every paint call may yield poor performance.

That bit looks okay to me, but don't feel pressured to include it in this CL.
You can leave the perf analysis and caching impl as a follow-up, if you wish.
As you explained via e-mail, you're just moving this code from message_view.cc

[dewittj, 2013-07-15 18:38:58]

First-pass comments.

https://codereview.chromium.org/18003003/diff/85001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.cc
(right):

https://codereview.chromium.org/18003003/diff/85001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:204: //
push the message center UPWARDS to align with the click point.
all-caps is not really chromium style.  If it is extra important, please explain
why.

https://codereview.chromium.org/18003003/diff/85001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/web_notification_tray.cc (right):

https://codereview.chromium.org/18003003/diff/85001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.cc:80: Alignment
GetSystrayAlignment() {
In the future we could use AppBar for this on Windows.  Perhaps a
TODO(sidharthms) would be good as a reminder.

https://codereview.chromium.org/18003003/diff/85001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.cc:121: // Gets the
corner of the screen where the message center should pop up.
This comment probably belongs on GetClosestCorner?

https://codereview.chromium.org/18003003/diff/85001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.cc:175:
show_settings, /* settings initally invisible */
//-style comment here.

https://codereview.chromium.org/18003003/diff/85001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/web_notification_tray.h (right):

https://codereview.chromium.org/18003003/diff/85001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.h:93:
MessageCenterWidgetDelegate* message_center_delegate_;
Rename this to message_center_widget_delegate_

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_frame_view.cc
(right):

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_frame_view.cc:22: #if
defined OS_WIN
Style is defined(OS_WIN).

Also is there any way to detect if transparency is supported?  That would
probably be a better switch than a compile-time flag.

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_frame_view.h (right):

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_frame_view.h:30: OVERRIDE
{}
I believe that we prefer to put the definitions of virtual overrides
out-of-line, in the cc file.  See
http://www.chromium.org/developers/coding-style, section "Inline functions"

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_frame_view.h:39: static
gfx::Insets GetBorderInsets();
Comment here please.  Why is this static?  Can this just return the result of
border->GetInsets()?

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.cc
(right):

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:119: }
blank line after this one.

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.h
(right):

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:58:
class MessageCenterWidgetDelegate : public views::WidgetDelegate,
Since this is a WidgetDelegate and a View, I wonder if you should follow the
same pattern as WidgetDelegateView with respect to ownership.  In that case, it
calls set_owned_by_client() and it deletes itself on
WidgetDelegate::DeleteDelegate.  This might prevent race conditions where the
view hierarchy has been cleaned up but the widget delegate still needs to be
alive for some reason.

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:87:
protected:
Remove this empty section.

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:92:
gfx::Rect GetBubbleBounds();
I know they're not public but we should have some comments on the above private
functions.  Especially GetCorrectedAnchor - explain the algorithm you're using
to choose the anchor point.

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/web_notification_tray.h (right):

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.h:68: void
HideBubble();
Rename so that we don't refer to Bubble.

https://codereview.chromium.org/18003003/diff/95001/ui/message_center/message...
File ui/message_center/message_center_tray.h (right):

https://codereview.chromium.org/18003003/diff/95001/ui/message_center/message...
ui/message_center/message_center_tray.h:42: bool HideMessageCenterBubble();
Please add to the comment for this method that it actually causes the message
center to be hidden.

https://codereview.chromium.org/18003003/diff/95001/ui/message_center/message...
ui/message_center/message_center_tray.h:46: void SetMessageCenterHidden();
Please rename this to MarkMessageCenterHidden to better imply that it should
already be hiding itself.

https://codereview.chromium.org/18003003/diff/95001/ui/message_center/views/m...
File ui/message_center/views/message_center_view_unittest.cc (right):

https://codereview.chromium.org/18003003/diff/95001/ui/message_center/views/m...
ui/message_center/views/message_center_view_unittest.cc:146: &message_center_,
NULL, 100, false, false /* buttons not at top */));
In this case, either do a //-style comment at the end, or do
/*buttons_on_top=*/false

https://codereview.chromium.org/18003003/diff/95001/ui/views/shadow_border.cc
File ui/views/shadow_border.cc (right):

https://codereview.chromium.org/18003003/diff/95001/ui/views/shadow_border.cc...
ui/views/shadow_border.cc:47: cached_border_.reset(new
gfx::ImageSkia(border_canvas.ExtractImageRep()));
Does this cache an image the size of the contained view?  I wonder if storing 4
separate images, one for each edge, would be better.  Message center can be the
height of the screen currently, so this would mean storing a 2000x380 or so
image, which is not a negligible size (around 18MB RAM uncompressed).

[dewittj, 2013-07-15 18:45:13]

er, 18 megabits or 2.3ish MB

[sidharthms, 2013-07-15 23:16:36]

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_frame_view.cc
(right):

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_frame_view.cc:22: #if
defined OS_WIN
On 2013/07/15 18:38:58, dewittj wrote:
> Style is defined(OS_WIN).
> 
> Also is there any way to detect if transparency is supported?  That would
> probably be a better switch than a compile-time flag.

Done. It doesn't look like we can detect if the platform supports transparency.
Currently it just silently fails.

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_frame_view.h (right):

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_frame_view.h:30: OVERRIDE
{}
On 2013/07/15 18:38:58, dewittj wrote:
> I believe that we prefer to put the definitions of virtual overrides
> out-of-line, in the cc file.  See
> http://www.chromium.org/developers/coding-style, section "Inline functions"

Done.

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_frame_view.h:39: static
gfx::Insets GetBorderInsets();
Done. Actually removed.

On 2013/07/15 18:38:58, dewittj wrote:
> Comment here please.  Why is this static?  Can this just return the result of
> border->GetInsets()?

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.cc
(right):

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:119: }
On 2013/07/15 18:38:58, dewittj wrote:
> blank line after this one.

Done.

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.h
(right):

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:58:
class MessageCenterWidgetDelegate : public views::WidgetDelegate,
On 2013/07/15 18:38:58, dewittj wrote:
> Since this is a WidgetDelegate and a View, I wonder if you should follow the
> same pattern as WidgetDelegateView with respect to ownership.  In that case,
it
> calls set_owned_by_client() and it deletes itself on
> WidgetDelegate::DeleteDelegate.  This might prevent race conditions where the
> view hierarchy has been cleaned up but the widget delegate still needs to be
> alive for some reason.

Done.

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:87:
protected:
On 2013/07/15 18:38:58, dewittj wrote:
> Remove this empty section.

Done.

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/message_center_widget_delegate.h:92:
gfx::Rect GetBubbleBounds();
On 2013/07/15 18:38:58, dewittj wrote:
> I know they're not public but we should have some comments on the above
private
> functions.  Especially GetCorrectedAnchor - explain the algorithm you're using
> to choose the anchor point.

Done.

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
File chrome/browser/ui/views/message_center/web_notification_tray.h (right):

https://codereview.chromium.org/18003003/diff/95001/chrome/browser/ui/views/m...
chrome/browser/ui/views/message_center/web_notification_tray.h:68: void
HideBubble();
On 2013/07/15 18:38:58, dewittj wrote:
> Rename so that we don't refer to Bubble.

Done.

https://codereview.chromium.org/18003003/diff/95001/ui/message_center/message...
File ui/message_center/message_center_tray.h (right):

https://codereview.chromium.org/18003003/diff/95001/ui/message_center/message...
ui/message_center/message_center_tray.h:42: bool HideMessageCenterBubble();
On 2013/07/15 18:38:58, dewittj wrote:
> Please add to the comment for this method that it actually causes the message
> center to be hidden.

Done.

https://codereview.chromium.org/18003003/diff/95001/ui/message_center/message...
ui/message_center/message_center_tray.h:46: void SetMessageCenterHidden();
On 2013/07/15 18:38:58, dewittj wrote:
> Please rename this to MarkMessageCenterHidden to better imply that it should
> already be hiding itself.

Done.

https://codereview.chromium.org/18003003/diff/95001/ui/message_center/views/m...
File ui/message_center/views/message_center_view_unittest.cc (right):

https://codereview.chromium.org/18003003/diff/95001/ui/message_center/views/m...
ui/message_center/views/message_center_view_unittest.cc:146: &message_center_,
NULL, 100, false, false /* buttons not at top */));
On 2013/07/15 18:38:58, dewittj wrote:
> In this case, either do a //-style comment at the end, or do
> /*buttons_on_top=*/false

Done.

https://codereview.chromium.org/18003003/diff/95001/ui/views/shadow_border.cc
File ui/views/shadow_border.cc (right):

https://codereview.chromium.org/18003003/diff/95001/ui/views/shadow_border.cc...
ui/views/shadow_border.cc:47: cached_border_.reset(new
gfx::ImageSkia(border_canvas.ExtractImageRep()));
Removed for now. I'll submit a separate cl to add caching.

On 2013/07/15 18:38:58, dewittj wrote:
> Does this cache an image the size of the contained view?  I wonder if storing
4
> separate images, one for each edge, would be better.  Message center can be
the
> height of the screen currently, so this would mean storing a 2000x380 or so
> image, which is not a negligible size (around 18MB RAM uncompressed).

[dewittj, 2013-07-16 00:21:28]

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
File chrome/browser/ui/views/message_center/web_notification_tray.cc (right):

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray.cc:153:
GetPositionInfo());
Now you have a potential use-after-free bug because MessageCenterWidgetDelegates
free themselves.  So in the DeleteDelegate call you'll need to erase this
pointer as well.

https://codereview.chromium.org/18003003/diff/114001/ui/message_center/views/...
File ui/message_center/views/message_center_view_unittest.cc (right):

https://codereview.chromium.org/18003003/diff/114001/ui/message_center/views/...
ui/message_center/views/message_center_view_unittest.cc:146: &message_center_,
NULL, 100, false, /* buttons_on_top = */ false));
I believe the style is without spaces in the /**/ comment.

[sidharthms, 2013-07-16 01:03:19]

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
File chrome/browser/ui/views/message_center/web_notification_tray.cc (right):

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray.cc:153:
GetPositionInfo());
Comment added. Done.

https://codereview.chromium.org/18003003/diff/114001/ui/message_center/views/...
File ui/message_center/views/message_center_view_unittest.cc (right):

https://codereview.chromium.org/18003003/diff/114001/ui/message_center/views/...
ui/message_center/views/message_center_view_unittest.cc:146: &message_center_,
NULL, 100, false, /* buttons_on_top = */ false));
On 2013/07/16 00:21:29, dewittj wrote:
> I believe the style is without spaces in the /**/ comment.

Done.

[dewittj, 2013-07-16 18:24:01]

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
File chrome/browser/ui/views/message_center/message_center_frame_view.cc
(right):

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/message_center_frame_view.cc:22: #if
defined(OS_WIN)
I'd prefer if the condition were opposite, so that the default is to have a
shadowborder, and the exception on Linux is to have a solid one.

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/message_center_frame_view.cc:58:
gfx::Path* window_mask) {}
Make this match the style of the others, put the } on the next line.

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.cc
(right):

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:113:
size.Enlarge(border_insets_.width(), border_insets_.height());
I am surprised you need to enlarge by the border size here.  GetPreferredSize is
for the view, not the widget, and the view size doesn't contain the border,
right?  Feel free to correct me if I'm wrong, but I don't see references to any
border in other overrides of GetPreferredSize in WidgetDelegates.

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
File chrome/browser/ui/views/message_center/web_notification_tray.cc (right):

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray.cc:152:
show_settings, /* settings initally invisible */
-> // style

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray.cc:159: return
ShowMessageCenterInternal(false /* show_settings */);
/*show_settings=*/ false please :)

https://codereview.chromium.org/18003003/diff/114001/ui/message_center/views/...
File ui/message_center/views/message_center_bubble.cc (right):

https://codereview.chromium.org/18003003/diff/114001/ui/message_center/views/...
ui/message_center/views/message_center_bubble.cc:97: buttons are always on
bottom for chrome os. */);
s/This class/MessageCenterBubble/
s/buttons/Buttons/
s/chrome os/ChromeOS/g

https://codereview.chromium.org/18003003/diff/114001/ui/views/shadow_border.h
File ui/views/shadow_border.h (right):

https://codereview.chromium.org/18003003/diff/114001/ui/views/shadow_border.h...
ui/views/shadow_border.h:36: // Number of pixels to shift shaodw to bottom.
s/shaodw/shadow/

https://codereview.chromium.org/18003003/diff/114001/ui/views/shadow_border.h...
ui/views/shadow_border.h:39: // Number of pixels to shift shaodw to right.
s/shaodw/shadow/

[sidharthms, 2013-07-16 21:31:23]

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
File chrome/browser/ui/views/message_center/message_center_frame_view.cc
(right):

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/message_center_frame_view.cc:22: #if
defined(OS_WIN)
On 2013/07/16 18:24:01, dewittj wrote:
> I'd prefer if the condition were opposite, so that the default is to have a
> shadowborder, and the exception on Linux is to have a solid one.

Done.

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/message_center_frame_view.cc:58:
gfx::Path* window_mask) {}
On 2013/07/16 18:24:01, dewittj wrote:
> Make this match the style of the others, put the } on the next line.

Done. Missed that one. cl-format always puts empty braces this way
unfortunately.

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.cc
(right):

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:113:
size.Enlarge(border_insets_.width(), border_insets_.height());
On 2013/07/16 18:24:01, dewittj wrote:
> I am surprised you need to enlarge by the border size here.  GetPreferredSize
is
> for the view, not the widget, and the view size doesn't contain the border,
> right?  Feel free to correct me if I'm wrong, but I don't see references to
any
> border in other overrides of GetPreferredSize in WidgetDelegates.

Done. Moved to GetMessageCenterBounds.

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
File chrome/browser/ui/views/message_center/web_notification_tray.cc (right):

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray.cc:152:
show_settings, /* settings initally invisible */
On 2013/07/16 18:24:01, dewittj wrote:
> -> // style

Done.

https://codereview.chromium.org/18003003/diff/114001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray.cc:159: return
ShowMessageCenterInternal(false /* show_settings */);
On 2013/07/16 18:24:01, dewittj wrote:
> /*show_settings=*/ false please :)

Done.

https://codereview.chromium.org/18003003/diff/114001/ui/message_center/views/...
File ui/message_center/views/message_center_bubble.cc (right):

https://codereview.chromium.org/18003003/diff/114001/ui/message_center/views/...
ui/message_center/views/message_center_bubble.cc:97: buttons are always on
bottom for chrome os. */);
On 2013/07/16 18:24:01, dewittj wrote:
> s/This class/MessageCenterBubble/
> s/buttons/Buttons/
> s/chrome os/ChromeOS/g

Done.

https://codereview.chromium.org/18003003/diff/114001/ui/views/shadow_border.h
File ui/views/shadow_border.h (right):

https://codereview.chromium.org/18003003/diff/114001/ui/views/shadow_border.h...
ui/views/shadow_border.h:36: // Number of pixels to shift shaodw to bottom.
On 2013/07/16 18:24:01, dewittj wrote:
> s/shaodw/shadow/

Done. Sorry!

https://codereview.chromium.org/18003003/diff/114001/ui/views/shadow_border.h...
ui/views/shadow_border.h:39: // Number of pixels to shift shaodw to right.
On 2013/07/16 18:24:01, dewittj wrote:
> s/shaodw/shadow/

Done.

[dewittj, 2013-07-16 21:42:09]

https://codereview.chromium.org/18003003/diff/146001/chrome/browser/ui/views/...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.cc
(right):

https://codereview.chromium.org/18003003/diff/146001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:215:
gfx::Size size = GetPreferredSize();
I wonder if GetWindowBoundsForClientBounds(GetPreferredSize()) would do the same
thing without having to save border_insets_.

[dewittj, 2013-07-16 22:00:24]

lgtm once getPreferredSize has the right size.

https://codereview.chromium.org/18003003/diff/146001/chrome/browser/ui/views/...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.cc
(right):

https://codereview.chromium.org/18003003/diff/146001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:108:
return gfx::Size(preferred_width_, GetHeightForWidth(preferred_width_));
This size is wrong - the preferred size is 380px wide, right?  You are missing
the side margins.

nit: remove preferred_width_, just use kNotificationWidth + 2*
kMarginBetweenItems

https://codereview.chromium.org/18003003/diff/146001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:215:
gfx::Size size = GetPreferredSize();
On 2013/07/16 21:42:10, dewittj wrote:
> I wonder if GetWindowBoundsForClientBounds(GetPreferredSize()) would do the
same
> thing without having to save border_insets_.

Fine to leave this for later.

[sidharthms, 2013-07-16 22:44:13]

https://codereview.chromium.org/18003003/diff/146001/chrome/browser/ui/views/...
File chrome/browser/ui/views/message_center/message_center_widget_delegate.cc
(right):

https://codereview.chromium.org/18003003/diff/146001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/message_center_widget_delegate.cc:108:
return gfx::Size(preferred_width_, GetHeightForWidth(preferred_width_));
On 2013/07/16 22:00:24, dewittj wrote:
> This size is wrong - the preferred size is 380px wide, right?  You are missing
> the side margins.
> 
> nit: remove preferred_width_, just use kNotificationWidth + 2*
> kMarginBetweenItems

Done.

[sky, 2013-07-16 23:32:34]

LGTM

https://codereview.chromium.org/18003003/diff/165001/ui/views/shadow_border.h
File ui/views/shadow_border.h (right):

https://codereview.chromium.org/18003003/diff/165001/ui/views/shadow_border.h...
ui/views/shadow_border.h:31: int blur_;
const on all these.

[sidharthms, 2013-07-17 00:56:46]

https://codereview.chromium.org/18003003/diff/165001/ui/views/shadow_border.h
File ui/views/shadow_border.h (right):

https://codereview.chromium.org/18003003/diff/165001/ui/views/shadow_border.h...
ui/views/shadow_border.h:31: int blur_;
On 2013/07/16 23:32:34, sky wrote:
> const on all these.

Done.

[commit-bot: I haz the power, 2013-07-17 03:27:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sidharthms@chromium.org/18003003/180001

[commit-bot: I haz the power, 2013-07-17 09:23:54]

Change committed as 212007