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) {
   // 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));
   }
 
+  // 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()) {
     needs_layout_ = false;
     Layout();
   }
 
   if (GetNeedsNotificationWhenVisibleBoundsChange())
     OnVisibleBoundsChanged();
 
@@ skipping 517 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