Add TraySms for SMS messages.

Add TraySms for SMS messages.

BUG=124724
TEST=Test SMS messages running Chrome with --aura-notify
TBR=ben for ash strings

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=137671

[stevenjb, 2012-05-15 02:57:16]

SMS tray is ready for review when you have a chance.

http://codereview.chromium.org/10332152/diff/2001/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10332152/diff/2001/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:273: }
Moved from below.

[sadrul, 2012-05-15 15:10:50]

http://codereview.chromium.org/10332152/diff/26/ash/system/network/tray_sms.cc
File ash/system/network/tray_sms.cc (right):

http://codereview.chromium.org/10332152/diff/26/ash/system/network/tray_sms.c...
ash/system/network/tray_sms.cc:108: #else
#else  // defined(OS_CHROMEOS)

http://codereview.chromium.org/10332152/diff/26/ash/system/network/tray_sms.c...
ash/system/network/tray_sms.cc:149: class TraySms::SmsMessageView : public
views::View,
Just to clarify, this is one row in a detailed/notification view?

http://codereview.chromium.org/10332152/diff/26/ash/system/network/tray_sms.c...
ash/system/network/tray_sms.cc:233: int LayoutNotificationView() {
It looks like the detailed view gets a close button, but the notification button
doesn't. Is that intentional? Should TrayNotificationView be involved somehow?

http://codereview.chromium.org/10332152/diff/26/ash/system/network/tray_sms.h
File ash/system/network/tray_sms.h (right):

http://codereview.chromium.org/10332152/diff/26/ash/system/network/tray_sms.h...
ash/system/network/tray_sms.h:24: OVERRIDE;
OVERRIDE shouldn't be in its own line. You can move the parameter from the line
before (see tray_power)

http://codereview.chromium.org/10332152/diff/26/ash/system/network/tray_sms.h...
ash/system/network/tray_sms.h:33: class SmsDefaultView;
This is OK. But I wanted to point out that the rest of the tray-related code has
these in a tray:: namespace.

http://codereview.chromium.org/10332152/diff/26/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10332152/diff/26/ash/system/tray/system_tray.c...
ash/system/tray/system_tray.cc:276: if (item == detailed_item_) {
I am thinking this should be a DCHECK.

[stevenjb, 2012-05-15 16:55:19]

http://codereview.chromium.org/10332152/diff/26/ash/system/network/tray_sms.cc
File ash/system/network/tray_sms.cc (right):

http://codereview.chromium.org/10332152/diff/26/ash/system/network/tray_sms.c...
ash/system/network/tray_sms.cc:108: #else
On 2012/05/15 15:10:50, sadrul wrote:
> #else  // defined(OS_CHROMEOS)

Done.

http://codereview.chromium.org/10332152/diff/26/ash/system/network/tray_sms.c...
ash/system/network/tray_sms.cc:149: class TraySms::SmsMessageView : public
views::View,
On 2012/05/15 15:10:50, sadrul wrote:
> Just to clarify, this is one row in a detailed/notification view?
Correct.

http://codereview.chromium.org/10332152/diff/26/ash/system/network/tray_sms.c...
ash/system/network/tray_sms.cc:233: int LayoutNotificationView() {
On 2012/05/15 15:10:50, sadrul wrote:
> It looks like the detailed view gets a close button, but the notification
button
> doesn't. Is that intentional? Should TrayNotificationView be involved somehow?

SmsNotificationView inherits a close button from TrayNotificationView. 
The detailed entries also need a close button, but the layout is a little
different. 
I could do this as a base class and two subclasses if that would be more
readable?

http://codereview.chromium.org/10332152/diff/26/ash/system/network/tray_sms.h
File ash/system/network/tray_sms.h (right):

http://codereview.chromium.org/10332152/diff/26/ash/system/network/tray_sms.h...
ash/system/network/tray_sms.h:24: OVERRIDE;
On 2012/05/15 15:10:50, sadrul wrote:
> OVERRIDE shouldn't be in its own line. You can move the parameter from the
line
> before (see tray_power)

Done.

http://codereview.chromium.org/10332152/diff/26/ash/system/network/tray_sms.h...
ash/system/network/tray_sms.h:33: class SmsDefaultView;
On 2012/05/15 15:10:50, sadrul wrote:
> This is OK. But I wanted to point out that the rest of the tray-related code
has
> these in a tray:: namespace.

That works fine too, but in this case most of these also need to be friends, so
this is more compact.

http://codereview.chromium.org/10332152/diff/26/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10332152/diff/26/ash/system/tray/system_tray.c...
ash/system/tray/system_tray.cc:276: if (item == detailed_item_) {
On 2012/05/15 15:10:50, sadrul wrote:
> I am thinking this should be a DCHECK.

I would rather keep this safe in case a class calls this without checking that
its detailed view is open. It should really be an early exit though, I'll fix
that.

[stevenjb, 2012-05-15 16:55:32]

PTAL

[sadrul, 2012-05-16 18:19:42]

LGTM

[stevenjb, 2012-05-16 23:31:04]

PTAL. 
I merged TraySms with TrayDetailsView.

I had to make a change to TrayDetailsView::Layout to handle the case when
size.height() == height() but the content is smaller than the bubble. (It was
simplest to always force the footer to the bottom).

There is also a change to system_tray_bubble.cc to always resize the default
view to its contents, so that it is sized correctly when a new entry is added to
the default view while showing a detailed view (e.g. a SMS arrives while viewing
the network view) and the < button is clicked.

[sadrul, 2012-05-16 23:51:16]

On 2012/05/16 23:31:04, stevenjb (chromium) wrote:
> PTAL. 
> I merged TraySms with TrayDetailsView.
> 
> I had to make a change to TrayDetailsView::Layout to handle the case when
> size.height() == height() but the content is smaller than the bubble. (It was
> simplest to always force the footer to the bottom).
> 
> There is also a change to system_tray_bubble.cc to always resize the default
> view to its contents, so that it is sized correctly when a new entry is added
to
> the default view while showing a detailed view (e.g. a SMS arrives while
viewing
> the network view) and the < button is clicked.

Both of these changes look good. Thanks! LGTM

[commit-bot: I haz the power, 2012-05-17 05:43:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/10332152/2003

[commit-bot: I haz the power, 2012-05-17 05:43:51]

Can't apply patch for file ui/resources/ui_resources.grd.
While running patch -p1 --forward --force;
patching file ui/resources/ui_resources.grd
Hunk #1 FAILED at 289.
1 out of 1 hunk FAILED -- saving rejects to file
ui/resources/ui_resources.grd.rej