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..f8477959242a7fb47f2b46669f61228769f19939 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) {

[sky, 2015/01/08 03:33:27]

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]

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]

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.
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).

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

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.

   // 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 =
@@ -1921,6 +1921,19 @@ void View::BoundsChanged(const gfx::Rect& previous_bounds) {
     // changed. Update them accordingly.
     UpdateChildLayerBounds(CalculateOffsetToAncestorWithLayer(NULL));
   }
+}
+
+void View::BoundsChanged(const gfx::Rect& previous_bounds) {
+  MirroredBoundsChanged(previous_bounds);
+
+  // 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()) {
+    for (int i = 0; i < child_count(); ++i) {
+      View* child = child_at(i);
+      child->MirroredBoundsChanged(child->bounds());
+    }
+  }
 
   OnBoundsChanged(previous_bounds);