Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2607533004:
[Mac] Flip toolbar in RTL (Closed)
Description[Mac] Flip toolbar in RTL
Browser actions will be reordered in a future change.
BUG=648558, 648563, 673362
Review-Url: https://codereview.chromium.org/2607533004
Cr-Commit-Position: refs/heads/master@{#441925}
Committed: https://chromium.googlesource.com/chromium/src/+/8b0fd02afc2107ba123774e0a464a43407a108cf
Patch Set 1 #Patch Set 2 : Toolbar test #
Total comments: 1
Patch Set 3 : Remove unused method #
Total comments: 33
Patch Set 4 : tapted@ comments #
Total comments: 10
Patch Set 5 : tapted@ comments #
Messages
Total messages: 19 (9 generated)
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Mac] Flip toolbar in RTL (WIP) BUG=648558, 648563, 673362 ========== to ========== [Mac] Flip toolbar in RTL Browser actions will be reordered in a future change. BUG=648558, 648563, 673362 ==========
lgrey@chromium.org changed reviewers: + tapted@chromium.org
Trent PTAL :) https://codereview.chromium.org/2607533004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/2607533004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:728: // Returns an array of views, ordered leading to trailing. The only thing that depended on this order was the test, which was also updated.
looks pretty good - mostly nits. (But TBH my eyes started glazing over with all the arithmetic..) https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm (right): https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm:222: CGFloat withDelta = location.x - dX; should this be widthDelta? https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm:226: containerFrame.size.width = std::min( 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. https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm:297: NSRect newFrame; |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; https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right): https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:587: // edge in the container so that, if the container is animating, the nit: rewrap https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.h (right): https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:203: - (void)toolbarFrameChanged; this needs a comment, per go/objcguide#Declaration_Comments https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:326: for (NSUInteger i = 0; i < leadingButtons.count; i++) { nit: [leadingButtons count] (dot notation never caught on in Chrome) https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:336: button.autoresizingMask = leadingButtonMask; [button setAutoresizingMask:leadingButtonMask] https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:362: appMenuButton_.autoresizingMask = trailingButtonMask; setAutoresizingMask https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:382: rightEdge = menuButtonFrame.origin.x; NSMinX? https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:384: locationBarFrame.size.width = rightEdge - locationBarFrame.origin.x; NSMinX? https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:397: browserActionsContainerView_.autoresizingMask = trailingButtonMask; setAutoresizingMask: https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:730: return [NSArray arrayWithObjects:backButton_, forwardButton_, reloadButton_, return @[ ... https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm (right): https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:218: EXPECT_FALSE([GetSubviewAt(kBrowserActionContainerViewIndex) isHidden]); Why this change? https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:445: }; nit: DISALLOW_COPY_AND_ASSIGN(.. (you'll need a default constructor)
https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm (right): https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm:222: CGFloat withDelta = location.x - dX; On 2017/01/04 06:08:09, tapted wrote: > should this be widthDelta? 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.) https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm:226: containerFrame.size.width = std::min( On 2017/01/04 06:08:09, tapted wrote: > 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. How's this? https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm:297: NSRect newFrame; On 2017/01/04 06:08:09, tapted wrote: > |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; > > Had this thought as well, but check the |if (animate)| clause below https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right): https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:587: // edge in the container so that, if the container is animating, the On 2017/01/04 06:08:09, tapted wrote: > nit: rewrap Done. https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.h (right): https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:203: - (void)toolbarFrameChanged; On 2017/01/04 06:08:09, tapted wrote: > this needs a comment, per go/objcguide#Declaration_Comments Done. https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:326: for (NSUInteger i = 0; i < leadingButtons.count; i++) { On 2017/01/04 06:08:10, tapted wrote: > nit: [leadingButtons count] > > (dot notation never caught on in Chrome) Done. https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:336: button.autoresizingMask = leadingButtonMask; On 2017/01/04 06:08:10, tapted wrote: > [button setAutoresizingMask:leadingButtonMask] Done. https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:362: appMenuButton_.autoresizingMask = trailingButtonMask; On 2017/01/04 06:08:10, tapted wrote: > setAutoresizingMask Done. https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:382: rightEdge = menuButtonFrame.origin.x; On 2017/01/04 06:08:10, tapted wrote: > NSMinX? Done. https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:384: locationBarFrame.size.width = rightEdge - locationBarFrame.origin.x; On 2017/01/04 06:08:09, tapted wrote: > NSMinX? Done. https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:397: browserActionsContainerView_.autoresizingMask = trailingButtonMask; On 2017/01/04 06:08:09, tapted wrote: > setAutoresizingMask: Done. https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:730: return [NSArray arrayWithObjects:backButton_, forwardButton_, reloadButton_, On 2017/01/04 06:08:10, tapted wrote: > return @[ ... Done. https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm (right): https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:218: EXPECT_FALSE([GetSubviewAt(kBrowserActionContainerViewIndex) isHidden]); On 2017/01/04 06:08:10, tapted wrote: > Why this change? Previously, there were no extensions so this was trivially true, since it's always hidden with no actions. Now, it should be shown whenever the buttons are in this test. https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:445: }; On 2017/01/04 06:08:10, tapted wrote: > nit: DISALLOW_COPY_AND_ASSIGN(.. > > (you'll need a default constructor) Done, but this doesn't seem very common for CocoaProfileTest subclasses (https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/extensions/choos..., most of those hits are for helper classes.) Why do we need it here? (For my info, not pushing back :))
lgtm % nits (I think you could still update resizeToWidth: in browser_actions_container_view.mm - up to you, but I don't want to block you, and it looks ok as-is). https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm (right): https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm:222: CGFloat withDelta = location.x - dX; On 2017/01/04 17:36:38, lgrey wrote: > On 2017/01/04 06:08:09, tapted wrote: > > should this be widthDelta? > > 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.) Acknowledged. https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm:226: containerFrame.size.width = std::min( On 2017/01/04 17:36:38, lgrey wrote: > On 2017/01/04 06:08:09, tapted wrote: > > 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. > > How's this? nice :) https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm:297: NSRect newFrame; On 2017/01/04 17:36:38, lgrey wrote: > On 2017/01/04 06:08:09, tapted wrote: > > |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; > > > > > > Had this thought as well, but check the |if (animate)| clause below I think that's currently using [self frame], not |frame| (so another reason not to have |frame| would be to avoid the shadowing :). https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm (right): https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:445: }; On 2017/01/04 17:36:39, lgrey wrote: > On 2017/01/04 06:08:10, tapted wrote: > > nit: DISALLOW_COPY_AND_ASSIGN(.. > > > > (you'll need a default constructor) > > Done, but this doesn't seem very common for CocoaProfileTest subclasses > (https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/extensions/choos..., > most of those hits are for helper classes.) Why do we need it here? (For my > info, not pushing back :)) http://go/cppguide#Copyable_Movable_Types says either to add proper support for copying/moving or to disable the the implicitly generated special functions that perform copies and moves. (In google3 the preference is to write out the macro, but in Chrome we still do DISALLOW_COPY_AND_ASSIGN) Of course you'll still get compile errors if you try to copy/assign since the parent class would have disabled the operators, but the errors are nicer, and it's good intrinsic documentation (and a lot of senior reviewers have always pointed it out to me if I forget to add the macro too :) https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.h (right): https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:205: // in response to frame changed/new window notifications. nit: wrap at 80 chars? (`git cl format` might even have learned this trick already) https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm (right): https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:433: // Test that subviews are ordered left to right nit: move this before the TEST_F line https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:448: DISALLOW_COPY_AND_ASSIGN(ToolbarControllerRTLTest); nit: blank line before https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:452: // Test that subviews are ordered right to left nit: move before TEST_F line
https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm (right): https://codereview.chromium.org/2607533004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm:297: NSRect newFrame; On 2017/01/04 22:57:50, tapted wrote: > On 2017/01/04 17:36:38, lgrey wrote: > > On 2017/01/04 06:08:09, tapted wrote: > > > |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; > > > > > > > > > > Had this thought as well, but check the |if (animate)| clause below > > I think that's currently using [self frame], not |frame| (so another reason not > to have |frame| would be to avoid the shadowing :). Done. https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.h (right): https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:205: // in response to frame changed/new window notifications. On 2017/01/04 22:57:50, tapted wrote: > nit: wrap at 80 chars? (`git cl format` might even have learned this trick > already) Done. https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm (right): https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:433: // Test that subviews are ordered left to right On 2017/01/04 22:57:50, tapted wrote: > nit: move this before the TEST_F line Done. https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:448: DISALLOW_COPY_AND_ASSIGN(ToolbarControllerRTLTest); On 2017/01/04 22:57:50, tapted wrote: > nit: blank line before Done. https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:452: // Test that subviews are ordered right to left On 2017/01/04 22:57:50, tapted wrote: > nit: move before TEST_F line > Done.
lgtm - thanks! https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm (right): https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm:231: return; hehe yeah - I was wondering about this. I thought about suggesting an early return on the first patchset, but saw some other data members changing. Of course the lines if (NSWidth(containerFrame) <= kMinimumContainerWidth) return; are now gone. I may have missed something, but I'm pretty sure that's redundant with the other checks. So this all looks fine.
Thanks, Trent! https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm (right): https://codereview.chromium.org/2607533004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_actions_container_view.mm:231: return; On 2017/01/05 23:09:36, tapted wrote: > hehe yeah - I was wondering about this. I thought about suggesting an early > return on the first patchset, but saw some other data members changing. > > Of course the lines > > if (NSWidth(containerFrame) <= kMinimumContainerWidth) > return; > > are now gone. I may have missed something, but I'm pretty sure that's redundant > with the other checks. So this all looks fine. What I ended up finding with this one is things would get screwed up for some reason that wasn't easily apparent to me. The one thing I can think of is that FLT_EPSILON is a valid delta at the rate this gets called!? Doesn't seem right to me, but since I added this in the first place, I figured it was fine to take back out. if (NSWidth(containerFrame) <= kMinimumContainerWidth) return; is basically redundant with clipping to kMinimumContainerWidth. I'm not sure why we would bail if it's equal though, so I guess this can be 1 pixel smaller than the previous version -- IMO, we should increase the minimum by 1 if that's a problem.
The CQ bit was checked by lgrey@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483703545130250,
"parent_rev": "5d044bb4fef05b493693b2d548352bc025f08d46", "commit_rev":
"8b0fd02afc2107ba123774e0a464a43407a108cf"}
Message was sent while issue was closed.
Description was changed from ========== [Mac] Flip toolbar in RTL Browser actions will be reordered in a future change. BUG=648558, 648563, 673362 ========== to ========== [Mac] Flip toolbar in RTL Browser actions will be reordered in a future change. BUG=648558, 648563, 673362 Review-Url: https://codereview.chromium.org/2607533004 Cr-Commit-Position: refs/heads/master@{#441925} Committed: https://chromium.googlesource.com/chromium/src/+/8b0fd02afc2107ba123774e0a464... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8b0fd02afc2107ba123774e0a464...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2622613002/ by lgrey@chromium.org. The reason for reverting is: Caused crbug.com/679249. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
