Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
399173004:
Adjust BrowserActionsContainer drag and drop to work for overflow (Closed)
DescriptionAdjust BrowserActionsContainer drag and drop to work for overflow
Adjust the drag and drop calculations for BrowserActionsContainer to work in
overflow mode. Now icons dragged to/from/within the overflow menu are placed
correctly.
BUG=393038
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286389
Patch Set 1 #
Total comments: 7
Patch Set 2 : Finnurs + RTL fix #
Total comments: 28
Patch Set 3 : Peter's #Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : Rebase #Patch Set 8 : Latest master #
Messages
Total messages: 16 (0 generated)
Another one. :) Between this one and https://codereview.chromium.org/399143004/, I think all the dragging works reasonably well. Cheers! https://codereview.chromium.org/399173004/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/399173004/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:380: int x = kItemSpacing + (index * IconWidth(true)) - We need this so that we can see the drop indicator to the left of the first icon in a row. https://codereview.chromium.org/399173004/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:485: std::min(std::max(before_icon_unclamped, 0), visible_icons_on_row); These calculations plus using a row/icon_on_row struct have the nice trait of "just working" in funny edge cases, like how dropping an icon to the right of the last icon in row 1 is actionably the same as dropping it to the left of the first icon in row 2, but needs to be displayed differently with the drop indicator. https://codereview.chromium.org/399173004/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc (left): https://codereview.chromium.org/399173004/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:18: const int kVerticalPadding = 8; I think this looks fine without this, since the badge issue was fixed.
Woot! https://codereview.chromium.org/399173004/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/399173004/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:434: browser_action_views_[GetFirstVisibleIconIndex()]->x(); Will this work in RTL? https://codereview.chromium.org/399173004/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:760: // chosen icon. Nit: "Chosen icon" is a bit weird here, because you are choosing an empty space between two icons, or in the case of edges (before/after the first/last icon).
https://codereview.chromium.org/399173004/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/399173004/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:434: browser_action_views_[GetFirstVisibleIconIndex()]->x(); On 2014/07/18 10:52:41, Finnur wrote: > Will this work in RTL? Nope! But it does now. :) https://codereview.chromium.org/399173004/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:760: // chosen icon. On 2014/07/18 10:52:41, Finnur wrote: > Nit: "Chosen icon" is a bit weird here, because you are choosing an empty space > between two icons, or in the case of edges (before/after the first/last icon). Done.
Peter, mind taking a look?
https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:103: bool BrowserActionsContainer::disable_animations_during_testing_ = false; Nit: Put this below the "BrowserActionsContainer" divider comment below. https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:105: struct BrowserActionsContainer::DropPosition { Nit: Consider a "// BrowserActionsContainer::DropPosition ---..." or similar divider about this class (up to you) https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:107: ~DropPosition(); Nit: Is declaring this destructor necessary? https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:380: int x = kItemSpacing + (index * IconWidth(true)) - So, we have kItemSpacing to the left of all the icons, but not between each pair of icons? https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:439: (base::i18n::IsRTL() ? width() - event.x() : event.x()) - Nit: Use "GetMirroredXInView(event.x())"; indent 2, not 4 https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:780: // draws to far to the right). Nit: to far -> too far This logic doesn't really make sense to me, though. This is mathematically equivalent to just having not inserted the "kItemSpacing / 2" in the calculation above (when we're RTL). It seems like if there is a rounding issue here, we'd want to shift things further right by 1 px in RTL, not further left. Maybe you can give some screnshots to illustrate the issue here? https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:782: drop_indicator_x = width() - drop_indicator_x - 1; Nit: MirroredXInView() https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1014: void BrowserActionsContainer::SetDropPosition(size_t row, size_t icon_in_row) { Nit: This is only called once. It might be clearer to do this in the caller. If you use my suggestion about storing this member by value instead of pointer (see comments in .h), we could do something like: const DropPosition old_pos(drop_position_); drop_position_ = DropPosition(row, icon_in_row); if (drop_position_ != old_pos) SchedulePaint(); https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1023: void BrowserActionsContainer::ResetDropPosition() { I'm not convinced having this is preferable to just calling reset() directly inline in the callers. https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:445: scoped_ptr<DropPosition> drop_position_; I wonder if instead of using NULL to mean "none", this should be a member-by-value and there should be a "no position" drop position value (much like npos works for std::string). https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc (right): https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:47: gfx::Size sz = container_->GetPreferredSize(); Nit: Can just be GetPreferredSize()
https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:103: bool BrowserActionsContainer::disable_animations_during_testing_ = false; On 2014/07/21 20:24:08, Peter Kasting wrote: > Nit: Put this below the "BrowserActionsContainer" divider comment below. Done. https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:105: struct BrowserActionsContainer::DropPosition { On 2014/07/21 20:24:08, Peter Kasting wrote: > Nit: Consider a "// BrowserActionsContainer::DropPosition ---..." or similar > divider about this class (up to you) SGTM. Done. https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:107: ~DropPosition(); On 2014/07/21 20:24:08, Peter Kasting wrote: > Nit: Is declaring this destructor necessary? For some reason, I thought it was chrome/google style. But I can't find a rule indicating it is, so taken out. https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:380: int x = kItemSpacing + (index * IconWidth(true)) - On 2014/07/21 20:24:08, Peter Kasting wrote: > So, we have kItemSpacing to the left of all the icons, but not between each pair > of icons? We do have it between each pair of icons, but IconWidth() is somewhat unfortunately done. The bool passed in indicates "include padding", which, if true, returns the width of the icon + kItemSpacing. The extra kItemSpacing here is to account for the spacing before each icon (e.g., for the 0th index, we still have 1 spacing, for the 1st index, we have 2 spacing, etc). https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:439: (base::i18n::IsRTL() ? width() - event.x() : event.x()) - On 2014/07/21 20:24:08, Peter Kasting wrote: > Nit: Use "GetMirroredXInView(event.x())"; indent 2, not 4 Done. https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:780: // draws to far to the right). On 2014/07/21 20:24:08, Peter Kasting wrote: > Nit: to far -> too far > > This logic doesn't really make sense to me, though. This is mathematically > equivalent to just having not inserted the "kItemSpacing / 2" in the calculation > above (when we're RTL). It seems like if there is a rounding issue here, we'd > want to shift things further right by 1 px in RTL, not further left. > > Maybe you can give some screnshots to illustrate the issue here? Yeah, it's confusing. :) So, in LTR, we shift things "closer to the icon" (because we take kItemSpacing / 2 = 3 / 2 = 1, so we only move one pixel away from the icon). When we mirror this to RTL, to get the same effect, "closer to the icon" requires an offset of 2 (because it's mirrored), so we basically want kItemSpacing / 2 = 2. Since we subtract kItemSpacing, to make up the difference, we subtract 1. And, of course, screenshots are worth 1000 words. http://imgur.com/PtFGQ4f The RTL is most apparently wrong without the -1 when dragging to the far right (i.e., closer to the address bar) as the drop indicator actually gets cut off. https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:782: drop_indicator_x = width() - drop_indicator_x - 1; On 2014/07/21 20:24:08, Peter Kasting wrote: > Nit: MirroredXInView() Done. https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1014: void BrowserActionsContainer::SetDropPosition(size_t row, size_t icon_in_row) { On 2014/07/21 20:24:08, Peter Kasting wrote: > Nit: This is only called once. It might be clearer to do this in the caller. > > If you use my suggestion about storing this member by value instead of pointer > (see comments in .h), we could do something like: > > const DropPosition old_pos(drop_position_); > drop_position_ = DropPosition(row, icon_in_row); > if (drop_position_ != old_pos) > SchedulePaint(); Inlined. See comment in the .h about the rest. https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1023: void BrowserActionsContainer::ResetDropPosition() { On 2014/07/21 20:24:08, Peter Kasting wrote: > I'm not convinced having this is preferable to just calling reset() directly > inline in the callers. Agreed - I put this in for parity with SetDropPosition, but it's really not needed (especially if we inline SetDropPosition()). https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:445: scoped_ptr<DropPosition> drop_position_; On 2014/07/21 20:24:09, Peter Kasting wrote: > I wonder if instead of using NULL to mean "none", this should be a > member-by-value and there should be a "no position" drop position value (much > like npos works for std::string). I personally prefer the scoped ptr approach, because it easily allows us to just have a forward declaration for the struct. Plus, it's a bit more code and somewhat odd to do an npos style thing, because it's not just POD so we'd have to have a static method, like: DropPosition { static DropPosition invalid() { return DropPosition(-1, -1); } }; We also then need to have allow things like == comparison and copying (which we allow for structs, but generally don't encourage unless necessary). Is there a deeper reason to go for the alternative? I'm happy to change it if so, just wanna make sure it's worth it. :) https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc (right): https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:47: gfx::Size sz = container_->GetPreferredSize(); On 2014/07/21 20:24:09, Peter Kasting wrote: > Nit: Can just be GetPreferredSize() Done.
https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:780: // draws to far to the right). On 2014/07/22 17:30:46, Devlin wrote: > On 2014/07/21 20:24:08, Peter Kasting wrote: > > Nit: to far -> too far > > > > This logic doesn't really make sense to me, though. This is mathematically > > equivalent to just having not inserted the "kItemSpacing / 2" in the > calculation > > above (when we're RTL). It seems like if there is a rounding issue here, we'd > > want to shift things further right by 1 px in RTL, not further left. > > > > Maybe you can give some screnshots to illustrate the issue here? > > Yeah, it's confusing. :) So, in LTR, we shift things "closer to the icon" > (because we take kItemSpacing / 2 = 3 / 2 = 1, so we only move one pixel away > from the icon). When we mirror this to RTL, to get the same effect, "closer to > the icon" requires an offset of 2 (because it's mirrored), so we basically want > kItemSpacing / 2 = 2. Since we subtract kItemSpacing, to make up the > difference, we subtract 1. > > And, of course, screenshots are worth 1000 words. > http://imgur.com/PtFGQ4f > The RTL is most apparently wrong without the -1 when dragging to the far right > (i.e., closer to the address bar) as the drop indicator actually gets cut off. The screenshots are pretty clear that what you're doing makes sense. It still boggles my mind, though. When you say "Since we subtract kItemSpacing, to make up the difference, we subtract 1", my response is, "But we subtract that value before we mirror everything -- and mirroring flips all the signs. So effectively, after we mirror, we've 'added' kItemSpacing/2, and now we're undoing that, rather than adding to it." Another way of saying this is, if you replaced "kItemSpacing / 2" with "2", eliminated the RTL-specific subtraction, and then ran in RTL mode, I'd expect you to get a different result than your current code. If I'm right, it suggests your justification here is a bit wonky, and you're not really trying to subtract "kItemSpacing / 2" in the first place, you really just have an LTR-specific "-1" for some reason. Consider testing the above theory and, if it's correct, thinking more deeply about how the code and comments here should read. https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:445: scoped_ptr<DropPosition> drop_position_; On 2014/07/22 17:30:47, Devlin wrote: > On 2014/07/21 20:24:09, Peter Kasting wrote: > > I wonder if instead of using NULL to mean "none", this should be a > > member-by-value and there should be a "no position" drop position value (much > > like npos works for std::string). > > I personally prefer the scoped ptr approach, because it easily allows us to just > have a forward declaration for the struct. Plus, it's a bit more code and > somewhat odd to do an npos style thing, because it's not just POD so we'd have > to have a static method, like: > DropPosition { > static DropPosition invalid() { return DropPosition(-1, -1); } > }; > > We also then need to have allow things like == comparison and copying (which we > allow for structs, but generally don't encourage unless necessary). > > Is there a deeper reason to go for the alternative? I'm happy to change it if > so, just wanna make sure it's worth it. :) Let me back up for a second. You start by saying this isn't a POD struct. But as long as you aren't defining a destructor, copy/assignment operators, etc. -- which you aren't now -- I believe this struct _is_ POD. That means you don't need any sort of invalid() definition, and you don't need to write operator==() (the compiler will do it for you). You can simply do something like this: namespace { const DropPosition kInvalidPos(static_cast<size_t>(-1), static_cast<size_t>(-1)); } ... if (position != kInvalidPos) ... (As long as DropPosition is POD, we're allowed to static-initialize it per the style guide.) Assuming all that is true, I think this code is actually simpler and clearer than the current code, and the only downside is that you have to declare the struct in the .h file (but the style guide explicitly says not to use pointer members in a class just to avoid having to declare other classes in a header). I'm curious whether you agree.
https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:780: // draws to far to the right). On 2014/07/22 18:21:11, Peter Kasting wrote: > On 2014/07/22 17:30:46, Devlin wrote: > > On 2014/07/21 20:24:08, Peter Kasting wrote: > > > Nit: to far -> too far > > > > > > This logic doesn't really make sense to me, though. This is mathematically > > > equivalent to just having not inserted the "kItemSpacing / 2" in the > > calculation > > > above (when we're RTL). It seems like if there is a rounding issue here, > we'd > > > want to shift things further right by 1 px in RTL, not further left. > > > > > > Maybe you can give some screnshots to illustrate the issue here? > > > > Yeah, it's confusing. :) So, in LTR, we shift things "closer to the icon" > > (because we take kItemSpacing / 2 = 3 / 2 = 1, so we only move one pixel away > > from the icon). When we mirror this to RTL, to get the same effect, "closer > to > > the icon" requires an offset of 2 (because it's mirrored), so we basically > want > > kItemSpacing / 2 = 2. Since we subtract kItemSpacing, to make up the > > difference, we subtract 1. > > > > And, of course, screenshots are worth 1000 words. > > http://imgur.com/PtFGQ4f > > The RTL is most apparently wrong without the -1 when dragging to the far right > > (i.e., closer to the address bar) as the drop indicator actually gets cut off. > > The screenshots are pretty clear that what you're doing makes sense. > > It still boggles my mind, though. When you say "Since we subtract kItemSpacing, > to make up the difference, we subtract 1", my response is, "But we subtract that > value before we mirror everything -- and mirroring flips all the signs. So > effectively, after we mirror, we've 'added' kItemSpacing/2, and now we're > undoing that, rather than adding to it." > > Another way of saying this is, if you replaced "kItemSpacing / 2" with "2", > eliminated the RTL-specific subtraction, and then ran in RTL mode, I'd expect > you to get a different result than your current code. If I'm right, it suggests > your justification here is a bit wonky, and you're not really trying to subtract > "kItemSpacing / 2" in the first place, you really just have an LTR-specific "-1" > for some reason. > > Consider testing the above theory and, if it's correct, thinking more deeply > about how the code and comments here should read. Ah! You're quite right, there's a couple things wrong here (even in the old code). We should not be subtracting kDropIndicatorWidth / 2, but rather kDropIndicatorWidth, as this is used to set the bounds (which need the starting x, not the center). We add in an extra pixel, because kItemSpacing is odd, and it looks better a bit closer to the icon and farther from the address bar. Then, we *do* need an RTL correction, because FillRect doesn't use RTL bounds (hence why we mirror in the first place). Because of this, it fills LTR, and we actually need to shift the bounds of the rect by kDropIndicatorWidth to compensate. https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:445: scoped_ptr<DropPosition> drop_position_; On 2014/07/22 18:21:11, Peter Kasting wrote: > On 2014/07/22 17:30:47, Devlin wrote: > > On 2014/07/21 20:24:09, Peter Kasting wrote: > > > I wonder if instead of using NULL to mean "none", this should be a > > > member-by-value and there should be a "no position" drop position value > (much > > > like npos works for std::string). > > > > I personally prefer the scoped ptr approach, because it easily allows us to > just > > have a forward declaration for the struct. Plus, it's a bit more code and > > somewhat odd to do an npos style thing, because it's not just POD so we'd have > > to have a static method, like: > > DropPosition { > > static DropPosition invalid() { return DropPosition(-1, -1); } > > }; > > > > We also then need to have allow things like == comparison and copying (which > we > > allow for structs, but generally don't encourage unless necessary). > > > > Is there a deeper reason to go for the alternative? I'm happy to change it if > > so, just wanna make sure it's worth it. :) > > Let me back up for a second. You start by saying this isn't a POD struct. But > as long as you aren't defining a destructor, copy/assignment operators, etc. -- > which you aren't now -- I believe this struct _is_ POD. That means you don't > need any sort of invalid() definition, and you don't need to write operator==() > (the compiler will do it for you). You can simply do something like this: > > namespace { > const DropPosition kInvalidPos(static_cast<size_t>(-1), > static_cast<size_t>(-1)); > } > ... > if (position != kInvalidPos) ... > > (As long as DropPosition is POD, we're allowed to static-initialize it per the > style guide.) > > Assuming all that is true, I think this code is actually simpler and clearer > than the current code, and the only downside is that you have to declare the > struct in the .h file (but the style guide explicitly says not to use pointer > members in a class just to avoid having to declare other classes in a header). > I'm curious whether you agree. But, if we define a constructor (default or otherwise), the struct is no longer POD (pre-C++11), and we can't statically initialize it. We could omit the constructor, but then we have a somewhat-awkward requirement to initialize the struct "by hand" everywhere - which is more code, and more likely to break IMHO. Thoughts?
https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:445: scoped_ptr<DropPosition> drop_position_; On 2014/07/22 20:29:33, Devlin wrote: > But, if we define a constructor (default or otherwise), the struct is no longer > POD (pre-C++11), Oof, I didn't recall that. It's funny too, we're about a week away from hopefully being able to start using C++11 language features. > We could omit the > constructor, but then we have a somewhat-awkward requirement to initialize the > struct "by hand" everywhere Well, you can use aggregate initialization, right? DropPosition drop_position = { x, y }; If so, this still doesn't seem _too_ bad -- but I'll leave it up to you about what the overall right way is. Either solution is OK by me. https://codereview.chromium.org/399173004/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/399173004/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:774: // plus 1 because it kItemSpacing is odd and the indicator looks Nit: remove "it" https://codereview.chromium.org/399173004/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:780: kItemSpacing / 2 + 1 - It still doesn't look to me like this is doing what you're saying. "... - kItemSpacing / 2 + 1 ..." --> "... - 3 / 2 + 1 ..." --> "... - 1 + 1 ..." --> "... <does nothing> ..." It sounds from the comment like you were trying to describe something more like "(kItemSpacing + 1) / 2", which will always mean "half, rounding up" no matter whether kItemSpacing is even or odd. Here's what I think you really want. This should do The Right Thing no matter what the size of kItemSpacing and kDropIndicatorWidth are. // Convert back to a pixel offset into the container. First find the X // coordinate of the drop icon. int drop_icon_x = browser_action_views_[GetFirstVisibleIconIndex()]->x() + (drop_position_->icon_in_row * IconWidth(true)); // Now place the drop indicator halfway between this and the end of the // previous icon. If there is an odd amount of available space between the // two icons after subtracting the drop indicator width, this calculation // puts the extra pixel on the left side of the indicator, since when the // indicator is between the address bar and the first icon, it looks better // closer to the icon. int drop_indicator_x = drop_icon_x - ((kItemSpacing + kDropIndicatorWidth) / 2); int row_height = IconHeight(); int drop_indicator_y = row_height * drop_position_->row; gfx::Rect indicator_bounds(drop_indicator_x, drop_indicator_y, kDropIndicatorWidth, row_height); indicator_bounds.set_x(GetMirroredXForRect(indicator_bounds)); Adding the item spacing and the drop indicator width together before dividing handles any combination of even and odd widths correctly. Note that the GetMirroredXForRect() call at the end takes care of adjusting by the indicator width for you in RTL.
https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/399173004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:445: scoped_ptr<DropPosition> drop_position_; On 2014/07/22 21:10:00, Peter Kasting wrote: > On 2014/07/22 20:29:33, Devlin wrote: > > But, if we define a constructor (default or otherwise), the struct is no > longer > > POD (pre-C++11), > > Oof, I didn't recall that. > > It's funny too, we're about a week away from hopefully being able to start using > C++11 language features. > > > We could omit the > > constructor, but then we have a somewhat-awkward requirement to initialize the > > struct "by hand" everywhere > > Well, you can use aggregate initialization, right? > > DropPosition drop_position = { x, y }; > > If so, this still doesn't seem _too_ bad -- but I'll leave it up to you about > what the overall right way is. Either solution is OK by me. Aggregate initialization isn't too bad, but we can't do it in the constructor of BrowserActionsContainer (i.e., BAC::BAC() : drop_position_({x, y}) isn't allowed), so we'd still be left with having to initialize by hand there. Overall... I think this is cleaner. We'll revisit in a bit once C++11 is more fully allowed. :) https://codereview.chromium.org/399173004/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/399173004/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:774: // plus 1 because it kItemSpacing is odd and the indicator looks On 2014/07/22 21:10:00, Peter Kasting wrote: > Nit: remove "it" Moot. https://codereview.chromium.org/399173004/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:780: kItemSpacing / 2 + 1 - On 2014/07/22 21:10:00, Peter Kasting wrote: > It still doesn't look to me like this is doing what you're saying. > > "... - kItemSpacing / 2 + 1 ..." > --> "... - 3 / 2 + 1 ..." > --> "... - 1 + 1 ..." > --> "... <does nothing> ..." > > It sounds from the comment like you were trying to describe something more like > "(kItemSpacing + 1) / 2", which will always mean "half, rounding up" no matter > whether kItemSpacing is even or odd. > > Here's what I think you really want. This should do The Right Thing no matter > what the size of kItemSpacing and kDropIndicatorWidth are. > > // Convert back to a pixel offset into the container. First find the X > // coordinate of the drop icon. > int drop_icon_x = browser_action_views_[GetFirstVisibleIconIndex()]->x() + > (drop_position_->icon_in_row * IconWidth(true)); > // Now place the drop indicator halfway between this and the end of the > // previous icon. If there is an odd amount of available space between the > // two icons after subtracting the drop indicator width, this calculation > // puts the extra pixel on the left side of the indicator, since when the > // indicator is between the address bar and the first icon, it looks better > // closer to the icon. > int drop_indicator_x = drop_icon_x - > ((kItemSpacing + kDropIndicatorWidth) / 2); > int row_height = IconHeight(); > int drop_indicator_y = row_height * drop_position_->row; > gfx::Rect indicator_bounds(drop_indicator_x, drop_indicator_y, > kDropIndicatorWidth, row_height); > indicator_bounds.set_x(GetMirroredXForRect(indicator_bounds)); > > Adding the item spacing and the drop indicator width together before dividing > handles any combination of even and odd widths correctly. Note that the > GetMirroredXForRect() call at the end takes care of adjusting by the indicator > width for you in RTL. 1. Wow, that's much more articulate. Let's use that. :) 2. If we're gonna do The Right Thing independent of actual values, we also need to check if we should use ToolbarView::kStandardSpacing vs kItemSpacing (see added step). 3. Thanks for the pointer to GetMirroredXForRect(). Still new to views and learning where everything is...
LGTM https://codereview.chromium.org/399173004/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/399173004/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:771: // Next, find the space before the previous icon. This will either be Nit: I think you either mean to say "the space after the previous icon" or "the space before this icon" or "the space between the previous item and this one". You'll also want to adjust the name of the temp.
Thanks for the attentiveness, Peter! https://codereview.chromium.org/399173004/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/399173004/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:771: // Next, find the space before the previous icon. This will either be On 2014/07/22 22:17:03, Peter Kasting wrote: > Nit: I think you either mean to say "the space after the previous icon" or "the > space before this icon" or "the space between the previous item and this one". > > You'll also want to adjust the name of the temp. d'oh. Done.
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/399173004/...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
Message was sent while issue was closed.
Change committed as 286389 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
