[Mac] Flip toolbar in RTL

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

 // Copyright (c) 2011 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.
 
 #import "chrome/browser/ui/cocoa/extensions/browser_actions_container_view.h"
 
 #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"
 
 NSString* const kBrowserActionGrippyDragStartedNotification =
     @"BrowserActionGrippyDragStartedNotification";
 NSString* const kBrowserActionGrippyDraggingNotification =
     @"BrowserActionGrippyDraggingNotification";
 NSString* const kBrowserActionGrippyDragFinishedNotification =
     @"BrowserActionGrippyDragFinishedNotification";
@@ skipping 31 matching lines @@
 @synthesize maxDesiredWidth = maxDesiredWidth_;
 @synthesize userIsResizing = userIsResizing_;
 @synthesize delegate = delegate_;
 
 #pragma mark -
 #pragma mark Overridden Class Functions
 
 - (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;
 
     resizeAnimation_.reset([[NSViewAnimation alloc] init]);
     [resizeAnimation_ setDuration:kAnimationDuration];
     [resizeAnimation_ setAnimationBlockingMode:NSAnimationNonblocking];
     [resizeAnimation_ setDelegate:self];
 
     [self setHidden:YES];
@@ skipping 84 matching lines @@
 
 - (void)setIsOverflow:(BOOL)isOverflow {
   if (isOverflow_ != isOverflow) {
     isOverflow_ = isOverflow;
     resizable_ = !isOverflow_;
     [self setNeedsDisplay:YES];
   }
 }
 
 - (void)resetCursorRects {
+  if (cocoa_l10n_util::ShouldDoExperimentalRTLLayout())
+    grippyRect_.origin.x = NSWidth([self frame]) - NSWidth(grippyRect_);
   [self addCursorRect:grippyRect_ cursor:[self appropriateCursorForGrippy]];
 }
 
 - (BOOL)acceptsFirstResponder {
   // The overflow container needs to receive key events to handle in-item
   // navigation. The top-level container should not become first responder,
   // allowing focus travel to proceed to the first action.
   return isOverflow_;
 }
 
@@ skipping 31 matching lines @@
 }
 
 - (void)mouseDragged:(NSEvent*)theEvent {
   if (!userIsResizing_)
     return;
 
   NSPoint location = [self convertPoint:[theEvent locationInWindow]
                                fromView:nil];
   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];
 
   [[NSNotificationCenter defaultCenter]
       postNotificationName:kBrowserActionGrippyDraggingNotification
                     object:self];
 }
 
 - (void)animationDidEnd:(NSAnimation*)animation {
@@ skipping 25 matching lines @@
 #pragma mark -
 #pragma mark Public Methods
 
 - (void)resizeToWidth:(CGFloat)width animate:(BOOL)animate {
   width = std::max(width, kMinimumContainerWidth);
   NSRect frame = [self frame];
 
   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;
 
   [self stopAnimation];
 
   if (animate) {
     NSDictionary* animationDictionary = @{
       NSViewAnimationTargetKey : self,
       NSViewAnimationStartFrameKey : [NSValue valueWithRect:[self frame]],
       NSViewAnimationEndFrameKey : [NSValue valueWithRect:newFrame]
@@ skipping 46 matching lines @@
     retVal = [NSCursor resizeLeftRightCursor];
   }
   return retVal;
 }
 
 - (CGFloat)maxAllowedWidth {
   return delegate_ ? delegate_->GetMaxAllowedWidth() : CGFLOAT_MAX;
 }
 
 @end