View: Resizing a layer in RTL now correctly repositions its children.

View: Resizing a layer in RTL now correctly repositions its children.

Split View::BoundsChanged into a new method MirroredBoundsChanged, which
recursively calls MirroredBoundsChanged on children in RTL mode if the
parent's width has changed. This causes the children to be correctly
updated with respect to their new mirrored bounds.

BUG=446407
Committed: https://crrev.com/a0f351d875b4d278883761ac6ffa9e02279391a4
Cr-Commit-Position: refs/heads/master@{#311011}

[Matt Giuca, 2015-01-06 07:08:25]

mgiuca@chromium.org changed reviewers:
+ sky@chromium.org

[Matt Giuca, 2015-01-06 07:08:26]

Hi Scott, PTAL. Thanks!

[sky, 2015-01-06 16:45:50]

https://codereview.chromium.org/819273003/diff/1/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/819273003/diff/1/ui/views/view.cc#newcode1927
ui/views/view.cc:1927: if (base::i18n::IsRTL() && bounds_.width() !=
previous_bounds.width()) {
I get that you need to do something in this case, but do you really need to
recurse through a new function? Do you get the behavior you're after by invoking
UpdateChildLayerBounds on the children?

[Matt Giuca, 2015-01-07 00:48:44]

https://codereview.chromium.org/819273003/diff/1/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/819273003/diff/1/ui/views/view.cc#newcode1927
ui/views/view.cc:1927: if (base::i18n::IsRTL() && bounds_.width() !=
previous_bounds.width()) {
You can't just call UpdateChildLayerBounds on the children --- if the children
are layers (as they are in my test case), then you need to call SetLayerBounds
on them instead. Effectively, you have to duplicate the entire "if (layer())"
block above. So we move that block into a new function (MirroredBoundsChanged).

Now I'm not 100% confident about this, but it seems reasonable that if the
physical (mirrored) bounds of a view have changed, then SetRootBoundsDirty and
SchedulePaintBoundsChanged should be called on those views. I don't have a case
to show that they are needed, but I figured they should be called. So I moved
those calls into MirroredBoundsChanged as well. If you don't think it's
necessary to call those, I can move them back into BoundsChanged.

Now that I think about it, this recursive block itself should not be here --- it
will never recurse more than one level because bounds_.width() will always equal
previous_bounds.width() on the second level of recursion. So I have moved this
loop back out into BoundsChanged.

[sky, 2015-01-07 16:55:57]

https://codereview.chromium.org/819273003/diff/1/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/819273003/diff/1/ui/views/view.cc#newcode1927
ui/views/view.cc:1927: if (base::i18n::IsRTL() && bounds_.width() !=
previous_bounds.width()) {
On 2015/01/07 00:48:44, Matt Giuca wrote:
> You can't just call UpdateChildLayerBounds on the children --- 

UpdateChildLayerBounds calls SetLayerBounds.

> if the children
> are layers (as they are in my test case), then you need to call SetLayerBounds
> on them instead. Effectively, you have to duplicate the entire "if (layer())"
> block above. So we move that block into a new function
(MirroredBoundsChanged).
>
> Now I'm not 100% confident about this, but it seems reasonable that if the
> physical (mirrored) bounds of a view have changed, then SetRootBoundsDirty and
> SchedulePaintBoundsChanged should be called on those views. I don't have a
case
> to show that they are needed, but I figured they should be called. So I moved
> those calls into MirroredBoundsChanged as well. If you don't think it's
> necessary to call those, I can move them back into BoundsChanged.
> 
> Now that I think about it, this recursive block itself should not be here ---
it
> will never recurse more than one level because bounds_.width() will always
equal
> previous_bounds.width() on the second level of recursion. So I have moved this
> loop back out into BoundsChanged.

[Matt Giuca, 2015-01-07 23:30:00]

https://codereview.chromium.org/819273003/diff/1/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/819273003/diff/1/ui/views/view.cc#newcode1927
ui/views/view.cc:1927: if (base::i18n::IsRTL() && bounds_.width() !=
previous_bounds.width()) {
Hmm, yeah it does (didn't notice that before), but it does so "incorrectly". It
doesn't take the mirrored coordinates into account. (I say "incorrectly" in
quotes because it makes sense to do that in the recursive case, but it doesn't
make sense in a direct call from BoundsChanged.)

If I just call UpdateChildLayerBounds on the children instead of calling
MirroredBoundsChanged, the children's layers get an x,y of (0,0) in my test case
(layered view inside a layered view).

So yes, I need to duplicate the whole if block from 1911 to 1923 and apply that
to each child (hence I moved it into a function). I wasn't as sure about the
need to apply the code above that if block (SetRootBoundsDirty and
SchedulePaintBoundsChanged) --- those aren't required for my test cases to pass
or to fix my bug, but as I said in my previous comment, it seems reasonable that
if the mirrored bounds of a view changes, then those two things which depend
upon the mirrored bounds should be refreshed.

[sky, 2015-01-08 03:33:28]

https://codereview.chromium.org/819273003/diff/20001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/819273003/diff/20001/ui/views/view.cc#newcode...
ui/views/view.cc:1897: void View::MirroredBoundsChanged(const gfx::Rect&
previous_bounds) {
I'm still against a new method here. As previously mentioned I think the right
thing to do is call UpdateChildLayerBounds on the children. It may be you need
to calculate the correct offset, but that's ok.

[Matt Giuca, 2015-01-08 04:22:15]

https://codereview.chromium.org/819273003/diff/20001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/819273003/diff/20001/ui/views/view.cc#newcode...
ui/views/view.cc:1897: void View::MirroredBoundsChanged(const gfx::Rect&
previous_bounds) {
Is this what you want?

I find this new version pretty confusing because UpdateChildLayerBounds is not
designed to be called on a view that has a layer; this code is essentially
relying on a quirk of UpdateChildLayerBounds (the fact that it does
SetLayerBounds(GetLayerBounds() + offset)).

Also, I am duplicating almost the entirety of the if/else block above, including
the logic for calculating the offset in both the layer and non-layer case. This
does not sit well for me.

If this isn't what you mean, then I don't know what you do want.

[sky, 2015-01-08 22:05:02]

https://codereview.chromium.org/819273003/diff/20001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/819273003/diff/20001/ui/views/view.cc#newcode...
ui/views/view.cc:1897: void View::MirroredBoundsChanged(const gfx::Rect&
previous_bounds) {
On 2015/01/08 04:22:15, Matt Giuca wrote:
> Is this what you want?
> 
> I find this new version pretty confusing because UpdateChildLayerBounds is not
> designed to be called on a view that has a layer; this code is essentially
> relying on a quirk of UpdateChildLayerBounds (the fact that it does
> SetLayerBounds(GetLayerBounds() + offset)).

What do you mean? UpdateChildLayerBounds handles the case of the view having a
layer or not. Maybe you mean it's weird that we have to call
UpdateChildLayerBounds on all the children of the view? Seems expected given
what we want to do.

> Also, I am duplicating almost the entirety of the if/else block above,
including
> the logic for calculating the offset in both the layer and non-layer case.
This
> does not sit well for me.
> 
> If this isn't what you mean, then I don't know what you do want.

I think the following does what you want:

    // In RTL mode, if our width has changed, our children's mirrored bounds
    // will have changed. We have to call UpdateChildLayerBounds() here for each
    // child as UpdateChildLayerBounds() stops if the View has a layer, and we
    // have a layer. We need only do this we have a layer as the else case
    // descends into the children.
    if (base::i18n::IsRTL() && bounds_.width() != previous_bounds.width()) {
      for (int i = 0; i < child_count(); ++i) {
        View* child = child_at(i);
        child->UpdateChildLayerBounds(
            gfx::Vector2d(child->GetMirroredX(), child->y()));
      }
    }

This goes in the if (layer()) conditional (after the SetLayerBounds).

https://codereview.chromium.org/819273003/diff/40001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/819273003/diff/40001/ui/views/view_unittest.c...
ui/views/view_unittest.cc:2935: // Now attach a paints-to-layer View as a child
view of |v1|.
Please write a new test for covering new functionality. Continually adding
assertions to tests makes for monolithic tests, which are hard to reason about.

[Matt Giuca, 2015-01-09 03:28:05]

https://codereview.chromium.org/819273003/diff/20001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/819273003/diff/20001/ui/views/view.cc#newcode...
ui/views/view.cc:1897: void View::MirroredBoundsChanged(const gfx::Rect&
previous_bounds) {
OK I see how you simplified it. I've verified that your suggestion does the same
thing as my original code (other than the calls to SetRootBoundsDirty and
SchedulePaintBoundsChanged).

(I meant it was weird to call "update *child* layer bounds" on our child ---
essentially meaning to update our grandchild's layer bounds --- but it does not
update the grandchild, only the child's layer bounds. Perhaps just the name
UpdateChildLayerBounds is misleading since it can potentially not recurse to
children. The logic makes sense to me now, modulo the naming.)

I am still wondering whether we also want to call SetRootBoundsDirty and
SchedulePaintBoundsChanged on the children, as I had in my original patch. But
you seem not to think so, and none of my test cases show that it is necessary,
so I am happy to leave that out.

https://codereview.chromium.org/819273003/diff/40001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/819273003/diff/40001/ui/views/view_unittest.c...
ui/views/view_unittest.cc:2935: // Now attach a paints-to-layer View as a child
view of |v1|.
On 2015/01/08 22:05:02, sky wrote:
> Please write a new test for covering new functionality. Continually adding
> assertions to tests makes for monolithic tests, which are hard to reason
about.

Done, and added a bunch of new asserts to the new test ResizeParentInRTL (to
test a layer view inside a non-layer view inside a layer view).

[Matt Giuca, 2015-01-09 03:30:57]

Note: Removed mention of the app launcher, and the app_list bug
(http://crbug.com/441962) in the CL description, since I was forced to
work-around that bug to merge into M40 (so this CL no longer directly fixes that
bug, though it will make the workaround obsolete).

[sky, 2015-01-09 16:56:38]

LGTM

Layer's are painted separately from views. So moving layers around triggers the
appropriate repaint for them in Layer::SetBounds (well, really
SetBoundsFromAnimation).

[Matt Giuca, 2015-01-11 23:53:41]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2015-01-11 23:54:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/819273003/60001

[commit-bot: I haz the power, 2015-01-12 00:03:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-12 00:04:36]

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