[Mac] Flip toolbar in RTL

chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm

Index: chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm
diff --git a/chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm b/chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm
index 97ed1a6ea1085fd73592af01ce133792e3657b81..9228a769035c940640462db20e8ea4b4adb0ff3e 100644
--- a/chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm
+++ b/chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm
@@ -7,6 +7,7 @@
 #include <algorithm>
 #include <utility>
 
+#import "chrome/browser/ui/cocoa/l10n_util.h"
 #import "chrome/browser/ui/cocoa/view_id_util.h"
 #include "ui/base/cocoa/appkit_utils.h"
 #include "ui/events/keycodes/keyboard_code_conversion_mac.h"
@@ -58,6 +59,9 @@ const CGFloat kMinimumContainerWidth = 3.0;
 - (id)initWithFrame:(NSRect)frameRect {
   if ((self = [super initWithFrame:frameRect])) {
     grippyRect_ = NSMakeRect(0.0, 0.0, kGrippyWidth, NSHeight([self bounds]));
+    if (cocoa_l10n_util::ShouldDoExperimentalRTLLayout())
+      grippyRect_.origin.x = NSWidth(frameRect) - NSWidth(grippyRect_);
+
     canDragLeft_ = YES;
     canDragRight_ = YES;
     resizable_ = YES;
@@ -162,6 +166,8 @@ const CGFloat kMinimumContainerWidth = 3.0;
 }
 
 - (void)resetCursorRects {
+  if (cocoa_l10n_util::ShouldDoExperimentalRTLLayout())
+    grippyRect_.origin.x = NSWidth([self frame]) - NSWidth(grippyRect_);
   [self addCursorRect:grippyRect_ cursor:[self appropriateCursorForGrippy]];
 }
 
@@ -214,23 +220,35 @@ const CGFloat kMinimumContainerWidth = 3.0;
   NSRect containerFrame = [self frame];
   CGFloat dX = [theEvent deltaX];
   CGFloat withDelta = location.x - dX;

[tapted, 2017/01/04 06:08:09]

should this be widthDelta?

[lgrey, 2017/01/04 17:36:38]

My understanding is it's "the drag point with the delta applied" aka if the
location at the previous tick is p1, withDelta is p1.x. I thought about renaming
this but couldn't think of something better (|dragStart| is inaccurate since the
drag start point is where the mouse went down, whereas withDelta is the location
before THIS drag event.)

[tapted, 2017/01/04 22:57:50]

Acknowledged.

-  canDragRight_ = (withDelta >= initialDragPoint_.x) &&
-      (NSWidth(containerFrame) > kMinimumContainerWidth);
+
   CGFloat maxAllowedWidth = [self maxAllowedWidth];
-  containerFrame.size.width =
-      std::max(NSWidth(containerFrame) - dX, kMinimumContainerWidth);
-  canDragLeft_ = withDelta <= initialDragPoint_.x &&
-      NSWidth(containerFrame) < maxDesiredWidth_ &&
-      NSWidth(containerFrame) < maxAllowedWidth;
 
-  if ((dX < 0.0 && !canDragLeft_) || (dX > 0.0 && !canDragRight_))
+  containerFrame.size.width = std::min(

[tapted, 2017/01/04 06:08:09]

does this need to clamp to maxAllowedWidth as well as maxDesiredWidth_?

Perhaps break this up - e.g. something like

  const CGFloat maxWidth = std::min(maxAllowedWidth, maxDesiredWidth_);
  GGFloat newWidth = NSWidth(containerFrame) + (RTL ? dX -dX);
  newWidth = std::min(std::max(newWidth, kMinimumContainerWidth), maxWidth);


Then only apply newWidth to the frame when you need to set the origin as well.

[lgrey, 2017/01/04 17:36:38]

How's this?

[tapted, 2017/01/04 22:57:50]

nice :)

+      maxDesiredWidth_,
+      std::max(
+          NSWidth(containerFrame) +
+              (cocoa_l10n_util::ShouldDoExperimentalRTLLayout() ? dX : -dX),
+          kMinimumContainerWidth));
+  BOOL canGrow = NSWidth(containerFrame) < maxDesiredWidth_ &&
+                 NSWidth(containerFrame) < maxAllowedWidth;
+  BOOL canShrink = NSWidth(containerFrame) > kMinimumContainerWidth;
+
+  canDragLeft_ =
+      withDelta <= initialDragPoint_.x &&
+      (cocoa_l10n_util::ShouldDoExperimentalRTLLayout() ? canShrink : canGrow);
+  canDragRight_ =
+      (withDelta >= initialDragPoint_.x) &&
+      (cocoa_l10n_util::ShouldDoExperimentalRTLLayout() ? canGrow : canShrink);
+  if ((dX < 0.0 && !canDragLeft_) || (dX > 0.0 && !canDragRight_) ||
+      fabs(dX) < FLT_EPSILON)
     return;
 
   if (NSWidth(containerFrame) <= kMinimumContainerWidth)
     return;
 
   grippyPinned_ = NSWidth(containerFrame) >= maxAllowedWidth;
-  containerFrame.origin.x += dX;
+  if (!cocoa_l10n_util::ShouldDoExperimentalRTLLayout())
+    containerFrame.origin.x += dX;
 
   [self setFrame:containerFrame];
   [self setNeedsDisplay:YES];
@@ -276,9 +294,15 @@ const CGFloat kMinimumContainerWidth = 3.0;
   CGFloat maxAllowedWidth = [self maxAllowedWidth];
   width = std::min(maxAllowedWidth, width);
 
-  CGFloat dX = frame.size.width - width;
-  frame.size.width = width;
-  NSRect newFrame = NSOffsetRect(frame, dX, 0);
+  NSRect newFrame;

[tapted, 2017/01/04 06:08:09]

|newFrame| doesn't really seem to serve a purpose distinct from |frame|, and
it's confusing having extra variables - can we just delete the |frame| dec? I'm
thinking something like


NSRect newFrame = [self frame];
CGFloat widthDelta = NSWidth(newFrame) - width;
newFrame.size.width = width;

if (!cocoa_l10n_util::ShouldDoExperimentalRTLLayout())
  newFrame.origin.x += widthDelta;

[lgrey, 2017/01/04 17:36:38]

Had this thought as well, but check the |if (animate)| clause below

[tapted, 2017/01/04 22:57:50]

I think that's currently using [self frame], not |frame|  (so another reason not
to have |frame| would be to avoid the shadowing :).

[lgrey, 2017/01/05 20:50:33]

Done.

+  if (cocoa_l10n_util::ShouldDoExperimentalRTLLayout()) {
+    newFrame = frame;
+    newFrame.size.width = width;
+  } else {
+    CGFloat dX = frame.size.width - width;
+    frame.size.width = width;
+    newFrame = NSOffsetRect(frame, dX, 0);
+  }
 
   grippyPinned_ = width == maxAllowedWidth;