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

ui/views/view.cc

Index: ui/views/view.cc
diff --git a/ui/views/view.cc b/ui/views/view.cc
index 7bf2ee68bc5512afdb7cb7ecbedbbf1fe6b0de2a..b0a5bf5035bafa1a352a2e8d65d32dc587c2ba67 100644
--- a/ui/views/view.cc
+++ b/ui/views/view.cc
@@ -1894,7 +1894,7 @@ void View::VisibilityChangedImpl(View* starting_from, bool is_visible) {
   VisibilityChanged(starting_from, is_visible);
 }
 
-void View::BoundsChanged(const gfx::Rect& previous_bounds) {
+void View::MirroredBoundsChanged(const gfx::Rect& previous_bounds) {
   // Mark our bounds as dirty for the paint root, also see if we need to
   // recompute our children's bounds due to origin change.
   bool origin_changed =
@@ -1922,6 +1922,19 @@ void View::BoundsChanged(const gfx::Rect& previous_bounds) {
     UpdateChildLayerBounds(CalculateOffsetToAncestorWithLayer(NULL));
   }
 
+  // In RTL mode, if our width has changed, our children's mirrored bounds will
+  // have changed. Notify them, so that they can update their layer bounds, etc.
+  if (base::i18n::IsRTL() && bounds_.width() != previous_bounds.width()) {

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

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]

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:56]

UpdateChildLayerBounds calls SetLayerBounds.

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

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.

+    for (int i = 0; i < child_count(); ++i) {
+      View* child = child_at(i);
+      child->MirroredBoundsChanged(child->bounds());
+    }
+  }
+}
+
+void View::BoundsChanged(const gfx::Rect& previous_bounds) {
+  MirroredBoundsChanged(previous_bounds);
+
   OnBoundsChanged(previous_bounds);
 
   if (previous_bounds.size() != size()) {