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

ui/views/view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #define _USE_MATH_DEFINES // For VC++ to get M_PI. This has to be first.
 
 #include "ui/views/view.h"
 
 #include <algorithm>
 #include <cmath>
@@ skipping 1876 matching lines @@
 void View::PropagateVisibilityNotifications(View* start, bool is_visible) {
   for (int i = 0, count = child_count(); i < count; ++i)
     child_at(i)->PropagateVisibilityNotifications(start, is_visible);
   VisibilityChangedImpl(start, is_visible);
 }
 
 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 =
       previous_bounds.OffsetFromOrigin() != bounds_.OffsetFromOrigin();
   SetRootBoundsDirty(origin_changed);
 
   if (visible_) {
     // Paint the new bounds.
     SchedulePaintBoundsChanged(
         bounds_.size() == previous_bounds.size() ? SCHEDULE_PAINT_SIZE_SAME :
         SCHEDULE_PAINT_SIZE_CHANGED);
   }
 
   if (layer()) {
     if (parent_) {
       SetLayerBounds(GetLocalBounds() +
                      gfx::Vector2d(GetMirroredX(), y()) +
                      parent_->CalculateOffsetToAncestorWithLayer(NULL));
     } else {
       SetLayerBounds(bounds_);
     }
   } else {
     // If our bounds have changed, then any descendant layer bounds may have
     // 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);
 
   if (previous_bounds.size() != size()) {
     needs_layout_ = false;
     Layout();
   }
 
   if (GetNeedsNotificationWhenVisibleBoundsChange())
     OnVisibleBoundsChanged();
@@ skipping 518 matching lines @@
   // Message the RootView to do the drag and drop. That way if we're removed
   // the RootView can detect it and avoid calling us back.
   gfx::Point widget_location(event.location());
   ConvertPointToWidget(this, &widget_location);
   widget->RunShellDrag(this, data, widget_location, drag_operations, source);
   // WARNING: we may have been deleted.
   return true;
 }
 
 }  // namespace views