Add View::ReorderChildView and Widget::MoveToTop.

Add View::ReorderChildView and Widget::MoveToTop.

The z-ordering of child views is currently maintained (implicitly?) by the order
they are added to the parent view. It may often be necessary to change the
z-order of views, and View::ReorderChildView can be used to do that.

I changed views_desktop to have the two pure-views windows to have overlapping
regions, and View::ReorderChildView is used to change the z-ordering of the two
windows. So it is possible to click a window to activate it and bring it on top
of the other. It could have been done without adding Widget::MoveToTop, but
given Widget::MoveAbove and MoveAboveWidget, it sounded like a good idea to have
Widget::MoveToTop (MoveToFront?). I do now know what the implementation for
windows should be, though. So left it as NOTIMPLEMENTED.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89363

[Ben Goodger (Google), 2011-06-16 14:35:22]

LGTM. Cool, sky & I were just talking about how we needed this yesterday.

[tfarina, 2011-06-16 14:58:05]

http://codereview.chromium.org/7185005/diff/1/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/7185005/diff/1/views/view.cc#newcode180
views/view.cc:180: DCHECK(view->parent() == this);
view->parent_ please!

DCHECK_EQ(view->parent_, this);

[sadrul, 2011-06-16 15:37:13]

I also explicitly turned on paint_to_texture_ for the NativeWidgetView so that
it always uses accelerated painting.

http://codereview.chromium.org/7185005/diff/1/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/7185005/diff/1/views/view.cc#newcode180
views/view.cc:180: DCHECK(view->parent() == this);
On 2011/06/16 14:58:05, tfarina wrote:
> view->parent_ please!
> 
> DCHECK_EQ(view->parent_, this);

Done.

[tfarina, 2011-06-16 15:40:10]

http://codereview.chromium.org/7185005/diff/3002/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/7185005/diff/3002/views/view.cc#newcode179
views/view.cc:179: void View::ReorderChildView(View* view, int index) {
Could you add an unittest for this function right after GetIndexOf in
view_unittest.cc?

[sadrul, 2011-06-16 15:55:18]

http://codereview.chromium.org/7185005/diff/3002/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/7185005/diff/3002/views/view.cc#newcode179
views/view.cc:179: void View::ReorderChildView(View* view, int index) {
On 2011/06/16 15:40:10, tfarina wrote:
> Could you add an unittest for this function right after GetIndexOf in
> view_unittest.cc?

Sounds like a good idea. Done!

[tfarina, 2011-06-16 15:59:42]

http://codereview.chromium.org/7185005/diff/1013/views/view_unittest.cc
File views/view_unittest.cc (right):

http://codereview.chromium.org/7185005/diff/1013/views/view_unittest.cc#newco...
views/view_unittest.cc:2147: // Move |foo1| at the end.
Could you test adding it to another position besides the end also works?

http://codereview.chromium.org/7185005/diff/1013/views/view_unittest.cc#newco...
views/view_unittest.cc:2155: }
Nice! Thanks for adding it. Looks good.

[sadrul, 2011-06-16 16:12:54]

http://codereview.chromium.org/7185005/diff/1013/views/view_unittest.cc
File views/view_unittest.cc (right):

http://codereview.chromium.org/7185005/diff/1013/views/view_unittest.cc#newco...
views/view_unittest.cc:2147: // Move |foo1| at the end.
On 2011/06/16 15:59:42, tfarina wrote:
> Could you test adding it to another position besides the end also works?

Done.

[tfarina, 2011-06-16 16:18:24]

Code I care, LGTM.

http://codereview.chromium.org/7185005/diff/2003/views/view_unittest.cc
File views/view_unittest.cc (right):

http://codereview.chromium.org/7185005/diff/2003/views/view_unittest.cc#newco...
views/view_unittest.cc:2115: TEST_F(ViewTest, ReorderChildren) {
I'd add a comment like this (but it's OK to not too):

// The tree looks like this:
// root
// |-- child
// |   |-- foo1
// |   |-- foo2
// +---|-- foo3

http://codereview.chromium.org/7185005/diff/2003/views/view_unittest.cc#newco...
views/view_unittest.cc:2157: // Move |foo2| at the front.
nit: at -> to?

[sadrul, 2011-06-16 16:25:01]

http://codereview.chromium.org/7185005/diff/2003/views/view_unittest.cc
File views/view_unittest.cc (right):

http://codereview.chromium.org/7185005/diff/2003/views/view_unittest.cc#newco...
views/view_unittest.cc:2115: TEST_F(ViewTest, ReorderChildren) {
On 2011/06/16 16:18:24, tfarina wrote:
> I'd add a comment like this (but it's OK to not too):
> 
> // The tree looks like this:
> // root
> // |-- child
> // |   |-- foo1
> // |   |-- foo2
> // +---|-- foo3

I think the view hierarchy is pretty self-evident, and since the hierarchy is
changed a lot of time, it probably doesn't make sense to draw the hierarchy :)

http://codereview.chromium.org/7185005/diff/2003/views/view_unittest.cc#newco...
views/view_unittest.cc:2157: // Move |foo2| at the front.
On 2011/06/16 16:18:24, tfarina wrote:
> nit: at -> to?

Done.

[Ben Goodger (Google), 2011-06-16 16:25:06]

http://codereview.chromium.org/7185005/diff/2003/views/view_unittest.cc
File views/view_unittest.cc (right):

http://codereview.chromium.org/7185005/diff/2003/views/view_unittest.cc#newco...
views/view_unittest.cc:2115: TEST_F(ViewTest, ReorderChildren) {
I think you mean:

On 2011/06/16 16:18:24, tfarina wrote:
> // root
> // +-- child
> //     +-- foo1
> //     +-- foo2
> //     +-- foo3

... though I'm not sure a tree this shallow needs a diagram.

[tfarina, 2011-06-16 16:28:12]

http://codereview.chromium.org/7185005/diff/2003/views/view_unittest.cc
File views/view_unittest.cc (right):

http://codereview.chromium.org/7185005/diff/2003/views/view_unittest.cc#newco...
views/view_unittest.cc:2115: TEST_F(ViewTest, ReorderChildren) {
On 2011/06/16 16:25:01, sadrul wrote:
> On 2011/06/16 16:18:24, tfarina wrote:
> > I'd add a comment like this (but it's OK to not too):
> > 
> > // The tree looks like this:
> > // root
> > // |-- child
> > // |   |-- foo1
> > // |   |-- foo2
> > // +---|-- foo3
> 
> I think the view hierarchy is pretty self-evident, and since the hierarchy is
> changed a lot of time, it probably doesn't make sense to draw the hierarchy :)

Sure, that why I said I'm OK to not have one. It's just a hint for me and for
any further reader to know instantly (without having to guess/read from the
code) how the tree we're testing looks like.