Remove the system menu display settings row

Remove the system menu display settings row

Remove the display row in the system menu which show up when number of displays
changes, screen orientation changes or display resolution changes. However,
Notification will be keep sending to the notification center as it is now.

Since display tray is removed, the dependencies to SystemTrayItem from TrayDisplay
is also removed. So the class name is updated to screen_layout_observer to reflect
that it keeps observing screen layout changes and sending notifications to users.

TEST=ScreenLayoutObserverTest in ash_unittests
BUG=630637
Committed: https://crrev.com/a68ade5167de02938634fa2387e53e20b9353c18
Cr-Commit-Position: refs/heads/master@{#415988}

[yiyix, 2016-08-04 21:04:55]

Description was changed from

==========
nits


merge


working version

second fix

test

add all

first try

finish

finish

remove comments

fix names

fix names

fix names

BUG=
==========

to

==========
Remove the system menu display settings row

Remove the display row in the system menu which show up when number of displays 
changes, screen orientation changes or display resolution changes. However,  
Notification will be keep sending to the notification center as it is now. 

Since display tray is removed, the dependencies to SystemTrayItem from
TrayDisplay 
is also removed. So the class name is updated to screen_layout_observer to
reflect 
that it keeps observing screen layout changes and sending notifications to
users.

BUG=630637
==========

[yiyix, 2016-08-04 21:04:55]

yiyix@chromium.org changed reviewers:
+ jamescook@chromium.org

[yiyix, 2016-08-04 21:06:22]

On 2016/08/04 21:04:55, yiyix wrote:
> mailto:yiyix@chromium.org changed reviewers:
> + mailto:jamescook@chromium.org

@jamescook, could you please review this change? Thanks.

[James Cook, 2016-08-05 17:00:25]

https://codereview.chromium.org/2217713002/diff/1/ash/common/system/tray/syst...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2217713002/diff/1/ash/common/system/tray/syst...
ash/common/system/tray/system_tray.cc:198: // TODO(jamescook): Remove this when
mus has support for display management
Delete this comment.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
File ash/system/chromeos/screen_layout_observer.cc (left):

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.cc:82:
IDS_ASH_STATUS_TRAY_DISPLAY_ANNOTATION, size_text,
Are these strings still used? If not, remove them from
ash_chromeos_strings.grdp.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.cc:84:
IDS_ASH_STATUS_TRAY_DISPLAY_ANNOTATION_OVERSCAN));
ditto

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.cc:89: return
l10n_util::GetStringFUTF16(IDS_ASH_STATUS_TRAY_DISPLAY_SINGLE_DISPLAY,
ditto

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
File ash/system/chromeos/screen_layout_observer.cc (right):

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.cc:14: #include
"ash/common/system/tray/actionable_view.h"
not needed?

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.cc:36: #include
"ui/views/controls/label.h"
not needed?

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.cc:107: base::string16
GetExternalDisplayName() {
Is the old function comment still relevant? If so, keep it. Also, can this go in
the anonymous namespace above?

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.cc:146: base::string16
GetDisplayMessage(base::string16* additional_message_out) {
ditto re: anonymous namespace

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
File ash/system/chromeos/screen_layout_observer.h (right):

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.h:14: #include
"ash/common/system/tray/system_tray_item.h"
not needed?

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.h:18: #include "ui/views/view.h"
not needed?

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.h:24: class ASH_EXPORT
ScreenLayoutObserver : public WmDisplayObserver {
Add class comment explaining what this does and why.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.h:26: explicit
ScreenLayoutObserver();
no explicit

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.h:59:
DISALLOW_COPY_AND_ASSIGN(ScreenLayoutObserver);
I like that you renamed to class to better represent what it does.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
File ash/system/chromeos/screen_layout_observer_unittest.cc (left):

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:436: }
I'm concerned that you're removing so much test coverage. While these tests
focus on the tray, they seem to test useful things that are not covered by the
notification tests. Can some of them be modified to check the notifications,
rather than the tray items?

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:584:
TEST_F(TrayDisplayTest, RotationOnInternalDisplay) {
I still see code in CreateOrUpdateNotification() that checks for rotation-based
changes. Can this test be modified to verify that no notification is created for
rotation changes? Or do you think the test in //ash/content/display has enough
coverage?

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
File ash/system/chromeos/screen_layout_observer_unittest.cc (right):

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:47: class
ScreenLayoutObserverTest : public ash::test::AshTestBase {
nit: ash:: not needed

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:65: // Weak reference,
owned by Shell.
Is this still true?

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:66: ScreenLayoutObserver*
screen_layout_observer_;
This does not appear to be read. Perhaps you meant it to be a unique_ptr<>?

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:72: :
screen_layout_observer_() {}
not needed

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:76: void
ScreenLayoutObserverTest::SetUp() {
Eliminate this method, because it does nothing.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:80: ScreenLayoutObserver*
ScreenLayoutObserverTest::UpdateTrayDisplay() {
This method name is odd -- I thought TrayDisplay was going away.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:81:
screen_layout_observer_ = new ScreenLayoutObserver();
I think this leaks memory. ScreenLayoutObserver doesn't appear to be owned by
anyone.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:222: // Test if
notification shows when display is switched to extend mode.
Thanks for documenting what the test does!

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:234:
UpdateTrayDisplay()->OnDisplayConfigurationChanged();
This reads strangely to me. I think creating the object should be separate from
changing it.

https://codereview.chromium.org/2217713002/diff/1/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/2217713002/diff/1/ash/test/ash_test_base.cc#n...
ash/test/ash_test_base.cc:41: #include
"ash/system/chromeos/screen_layout_observer.h"
Is this still used?

[tdanderson, 2016-08-08 21:15:57]

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

[tdanderson, 2016-08-08 21:15:57]

https://chromiumcodereview.appspot.com/2217713002/diff/1/ash/system/chromeos/...
File ash/system/chromeos/tray_display.cc (left):

https://chromiumcodereview.appspot.com/2217713002/diff/1/ash/system/chromeos/...
ash/system/chromeos/tray_display.cc:165:
bundle.GetImageNamed(IDR_AURA_UBER_TRAY_DISPLAY).ToImageSkia());
Don't forget to also remove this ID from ash_resources.grd and to delete the
png(s) it points to.

[yiyix, 2016-08-13 05:28:52]

I made some updates to the unit test class. 

@jamescook, tdanderson, could you please look at this change again? Thanks.

https://codereview.chromium.org/2217713002/diff/1/ash/common/system/tray/syst...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2217713002/diff/1/ash/common/system/tray/syst...
ash/common/system/tray/system_tray.cc:198: // TODO(jamescook): Remove this when
mus has support for display management
On 2016/08/05 17:00:24, James Cook wrote:
> Delete this comment.

Done.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
File ash/system/chromeos/screen_layout_observer.cc (left):

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.cc:82:
IDS_ASH_STATUS_TRAY_DISPLAY_ANNOTATION, size_text,
On 2016/08/05 17:00:24, James Cook wrote:
> Are these strings still used? If not, remove them from
> ash_chromeos_strings.grdp.

Done.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.cc:84:
IDS_ASH_STATUS_TRAY_DISPLAY_ANNOTATION_OVERSCAN));
On 2016/08/05 17:00:24, James Cook wrote:
> ditto

This string is used for notification. When user tries to connect an external
monitor to the device while an internal monitor is connected with overscan on
screen, the external monitor will have the overscan on it with this
notification.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.cc:89: return
l10n_util::GetStringFUTF16(IDS_ASH_STATUS_TRAY_DISPLAY_SINGLE_DISPLAY,
On 2016/08/05 17:00:24, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
File ash/system/chromeos/screen_layout_observer.cc (right):

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.cc:14: #include
"ash/common/system/tray/actionable_view.h"
On 2016/08/05 17:00:24, James Cook wrote:
> not needed?

Done.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.cc:36: #include
"ui/views/controls/label.h"
On 2016/08/05 17:00:24, James Cook wrote:
> not needed?

I will go through the list to remove all unnecessary includes. Sorry I forget to
update the list.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.cc:107: base::string16
GetExternalDisplayName() {
On 2016/08/05 17:00:24, James Cook wrote:
> Is the old function comment still relevant? If so, keep it. Also, can this go
in
> the anonymous namespace above?

Done.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.cc:146: base::string16
GetDisplayMessage(base::string16* additional_message_out) {
On 2016/08/05 17:00:24, James Cook wrote:
> ditto re: anonymous namespace

Done.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
File ash/system/chromeos/screen_layout_observer.h (right):

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.h:14: #include
"ash/common/system/tray/system_tray_item.h"
On 2016/08/05 17:00:24, James Cook wrote:
> not needed?

Done.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.h:18: #include "ui/views/view.h"
On 2016/08/05 17:00:24, James Cook wrote:
> not needed?

Done.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.h:24: class ASH_EXPORT
ScreenLayoutObserver : public WmDisplayObserver {
On 2016/08/05 17:00:24, James Cook wrote:
> Add class comment explaining what this does and why.

Done.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.h:26: explicit
ScreenLayoutObserver();
On 2016/08/05 17:00:24, James Cook wrote:
> no explicit

Done.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer.h:59:
DISALLOW_COPY_AND_ASSIGN(ScreenLayoutObserver);
On 2016/08/05 17:00:24, James Cook wrote:
> I like that you renamed to class to better represent what it does.
Thanks!

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
File ash/system/chromeos/screen_layout_observer_unittest.cc (left):

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:436: }
On 2016/08/05 17:00:25, James Cook wrote:
> I'm concerned that you're removing so much test coverage. While these tests
> focus on the tray, they seem to test useful things that are not covered by the
> notification tests. Can some of them be modified to check the notifications,
> rather than the tray items?

I believe most of them are covered. Tests are missing:
- test notification on overscan mode. -- added
- UpdateDuringDisplayConfigurationChange -- nothing special on this one,
"Extended screen" is shown regardless the screen is @1.5 or @1.0

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:584:
TEST_F(TrayDisplayTest, RotationOnInternalDisplay) {
On 2016/08/05 17:00:25, James Cook wrote:
> I still see code in CreateOrUpdateNotification() that checks for
rotation-based
> changes. Can this test be modified to verify that no notification is created
for
> rotation changes? Or do you think the test in //ash/content/display has enough
> coverage?

Notification center uses ignore_display_configuration_updates to check if the
rotation is caused by accelerometer and notification should be blocked. By
calling SetDisplayRotation,  the code does not check the source of rotation and
the variable ignore_display_configuration_updates is not being set properly, so
the notification will still be shown even the source of rotation is caused by
accelerometer.

I found that ScreenOrientationControllerTest.BlockRotationNotifications in
screen_orientation_controller_chromeos_unittest.cc test this scenario.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
File ash/system/chromeos/screen_layout_observer_unittest.cc (right):

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:47: class
ScreenLayoutObserverTest : public ash::test::AshTestBase {
On 2016/08/05 17:00:25, James Cook wrote:
> nit: ash:: not needed

Done.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:65: // Weak reference,
owned by Shell.
On 2016/08/05 17:00:25, James Cook wrote:
> Is this still true?

it used to owned by |tray_|, it is now owned by shell.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:66: ScreenLayoutObserver*
screen_layout_observer_;
On 2016/08/05 17:00:25, James Cook wrote:
> This does not appear to be read. Perhaps you meant it to be a unique_ptr<>?

I removed this method, the ScreenLayoutObserver is created during the shell
initialization stage.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:72: :
screen_layout_observer_() {}
On 2016/08/05 17:00:25, James Cook wrote:
> not needed

Done.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:76: void
ScreenLayoutObserverTest::SetUp() {
On 2016/08/05 17:00:25, James Cook wrote:
> Eliminate this method, because it does nothing.

Done.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:80: ScreenLayoutObserver*
ScreenLayoutObserverTest::UpdateTrayDisplay() {
On 2016/08/05 17:00:25, James Cook wrote:
> This method name is odd -- I thought TrayDisplay was going away.

I forget to update the name. Sorry.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:81:
screen_layout_observer_ = new ScreenLayoutObserver();
On 2016/08/05 17:00:25, James Cook wrote:
> I think this leaks memory. ScreenLayoutObserver doesn't appear to be owned by
> anyone.

It is updated.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/screen_...
ash/system/chromeos/screen_layout_observer_unittest.cc:234:
UpdateTrayDisplay()->OnDisplayConfigurationChanged();
On 2016/08/05 17:00:25, James Cook wrote:
> This reads strangely to me. I think creating the object should be separate
from
> changing it.

It is updated.

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/tray_di...
File ash/system/chromeos/tray_display.cc (left):

https://codereview.chromium.org/2217713002/diff/1/ash/system/chromeos/tray_di...
ash/system/chromeos/tray_display.cc:165:
bundle.GetImageNamed(IDR_AURA_UBER_TRAY_DISPLAY).ToImageSkia());
On 2016/08/08 21:15:57, tdanderson wrote:
> Don't forget to also remove this ID from ash_resources.grd and to delete the
> png(s) it points to.

Thank you, I totally forget to remove resources.

https://codereview.chromium.org/2217713002/diff/1/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/2217713002/diff/1/ash/test/ash_test_base.cc#n...
ash/test/ash_test_base.cc:41: #include
"ash/system/chromeos/screen_layout_observer.h"
On 2016/08/05 17:00:25, James Cook wrote:
> Is this still used?

You are right.

[James Cook, 2016-08-15 16:33:45]

I will get to this when I can, but the cros build is having trouble and I need
to investigate that first.

Maybe tdanderson can make a pass first?

[tdanderson, 2016-08-15 18:55:16]

I think this lg, assuming that you have signoff from Tom/UX regarding any
special cases you have found (is crbug.com/630637 comment #5 the only case that
needs feedback)?

https://codereview.chromium.org/2217713002/diff/20001/ash/system/chromeos/scr...
File ash/system/chromeos/screen_layout_observer.cc (right):

https://codereview.chromium.org/2217713002/diff/20001/ash/system/chromeos/scr...
ash/system/chromeos/screen_layout_observer.cc:115: if (external_id ==
display::Display::kInvalidDisplayID) {
nit: no {}

https://codereview.chromium.org/2217713002/diff/20001/ash/system/chromeos/scr...
File ash/system/chromeos/screen_layout_observer_unittest.cc (right):

https://codereview.chromium.org/2217713002/diff/20001/ash/system/chromeos/scr...
ash/system/chromeos/screen_layout_observer_unittest.cc:211:
TEST_F(ScreenLayoutObserverTest, DisplayConfigurationChangedTwice) {
While you're here, can you please add descriptions to the other tests in this
fixture (i.e., above lines 211 and 233)?

https://codereview.chromium.org/2217713002/diff/20001/ash/system/chromeos/scr...
ash/system/chromeos/screen_layout_observer_unittest.cc:248: // Verify that if no
notification is shown when overscan of a screen is changed.
nit: remove "if"

https://codereview.chromium.org/2217713002/diff/20001/ash/system/chromeos/scr...
ash/system/chromeos/screen_layout_observer_unittest.cc:256: // /o creates the
default overscan.
nit: typo

https://codereview.chromium.org/2217713002/diff/20001/ash/system/chromeos/scr...
ash/system/chromeos/screen_layout_observer_unittest.cc:261: // reset the
overscan.
nit: reset->Reset

[James Cook, 2016-08-15 21:42:15]

Also, can you add a TEST= line to the CL description? I like to reference the
test suite that covers the code that changed, for example:

TEST=ash_unittests

https://codereview.chromium.org/2217713002/diff/20001/ash/test/test_system_tr...
File ash/test/test_system_tray_delegate.cc (right):

https://codereview.chromium.org/2217713002/diff/20001/ash/test/test_system_tr...
ash/test/test_system_tray_delegate.cc:18: #include
"ash/system/chromeos/screen_layout_observer.h"
unused?

[yiyix, 2016-08-16 15:45:37]

Description was changed from

==========
Remove the system menu display settings row

Remove the display row in the system menu which show up when number of displays 
changes, screen orientation changes or display resolution changes. However,  
Notification will be keep sending to the notification center as it is now. 

Since display tray is removed, the dependencies to SystemTrayItem from
TrayDisplay 
is also removed. So the class name is updated to screen_layout_observer to
reflect 
that it keeps observing screen layout changes and sending notifications to
users.

BUG=630637
==========

to

==========
Remove the system menu display settings row

Remove the display row in the system menu which show up when number of displays 
changes, screen orientation changes or display resolution changes. However,  
Notification will be keep sending to the notification center as it is now. 

Since display tray is removed, the dependencies to SystemTrayItem from
TrayDisplay 
is also removed. So the class name is updated to screen_layout_observer to
reflect 
that it keeps observing screen layout changes and sending notifications to
users.

TEST=ScreenLayoutObserverTest in ash_unittests
BUG=630637
==========

[yiyix, 2016-08-16 15:58:37]

@tdanderson and I have discussed in person that it is okay to not show a
notification when overscan changed. 

@tdanderson and @jamescook, could you please take another look at this code
change?

https://codereview.chromium.org/2217713002/diff/20001/ash/system/chromeos/scr...
File ash/system/chromeos/screen_layout_observer.cc (right):

https://codereview.chromium.org/2217713002/diff/20001/ash/system/chromeos/scr...
ash/system/chromeos/screen_layout_observer.cc:115: if (external_id ==
display::Display::kInvalidDisplayID) {
On 2016/08/15 18:55:16, tdanderson wrote:
> nit: no {}

Done.

https://codereview.chromium.org/2217713002/diff/20001/ash/system/chromeos/scr...
File ash/system/chromeos/screen_layout_observer_unittest.cc (right):

https://codereview.chromium.org/2217713002/diff/20001/ash/system/chromeos/scr...
ash/system/chromeos/screen_layout_observer_unittest.cc:211:
TEST_F(ScreenLayoutObserverTest, DisplayConfigurationChangedTwice) {
On 2016/08/15 18:55:16, tdanderson wrote:
> While you're here, can you please add descriptions to the other tests in this
> fixture (i.e., above lines 211 and 233)?

Done.

https://codereview.chromium.org/2217713002/diff/20001/ash/system/chromeos/scr...
ash/system/chromeos/screen_layout_observer_unittest.cc:248: // Verify that if no
notification is shown when overscan of a screen is changed.
On 2016/08/15 18:55:16, tdanderson wrote:
> nit: remove "if"

Done.

https://codereview.chromium.org/2217713002/diff/20001/ash/system/chromeos/scr...
ash/system/chromeos/screen_layout_observer_unittest.cc:256: // /o creates the
default overscan.
On 2016/08/15 18:55:16, tdanderson wrote:
> nit: typo

Done.

https://codereview.chromium.org/2217713002/diff/20001/ash/system/chromeos/scr...
ash/system/chromeos/screen_layout_observer_unittest.cc:261: // reset the
overscan.
On 2016/08/15 18:55:16, tdanderson wrote:
> nit: reset->Reset

Done.

https://codereview.chromium.org/2217713002/diff/20001/ash/test/test_system_tr...
File ash/test/test_system_tray_delegate.cc (right):

https://codereview.chromium.org/2217713002/diff/20001/ash/test/test_system_tr...
ash/test/test_system_tray_delegate.cc:18: #include
"ash/system/chromeos/screen_layout_observer.h"
On 2016/08/15 21:42:15, James Cook (slow reviews) wrote:
> unused?

Done.

[tdanderson, 2016-08-16 16:05:28]

lgtm

[James Cook, 2016-08-16 16:20:19]

LGTM. Nice patch.

[yiyix, 2016-08-16 17:51:13]

yiyix@chromium.org changed reviewers:
+ oshima@chromium.org

[yiyix, 2016-08-16 17:51:14]

@oshima, could you please review the changes in chrome/? Thank you

[oshima, 2016-08-16 18:29:54]

I didn't know that we're removing it, but I agree that notificaiton would be
good enough. 

IIRC, the notification won't be shown when the setting menu is opened. Can you
confirm it? then lgtm

[yiyix, 2016-08-18 20:44:23]

On 2016/08/16 18:29:54, oshima wrote:
> I didn't know that we're removing it, but I agree that notificaiton would be
> good enough. 
> 
> IIRC, the notification won't be shown when the setting menu is opened. Can you
> confirm it? then lgtm

What do you mean? The notification is shown regardless if the setting menu is
opened or not. I can confirm if this is the expected behavior.

[oshima, 2016-08-24 13:05:22]

On 2016/08/18 20:44:23, yiyix wrote:
> On 2016/08/16 18:29:54, oshima wrote:
> > I didn't know that we're removing it, but I agree that notificaiton would be
> > good enough. 
> > 
> > IIRC, the notification won't be shown when the setting menu is opened. Can
you
> > confirm it? then lgtm
> 
> What do you mean? The notification is shown regardless if the setting menu is
> opened or not. I can confirm if this is the expected behavior.

Sorry I meant setting page.

[oshima, 2016-08-24 13:05:24]

On 2016/08/18 20:44:23, yiyix wrote:
> On 2016/08/16 18:29:54, oshima wrote:
> > I didn't know that we're removing it, but I agree that notificaiton would be
> > good enough. 
> > 
> > IIRC, the notification won't be shown when the setting menu is opened. Can
you
> > confirm it? then lgtm
> 
> What do you mean? The notification is shown regardless if the setting menu is
> opened or not. I can confirm if this is the expected behavior.

Sorry I meant setting page.

[yiyix, 2016-08-31 19:23:39]

The CQ bit was checked by yiyix@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 19:24:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 19:29:12]

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

[commit-bot: I haz the power, 2016-08-31 19:29:13]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[yiyix, 2016-09-01 15:25:09]

The CQ bit was checked by yiyix@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 15:25:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yiyix, 2016-09-01 15:40:18]

Patchset #4 (id:60001) has been deleted

[yiyix, 2016-09-01 15:40:45]

The CQ bit was checked by yiyix@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 15:41:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 16:32:24]

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

[commit-bot: I haz the power, 2016-09-01 16:32:25]

Dry run: This issue passed the CQ dry run.

[yiyix, 2016-09-01 17:32:44]

The CQ bit was checked by yiyix@chromium.org

[yiyix, 2016-09-01 17:32:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, jamescook@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2217713002/#ps80001
(title: "merge")

[commit-bot: I haz the power, 2016-09-01 17:33:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 17:38:30]

Description was changed from

==========
Remove the system menu display settings row

Remove the display row in the system menu which show up when number of displays 
changes, screen orientation changes or display resolution changes. However,  
Notification will be keep sending to the notification center as it is now. 

Since display tray is removed, the dependencies to SystemTrayItem from
TrayDisplay 
is also removed. So the class name is updated to screen_layout_observer to
reflect 
that it keeps observing screen layout changes and sending notifications to
users.

TEST=ScreenLayoutObserverTest in ash_unittests
BUG=630637
==========

to

==========
Remove the system menu display settings row

Remove the display row in the system menu which show up when number of displays 
changes, screen orientation changes or display resolution changes. However,  
Notification will be keep sending to the notification center as it is now. 

Since display tray is removed, the dependencies to SystemTrayItem from
TrayDisplay 
is also removed. So the class name is updated to screen_layout_observer to
reflect 
that it keeps observing screen layout changes and sending notifications to
users.

TEST=ScreenLayoutObserverTest in ash_unittests
BUG=630637
==========

[commit-bot: I haz the power, 2016-09-01 17:38:31]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-09-01 17:41:58]

Description was changed from

==========
Remove the system menu display settings row

Remove the display row in the system menu which show up when number of displays 
changes, screen orientation changes or display resolution changes. However,  
Notification will be keep sending to the notification center as it is now. 

Since display tray is removed, the dependencies to SystemTrayItem from
TrayDisplay 
is also removed. So the class name is updated to screen_layout_observer to
reflect 
that it keeps observing screen layout changes and sending notifications to
users.

TEST=ScreenLayoutObserverTest in ash_unittests
BUG=630637
==========

to

==========
Remove the system menu display settings row

Remove the display row in the system menu which show up when number of displays
changes, screen orientation changes or display resolution changes. However,
Notification will be keep sending to the notification center as it is now.

Since display tray is removed, the dependencies to SystemTrayItem from
TrayDisplay
is also removed. So the class name is updated to screen_layout_observer to
reflect
that it keeps observing screen layout changes and sending notifications to
users.

TEST=ScreenLayoutObserverTest in ash_unittests
BUG=630637

Committed: https://crrev.com/a68ade5167de02938634fa2387e53e20b9353c18
Cr-Commit-Position: refs/heads/master@{#415988}
==========

[commit-bot: I haz the power, 2016-09-01 17:42:00]

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