Update the window labels if they change in overview mode.

Update the window labels if they change in overview mode.

This code adds a function to a WindowObserver that notfies it of a window title
change, allowing to update the label in overview mode dynamically.

BUG=387130
TEST=WindowSelectorTest.CreateLabelUnderWindow

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

[Nina, 2014-06-20 20:57:19]

Terry, can you give this patch a first look? Thanks!

[tdanderson, 2014-06-20 22:33:23]

Nice! LGTM. (a couple of nits below)

https://codereview.chromium.org/331643004/diff/1/ash/wm/overview/window_selec...
File ash/wm/overview/window_selector_panels.cc (right):

https://codereview.chromium.org/331643004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector_panels.cc:147: if (window ==
transform_windows_[0]->window()) {
Brief comment to explain what you're doing in lines 147-150.

https://codereview.chromium.org/331643004/diff/1/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/331643004/diff/1/ui/aura/window.cc#newcode311
ui/aura/window.cc:311: title));
Each of the arguments of FOR_EACH_OBSERVER on a newline.

[Nina, 2014-06-20 23:02:37]

flackr, can you look at this patch?

Terry, I got the nits fixed.

Thanks!

https://codereview.chromium.org/331643004/diff/1/ash/wm/overview/window_selec...
File ash/wm/overview/window_selector_panels.cc (right):

https://codereview.chromium.org/331643004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector_panels.cc:147: if (window ==
transform_windows_[0]->window()) {
On 2014/06/20 22:33:23, tdanderson wrote:
> Brief comment to explain what you're doing in lines 147-150.

Done.

https://codereview.chromium.org/331643004/diff/1/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/331643004/diff/1/ui/aura/window.cc#newcode311
ui/aura/window.cc:311: title));
On 2014/06/20 22:33:23, tdanderson wrote:
> Each of the arguments of FOR_EACH_OBSERVER on a newline.

Done.

[flackr, 2014-06-24 17:10:52]

lgtm with nits

https://codereview.chromium.org/331643004/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.cc (right):

https://codereview.chromium.org/331643004/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:96:
transform_windows_[0]->window()->RemoveObserver(this);
nit: For consistency use transform_windows_.front() here and elsewhere.

https://codereview.chromium.org/331643004/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:148: // stop doing so before it is
destructed.
I would also be fine with observing all of the windows but only updating the
label when the window we care about had a title change if it made this simpler.
Eventually I'd like to be able to handle bounds changes and other events in here
rather than having windowselector/windowgrid have to observe every window.

https://codereview.chromium.org/331643004/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/331643004/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:771: panel2->SetTitle(panel2_title);
Maybe test update of panel1 title and after deleting panel1, an update of panel2
title to cover the different observer behavior in panels?

https://codereview.chromium.org/331643004/diff/40001/ui/aura/window_observer.h
File ui/aura/window_observer.h (right):

https://codereview.chromium.org/331643004/diff/40001/ui/aura/window_observer....
ui/aura/window_observer.h:118: const base::string16& new_title) {}
nit: since we're not passing the old title I would just call this title instead
of new_title.

[Nina, 2014-06-24 18:36:28]

Got the nits sorted out. Terry, would you please check the CQ bit? Thanks!

https://codereview.chromium.org/331643004/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.cc (right):

https://codereview.chromium.org/331643004/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:96:
transform_windows_[0]->window()->RemoveObserver(this);
On 2014/06/24 17:10:51, flackr wrote:
> nit: For consistency use transform_windows_.front() here and elsewhere.

Done.

https://codereview.chromium.org/331643004/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:148: // stop doing so before it is
destructed.
On 2014/06/24 17:10:51, flackr wrote:
> I would also be fine with observing all of the windows but only updating the
> label when the window we care about had a title change if it made this
simpler.
> Eventually I'd like to be able to handle bounds changes and other events in
here
> rather than having windowselector/windowgrid have to observe every window.

Changed the code to observe all of the transform windows, I agree that this does
make it simpler to transition to handling the bounds change in the
WindowSelectorItem.

https://codereview.chromium.org/331643004/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/331643004/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:771: panel2->SetTitle(panel2_title);
On 2014/06/24 17:10:51, flackr wrote:
> Maybe test update of panel1 title and after deleting panel1, an update of
panel2
> title to cover the different observer behavior in panels?

Updated test to cover these cases plus changing the label of panel1.

https://codereview.chromium.org/331643004/diff/40001/ui/aura/window_observer.h
File ui/aura/window_observer.h (right):

https://codereview.chromium.org/331643004/diff/40001/ui/aura/window_observer....
ui/aura/window_observer.h:118: const base::string16& new_title) {}
On 2014/06/24 17:10:51, flackr wrote:
> nit: since we're not passing the old title I would just call this title
instead
> of new_title.

Done.

[tdanderson, 2014-06-24 18:38:36]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-06-24 18:40:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nsatragno@chromium.org/331643004/60001

[commit-bot: I haz the power, 2014-06-24 22:02:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-24 22:05:29]

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

[commit-bot: I haz the power, 2014-06-24 22:05:31]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Nina, 2014-06-24 22:10:49]

Hi Sky,

Can you please review this change? ui/*, content/* and  ash/display/* need
review from an owner. Should be trivial.

Thanks!

[sky, 2014-06-25 16:24:44]

https://codereview.chromium.org/331643004/diff/60001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/331643004/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:175: static_cast<views::Label*>(
static_casts like this are often times fragile, in so far as it's easy for
someone to change the contents view code and have this continue to compile and
only get crashes later on. I would rather you add the label as a member and use
said member here.

https://codereview.chromium.org/331643004/diff/60001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/331643004/diff/60001/ui/aura/window.cc#newcod...
ui/aura/window.cc:312: OnWindowTitleChanged(this, title));
Callers that care about the title can ask for it. The only interesting thing
would be the old title, but since no one needs that either make
OnWindowTitleChanged only take the window.

[Nina, 2014-06-25 19:54:41]

Sky, please give the patch another look. Thanks!

https://codereview.chromium.org/331643004/diff/60001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/331643004/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:175: static_cast<views::Label*>(
On 2014/06/25 16:24:44, sky wrote:
> static_casts like this are often times fragile, in so far as it's easy for
> someone to change the contents view code and have this continue to compile and
> only get crashes later on. I would rather you add the label as a member and
use
> said member here.

Okay, naturally I also replaced the other places in the code where the static
cast was done. Please give it a new look.

https://codereview.chromium.org/331643004/diff/60001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/331643004/diff/60001/ui/aura/window.cc#newcod...
ui/aura/window.cc:312: OnWindowTitleChanged(this, title));
On 2014/06/25 16:24:44, sky wrote:
> Callers that care about the title can ask for it. The only interesting thing
> would be the old title, but since no one needs that either make
> OnWindowTitleChanged only take the window.

Done. I was following the same logic as OnWindowParentChanged,
OnWindowVisibility*, OnWindowBoundsChanged and OnWindowRemovingFromRootWindow.
Perhaps we should also change those?

https://codereview.chromium.org/331643004/diff/80001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/331643004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:81: window_label_view_(NULL) {
This is so that the tests fail gracefully if, for some reason, the label is not
created.

https://codereview.chromium.org/331643004/diff/80001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/331643004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:761: EXPECT_EQ(real_label_bounds,
expected_label_bounds);
This was a NO-OP before... I realized and changed it so that it works.

[sky, 2014-06-25 21:04:07]

LGTM

[tdanderson, 2014-06-25 21:18:11]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-06-25 21:20:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nsatragno@chromium.org/331643004/80001

[commit-bot: I haz the power, 2014-06-26 00:42:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-26 01:55:28]

Change committed as 279887