Updates the display message in the uber tray.

Updates the display message in the uber tray.

This CL contains the following fixes:
- shows the notification for display rotation / ui_scale
- adds (width x height) annotation to the display if something
  has been edited
- shows the internal display's status if something for the internal
  display has been edited and no external display is connected
- shows the tooltip text to provide the status of both internal and
  extrenal displays at the same time

This CL does not contain:
- the feature to prevent notification when the settings change happens
  from chrome://settings/display

I think we can achieve this by suppressing the notification when
the current active tab is chrome://settings/display, but anyways
it'll be done in another CL. That is marked as a TODO.

BUG=250650, 246271
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207998

[oshima, 2013-06-19 18:20:31]

can you add unit test for messages?

https://codereview.chromium.org/17445002/diff/1/ash/system/chromeos/tray_disp...
File ash/system/chromeos/tray_display.cc (right):

https://codereview.chromium.org/17445002/diff/1/ash/system/chromeos/tray_disp...
ash/system/chromeos/tray_display.cc:73: name += UTF8ToUTF16(" (") +
GetDisplaySize(external_id) + UTF8ToUTF16(")");
Any RTL issue here? (I think it's ok, but just double checking)

https://codereview.chromium.org/17445002/diff/1/ash/system/chromeos/tray_disp...
ash/system/chromeos/tray_display.cc:183: *tooltip = tray_message +
base::string16(1, '\n') + internal_message;
RTL?

[Jun Mukai, 2013-06-19 23:42:35]

will add test. please wait a bit.

https://codereview.chromium.org/17445002/diff/1/ash/system/chromeos/tray_disp...
File ash/system/chromeos/tray_display.cc (right):

https://codereview.chromium.org/17445002/diff/1/ash/system/chromeos/tray_disp...
ash/system/chromeos/tray_display.cc:73: name += UTF8ToUTF16(" (") +
GetDisplaySize(external_id) + UTF8ToUTF16(")");
On 2013/06/19 18:20:31, oshima wrote:
> Any RTL issue here? (I think it's ok, but just double checking)

looks okay to me

https://codereview.chromium.org/17445002/diff/1/ash/system/chromeos/tray_disp...
ash/system/chromeos/tray_display.cc:183: *tooltip = tray_message +
base::string16(1, '\n') + internal_message;
On 2013/06/19 18:20:31, oshima wrote:
> RTL?

linebreak won't suffer from RTL issue.

[Jun Mukai, 2013-06-20 03:24:42]

test is added.  PTAL

[Jun Mukai, 2013-06-20 21:17:01]

ping?

[oshima, 2013-06-20 21:28:57]

https://codereview.chromium.org/17445002/diff/14001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display.cc (right):

https://codereview.chromium.org/17445002/diff/14001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:326:
static_cast<views::View*>(default_)->GetTooltipText(gfx::Point(), &tooltip);
isn't DefaultView a view?
also it's probably better to expose default_ as a view instead
of adding menu test methods.

https://codereview.chromium.org/17445002/diff/14001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display_unittest.cc (right):

https://codereview.chromium.org/17445002/diff/14001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display_unittest.cc:24: return line1 +
base::string16(1, '\n') + line2;
nit: ASCIIToUTF16("\n") may be shorter and easier to read:
it's up to you though.

https://codereview.chromium.org/17445002/diff/14001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display_unittest.cc:71: TrayDisplay* tray_display_;
nit: comment ownership.

[Jun Mukai, 2013-06-20 22:03:07]

https://codereview.chromium.org/17445002/diff/14001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display.cc (right):

https://codereview.chromium.org/17445002/diff/14001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:326:
static_cast<views::View*>(default_)->GetTooltipText(gfx::Point(), &tooltip);
On 2013/06/20 21:28:57, oshima wrote:
> isn't DefaultView a view?
> also it's probably better to expose default_ as a view instead
> of adding menu test methods.
> 

done

https://codereview.chromium.org/17445002/diff/14001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display_unittest.cc (right):

https://codereview.chromium.org/17445002/diff/14001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display_unittest.cc:24: return line1 +
base::string16(1, '\n') + line2;
On 2013/06/20 21:28:57, oshima wrote:
> nit: ASCIIToUTF16("\n") may be shorter and easier to read:
> it's up to you though.

Done.

https://codereview.chromium.org/17445002/diff/14001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display_unittest.cc:71: TrayDisplay* tray_display_;
On 2013/06/20 21:28:57, oshima wrote:
> nit: comment ownership.

Done.

[Jun Mukai, 2013-06-20 23:09:21]

fixed the code to disable the display notifications for all ash tests (except
for tray_display_unittests).  PTAL

[oshima, 2013-06-20 23:16:11]

lgtm with nits

https://codereview.chromium.org/17445002/diff/26001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display.cc (right):

https://codereview.chromium.org/17445002/diff/26001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:27: bool g_display_notifications_enabled =
true;
optional: since this isn't really global (this has file local scope),
so g_ isn't required.

https://codereview.chromium.org/17445002/diff/26001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display.h (right):

https://codereview.chromium.org/17445002/diff/26001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.h:75: base::string16
GetDisplayMessageForNotification();
const?

https://codereview.chromium.org/17445002/diff/26001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.h:94: string16 current_message() { return
current_message_; }
nit: const& const

[Jun Mukai, 2013-06-20 23:34:33]

https://codereview.chromium.org/17445002/diff/26001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display.cc (right):

https://codereview.chromium.org/17445002/diff/26001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:27: bool g_display_notifications_enabled =
true;
On 2013/06/20 23:16:11, oshima wrote:
> optional: since this isn't really global (this has file local scope),
> so g_ isn't required.

Done.

https://codereview.chromium.org/17445002/diff/26001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display.h (right):

https://codereview.chromium.org/17445002/diff/26001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.h:75: base::string16
GetDisplayMessageForNotification();
On 2013/06/20 23:16:11, oshima wrote:
> const?

not const. It updates |display_info| and compare with the old one.

https://codereview.chromium.org/17445002/diff/26001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.h:94: string16 current_message() { return
current_message_; }
On 2013/06/20 23:16:11, oshima wrote:
> nit: const& const

Done.

[Jun Mukai, 2013-06-20 23:35:06]

stevenjb, jamescook, please review this

[James Cook, 2013-06-21 00:38:26]

LGTM with a suggestion for strings.

Are you sure you can get the change to suppress notifications from
chrome://settings/display into M29 as well?  If not, would it be better to land
both changes in the same milestone?  I think it would be strange to show these
notifications from settings as well as from keyboard changes.

https://codereview.chromium.org/17445002/diff/26002/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/17445002/diff/26002/ash/ash_strings.grd#newco...
ash/ash_strings.grd:325: <ph name="DISPLAY_NAME">$1</ph> is resized to <ph
name="RESOLUTION">$2</ph>
Should this be "has been resized to"?

https://codereview.chromium.org/17445002/diff/26002/ash/ash_strings.grd#newco...
ash/ash_strings.grd:328: <ph name="DISPLAY_NAME">$1</ph> is rotated
"has been rotated"?

[Jun Mukai, 2013-06-21 00:46:48]

https://codereview.chromium.org/17445002/diff/26002/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/17445002/diff/26002/ash/ash_strings.grd#newco...
ash/ash_strings.grd:325: <ph name="DISPLAY_NAME">$1</ph> is resized to <ph
name="RESOLUTION">$2</ph>
On 2013/06/21 00:38:27, James Cook (Chromium) wrote:
> Should this be "has been resized to"?

Done.

https://codereview.chromium.org/17445002/diff/26002/ash/ash_strings.grd#newco...
ash/ash_strings.grd:328: <ph name="DISPLAY_NAME">$1</ph> is rotated
On 2013/06/21 00:38:27, James Cook (Chromium) wrote:
> "has been rotated"?

Done.

[Jun Mukai, 2013-06-21 00:49:47]

On 2013/06/21 00:38:26, James Cook (Chromium) wrote:
> LGTM with a suggestion for strings.
> 
> Are you sure you can get the change to suppress notifications from
> chrome://settings/display into M29 as well?  If not, would it be better to
land
> both changes in the same milestone?  I think it would be strange to show these
> notifications from settings as well as from keyboard changes.
> 

IIRC, we agreed that showing the notification would be more important than
controlling
it precisely.  It's better to be done in the same milestone, but I'm not sure.

Right now, I think to add a suppression if the currently focused tab is
chrome://settings/display.
That approach would be good enough, and hopefully simple and small. I'll write
another CL for that.

[James Cook, 2013-06-21 00:52:48]

> IIRC, we agreed that showing the notification would be more important than
> controlling
> it precisely.  It's better to be done in the same milestone, but I'm not sure.
> 
> Right now, I think to add a suppression if the currently focused tab is
> chrome://settings/display.
> That approach would be good enough, and hopefully simple and small. I'll write
> another CL for that.

OK, sounds good to me.  Now that I think about it more, I agree it's more
important to show the notification than to control it, since the user hitting
Ctrl-Shift-+ accidentally can be quite confusing.  So I'm OK with landing this
patch now, even if the chrome://settings/display patch might not be in M29.

[stevenjb, 2013-06-21 01:08:34]

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display.cc (right):

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:27: bool display_notifications_enabled =
true;
This should really be 'display_notifications_disabled' with the default set to
false. The test setter should also be renamed appropriately.

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:241: if (display_info_.size() ==
old_info.size()) {
I would make this an early exit with a comment; the logic here is kind of
confusing and less nesting would help a bit.

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:247: break;
I'm not sure I follow this logic. Do we really want to break the first time an
entry in display_info_ is not in old_info? If so, can you add a comment
explaining the logic?

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:303: void
TrayDisplay::SetDisplayNotificationsEnabledForTest(bool enabled) {
I would make this 'DisableDisplayNotificationsForTest()' unless we actually need
to toggle back and forth.

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display.h (right):

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.h:61: };
I kind of hate exposing this in the header just for the sake of a test. I
wouldn't necessarily change it here, but in the future I think it would be
better to add ForTest accessors to the primary class instead.

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.h:88: // Call this to true when the test case
wants to see the display notification.
s/to true/with |enabled| = true/
(The 'Default' clause can be omitted to, since that is implied)

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.h:92: // Test utilities.
nit: s/utilities/accessors/

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.h:99: std::map<int64, DisplayInfo>
display_info_;
We should typedef this, e.g. DisplayInfoMap. It will make the c++ code easier to
read.

[Jun Mukai, 2013-06-21 04:48:49]

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display.cc (right):

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:27: bool display_notifications_enabled =
true;
On 2013/06/21 01:08:35, stevenjb (chromium) wrote:
> This should really be 'display_notifications_disabled' with the default set to
> false. The test setter should also be renamed appropriately.

Done.

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:241: if (display_info_.size() ==
old_info.size()) {
On 2013/06/21 01:08:35, stevenjb (chromium) wrote:
> I would make this an early exit with a comment; the logic here is kind of
> confusing and less nesting would help a bit.

Done.

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:247: break;
On 2013/06/21 01:08:35, stevenjb (chromium) wrote:
> I'm not sure I follow this logic. Do we really want to break the first time an
> entry in display_info_ is not in old_info? If so, can you add a comment
> explaining the logic?

Done.

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:303: void
TrayDisplay::SetDisplayNotificationsEnabledForTest(bool enabled) {
On 2013/06/21 01:08:35, stevenjb (chromium) wrote:
> I would make this 'DisableDisplayNotificationsForTest()' unless we actually
need
> to toggle back and forth.
> 

it actually needs to be toggled.  AshTestBase does the disable by default, but
only TrayDisplayTest wants to re-enable it.

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display.h (right):

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.h:61: };
On 2013/06/21 01:08:35, stevenjb (chromium) wrote:
> I kind of hate exposing this in the header just for the sake of a test. I
> wouldn't necessarily change it here, but in the future I think it would be
> better to add ForTest accessors to the primary class instead.

Done.

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.h:88: // Call this to true when the test case
wants to see the display notification.
On 2013/06/21 01:08:35, stevenjb (chromium) wrote:
> s/to true/with |enabled| = true/
> (The 'Default' clause can be omitted to, since that is implied)

Done.

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.h:92: // Test utilities.
On 2013/06/21 01:08:35, stevenjb (chromium) wrote:
> nit: s/utilities/accessors/

Done.

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.h:99: std::map<int64, DisplayInfo>
display_info_;
On 2013/06/21 01:08:35, stevenjb (chromium) wrote:
> We should typedef this, e.g. DisplayInfoMap. It will make the c++ code easier
to
> read.

Done.

[stevenjb, 2013-06-21 20:17:51]

lgtm

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
File ash/system/chromeos/tray_display.cc (right):

https://codereview.chromium.org/17445002/diff/31001/ash/system/chromeos/tray_...
ash/system/chromeos/tray_display.cc:247: break;
On 2013/06/21 04:48:50, Jun Mukai wrote:
> On 2013/06/21 01:08:35, stevenjb (chromium) wrote:
> > I'm not sure I follow this logic. Do we really want to break the first time
an
> > entry in display_info_ is not in old_info? If so, can you add a comment
> > explaining the logic?
> 
> Done.
Much clearer now, thanks.

[commit-bot: I haz the power, 2013-06-21 21:02:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/17445002/37001

[commit-bot: I haz the power, 2013-06-22 02:42:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/17445002/37001

[commit-bot: I haz the power, 2013-06-22 03:06:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/17445002/37001

[commit-bot: I haz the power, 2013-06-22 03:11:48]

Change committed as 207998