Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
553753003:
Rework app list item drag zones. (Closed)
DescriptionRework app list item drag zones.
This CL changes the app list item drag zones to use the app icons as
folder creation zones and the space between icons as reorder zones.
The main difference from the previous implementation is that the space
between icons no longer is a drop zone of both adjacent tiles.
These new drag zones attempt to allow the user to move apps into the
gaps between icons which will then create a space for it by moving other
apps out of the way.
The non-folder drag has been unified with the folder drag, causing the
non-folder drag to lose the ability to drop onto the page switcher.
BUG=411775
Committed: https://crrev.com/df474d8b2e9a64b4dad10512fbadee6169ea077e
Cr-Commit-Position: refs/heads/master@{#294811}
Patch Set 1 : #Patch Set 2 : #
Total comments: 52
Patch Set 3 : address comments #
Total comments: 11
Patch Set 4 : address comments #Patch Set 5 : fix test #
Messages
Total messages: 20 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
calamity@chromium.org changed reviewers: + tapted@chromium.org
https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... File ui/app_list/views/apps_grid_view.cc (left): https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:2045: CanDropIntoTarget(folder_drop_target_); I don't think this belongs here. I'd always assumed `EnableFolderDragDropUI()` doesn't change its return value after the AppsGridView is constructed. What breaks without this? https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:485: reorder_placeholder_ = GetIndexOfView(drag_view_); nit: GetIndexOfView(drag_view_); -> drag_view_init_index_ https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:782: // Since the item is new, it's placeholder is conceptually at the back of the nit: `it's` -> `its` https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1119: int view_index = view_model_.view_size() - 1; DCHECK(!view_model_.empty()); .. assuming we can guarantee that https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1394: const gfx::Point& drag_point, |drag_point| looks unused now -> remove? https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1402: reorder_drop_target_ = GetIndexOfView(drag_view_); maybe.. drag_view_init_index_ https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1415: } else { early return before this. no else. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1416: DCHECK(drag_view_); move this to the start of the function https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1417: const int d_folder_dropping = d looks like an abbreviation - maybe `folder_threshold`. But this probably needs a comment too about why it's calculated this way (e.g. is it coincidence that half the icon dimension gives a good result, or does it need to be that way) https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1429: CanDropIntoTarget(nearest_tile_index)) { CanDropIntoTarget returns true if the drop_target is past the end (I think) - is that what we want here? https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1432: return; there's a lot of state above that isn't used for the rest of the function. I think this needs some decomposition. Something like if (IsFolderDrop(point, &folder_drop_target_)) { drop_attempt_ = DROP_FOR_FOLDER; return; } bool AppsGridView::IsFolderDrop(const gfx::Point& point, Index* drop_target) const { const int folder_threshold = kFolderDroppingCircleRadius + kGridIconDimension / 2; Index nearest_tile_index(pagination_model_.selected_page(), GetNearestTileSlotForPoint(point)); int distance_to_tile_center = (point - GetExpectedTileBounds(nearest_tile_index.slot).CenterPoint()) .Length(); if (nearest_tile_index != reorder_placeholder_ && distance_to_tile_center < d_folder_dropping && !IsFolderItem(drag_view_->item()) && CanDropIntoTarget(nearest_tile_index)) { *drop_target = nearest_tile_index; return true; } return false; } https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1442: std::min(row * cols_ + grid_col, view_model_.view_size())); This is pretty fiddly... but I don't see an obvious improvement. Maybe you can re-use GetNearestTileSlotForPoint(point)) here and ignore the clamping until the end of the function. But the big question here is: what about folders-disabled? I think AppsGridView::CalculateDropTarget() needs an update too. Probably this last bit needs to be a function like AppsGridView::CalculateReorderDropTarget(), which is also called by CalculateDropTarget https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1446: int dir = 0; dir isn't a good name - maybe column_offset? https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1453: // Offset the target column base on the direction of the target. This will nit: base->based https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:2121: gfx::Rect bounds = GetContentsBounds(); maybe DCHECK(IsPointWithinDragBuffer(point)); before this? Otherwise, I have to think harder about whether it makes sense https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... File ui/app_list/views/apps_grid_view.h (right): https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.h:239: if (page == other.page) optional-nit: This is fine, but I usually write these like if (page != other.page) return page < other.page; return slot < other.slot; it means if you have more than 2 axes you don't have to nest things, and you don't have to mix axes in each condition/statement. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.h:275: // Gets the index of the AppListItemView in the view model. `in the view model.` -> `at the end of the view model.` https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.h:405: // Returns the slot number which the given |point| falls into. nit: This comment suggests to me that if |point| is not over a slot it will return -1 or something. Still reading through the CL. I came back: could this return an AppsGridView::Index with page == current page? - might neaten things up a bit https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.h:406: int GetNearestTileSlotForPoint(const gfx::Point& point); nit: declare const? https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.h:409: gfx::Rect GetExpectedTileBounds(int slot) const; maybe take an AppsGridView::Index ? looks like it's always called with index.slot https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.h:411: // Gets the bounds of the tile located at |row| and |col| on current page. nit: on -> on the https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.h:519: Index reorder_placeholder_; Can you say more about this in the comment - I'm not sure why the placeholder is needed. And, e.g., how is this different to drag_view_init_index_? https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... File ui/app_list/views/apps_grid_view_unittest.cc (right): https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view_unittest.cc:440: } I think you need to add more here - at least to cover some of the possibilities of `dir`
Patchset #3 (id:100001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... File ui/app_list/views/apps_grid_view.cc (left): https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:2045: CanDropIntoTarget(folder_drop_target_); On 2014/09/11 08:08:54, tapted wrote: > I don't think this belongs here. I'd always assumed `EnableFolderDragDropUI()` > doesn't change its return value after the AppsGridView is constructed. What > breaks without this? Nothing afaict. It was obviously needed for the "can this be dropped?" case which is at line 1429 but that now uses CanDropIntoTarget directly. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:485: reorder_placeholder_ = GetIndexOfView(drag_view_); On 2014/09/11 08:08:54, tapted wrote: > nit: GetIndexOfView(drag_view_); -> drag_view_init_index_ Done. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:782: // Since the item is new, it's placeholder is conceptually at the back of the On 2014/09/11 08:08:54, tapted wrote: > nit: `it's` -> `its` Done. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1119: int view_index = view_model_.view_size() - 1; On 2014/09/11 08:08:53, tapted wrote: > DCHECK(!view_model_.empty()); .. assuming we can guarantee that It'll be a shoddy guarantee.. (A component app should always exist) https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1394: const gfx::Point& drag_point, On 2014/09/11 08:08:53, tapted wrote: > |drag_point| looks unused now -> remove? Done. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1402: reorder_drop_target_ = GetIndexOfView(drag_view_); On 2014/09/11 08:08:53, tapted wrote: > maybe.. drag_view_init_index_ Done. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1415: } else { On 2014/09/11 08:08:54, tapted wrote: > early return before this. no else. Done. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1416: DCHECK(drag_view_); On 2014/09/11 08:08:54, tapted wrote: > move this to the start of the function Done. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1417: const int d_folder_dropping = On 2014/09/11 08:08:55, tapted wrote: > d looks like an abbreviation - maybe `folder_threshold`. But this probably needs > a comment too about why it's calculated this way (e.g. is it coincidence that > half the icon dimension gives a good result, or does it need to be that way) I think this is saying the radius is the distance to the edge of the icon + 15... Maybe kFolderDroppingCircleRadius should just be the folder dropping circle radius.. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1429: CanDropIntoTarget(nearest_tile_index)) { On 2014/09/11 08:08:54, tapted wrote: > CanDropIntoTarget returns true if the drop_target is past the end (I think) - is > that what we want here? Nope. Fixed. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1432: return; On 2014/09/11 08:08:53, tapted wrote: > there's a lot of state above that isn't used for the rest of the function. I > think this needs some decomposition. Something like > > if (IsFolderDrop(point, &folder_drop_target_)) { > drop_attempt_ = DROP_FOR_FOLDER; > return; > } > > bool AppsGridView::IsFolderDrop(const gfx::Point& point, > Index* drop_target) const { > const int folder_threshold = > kFolderDroppingCircleRadius + kGridIconDimension / 2; > Index nearest_tile_index(pagination_model_.selected_page(), > GetNearestTileSlotForPoint(point)); > int distance_to_tile_center = > (point - GetExpectedTileBounds(nearest_tile_index.slot).CenterPoint()) > .Length(); > if (nearest_tile_index != reorder_placeholder_ && > distance_to_tile_center < d_folder_dropping && > !IsFolderItem(drag_view_->item()) && > CanDropIntoTarget(nearest_tile_index)) { > *drop_target = nearest_tile_index; > return true; > } > return false; > } Done. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1442: std::min(row * cols_ + grid_col, view_model_.view_size())); On 2014/09/11 08:08:54, tapted wrote: > This is pretty fiddly... but I don't see an obvious improvement. Maybe you can > re-use GetNearestTileSlotForPoint(point)) here and ignore the clamping until the > end of the function. > Done. > But the big question here is: what about folders-disabled? I think > AppsGridView::CalculateDropTarget() needs an update too. Probably this last bit > needs to be a function like AppsGridView::CalculateReorderDropTarget(), which is > also called by CalculateDropTarget Done. This removes some functionality. Updated the CL description. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1446: int dir = 0; On 2014/09/11 08:08:54, tapted wrote: > dir isn't a good name - maybe column_offset? Done. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1453: // Offset the target column base on the direction of the target. This will On 2014/09/11 08:08:54, tapted wrote: > nit: base->based Done. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:2121: gfx::Rect bounds = GetContentsBounds(); On 2014/09/11 08:08:54, tapted wrote: > maybe DCHECK(IsPointWithinDragBuffer(point)); before this? Otherwise, I have to > think harder about whether it makes sense Hmm. Unfortunately IsPointWithinDragBuffer includes a 15px buffer around the drag view which threatens to ruin this. How about this? https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... File ui/app_list/views/apps_grid_view.h (right): https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.h:239: if (page == other.page) On 2014/09/11 08:08:55, tapted wrote: > optional-nit: This is fine, but I usually write these like > > if (page != other.page) > return page < other.page; > > return slot < other.slot; > > > it means if you have more than 2 axes you don't have to nest things, and you > don't have to mix axes in each condition/statement. Done. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.h:275: // Gets the index of the AppListItemView in the view model. On 2014/09/11 08:08:55, tapted wrote: > `in the view model.` -> `at the end of the view model.` Done. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.h:405: // Returns the slot number which the given |point| falls into. On 2014/09/11 08:08:55, tapted wrote: > nit: This comment suggests to me that if |point| is not over a slot it will > return -1 or something. Still reading through the CL. I came back: could this > return an AppsGridView::Index with page == current page? - might neaten things > up a bit Returns an Index, changed description. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.h:406: int GetNearestTileSlotForPoint(const gfx::Point& point); On 2014/09/11 08:08:55, tapted wrote: > nit: declare const? Done. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.h:409: gfx::Rect GetExpectedTileBounds(int slot) const; On 2014/09/11 08:08:55, tapted wrote: > maybe take an AppsGridView::Index ? looks like it's always called with > index.slot I think slot is better since using an Index kind of implies that this will consider the page when calculating the bounds. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.h:411: // Gets the bounds of the tile located at |row| and |col| on current page. On 2014/09/11 08:08:55, tapted wrote: > nit: on -> on the Done. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.h:519: Index reorder_placeholder_; On 2014/09/11 08:08:55, tapted wrote: > Can you say more about this in the comment - I'm not sure why the placeholder is > needed. And, e.g., how is this different to drag_view_init_index_? Done. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... File ui/app_list/views/apps_grid_view_unittest.cc (right): https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view_unittest.cc:440: } On 2014/09/11 08:08:55, tapted wrote: > I think you need to add more here - at least to cover some of the possibilities > of `dir` Added a couple. Any other cases you can think of that are worth testing?
lgtm % nits and making-more-member-functions-const https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:485: reorder_placeholder_ = GetIndexOfView(drag_view_); On 2014/09/15 03:26:10, calamity wrote: > On 2014/09/11 08:08:54, tapted wrote: > > nit: GetIndexOfView(drag_view_); -> drag_view_init_index_ > > Done. missed? https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1119: int view_index = view_model_.view_size() - 1; On 2014/09/15 03:26:10, calamity wrote: > On 2014/09/11 08:08:53, tapted wrote: > > DCHECK(!view_model_.empty()); .. assuming we can guarantee that > > It'll be a shoddy guarantee.. (A component app should always exist) Yep - that's all i'm after really. Since if it is empty and things go negative it's more thinking here. https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:1417: const int d_folder_dropping = On 2014/09/15 03:26:09, calamity wrote: > On 2014/09/11 08:08:55, tapted wrote: > > d looks like an abbreviation - maybe `folder_threshold`. But this probably > needs > > a comment too about why it's calculated this way (e.g. is it coincidence that > > half the icon dimension gives a good result, or does it need to be that way) > > I think this is saying the radius is the distance to the edge of the icon + > 15... > > Maybe kFolderDroppingCircleRadius should just be the folder dropping circle > radius.. yep - much nicer https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:2121: gfx::Rect bounds = GetContentsBounds(); On 2014/09/15 03:26:09, calamity wrote: > On 2014/09/11 08:08:54, tapted wrote: > > maybe DCHECK(IsPointWithinDragBuffer(point)); before this? Otherwise, I have > to > > think harder about whether it makes sense > > Hmm. Unfortunately IsPointWithinDragBuffer includes a 15px buffer around the > drag view which threatens to ruin this. > > How about this? sweet https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... File ui/app_list/views/apps_grid_view_unittest.cc (right): https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view_unittest.cc:440: } On 2014/09/15 03:26:11, calamity wrote: > On 2014/09/11 08:08:55, tapted wrote: > > I think you need to add more here - at least to cover some of the > possibilities > > of `dir` > > Added a couple. Any other cases you can think of that are worth testing? That's good i think - I think you've got most of the corner cases https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... ui/app_list/views/apps_grid_view.cc:1365: } neat! https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... ui/app_list/views/apps_grid_view.cc:1368: Index* drop_target) { It would be nice if these functions were const. But you'll need to sweep through and ensure GetExpectedTileBounds, CanDropIntoTarget, etc. are all const too. There might be a dealbreaker along the way though, in which case (if it's not const) the drop_target argument is probably unwarranted. (I find it pretty readable with that though, so up to you if you want to keep it) rationale: AppsGridView is pretty epic and hard to reason about. `const` will help a bit. Basically all the Can* and Get* functions should be const. Also at the back of my mind is that all the DragState-related stuff could move to some kind of `DragHelper` class (the `ResetDragState` function is kindof a giveaway that such a thing should exist to encapsulate that). But that's all a bit orthogonal. https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... ui/app_list/views/apps_grid_view.cc:1407: int column_offset = column_offset_direction * nit: maybe a _px or _pixels suffix? Or even just `x_offset`. currently seems a bit too much like it should be offsetting the column index. https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... ui/app_list/views/apps_grid_view.cc:1413: GetLastViewIndex()); nice https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... File ui/app_list/views/apps_grid_view.h (right): https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... ui/app_list/views/apps_grid_view.h:312: // Same as CalculateDropTarget, but with folder UI enabled. The drop target I think this comment is a stray now https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... ui/app_list/views/apps_grid_view.h:409: // slot if |point| if outside the page's bounds. nit: if outside -> is outside
https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/553753003/diff/80001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:485: reorder_placeholder_ = GetIndexOfView(drag_view_); On 2014/09/15 06:12:53, tapted wrote: > On 2014/09/15 03:26:10, calamity wrote: > > On 2014/09/11 08:08:54, tapted wrote: > > > nit: GetIndexOfView(drag_view_); -> drag_view_init_index_ > > > > Done. > > missed? Done for real. https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... ui/app_list/views/apps_grid_view.cc:1365: } On 2014/09/15 06:12:53, tapted wrote: > neat! Acknowledged. https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... ui/app_list/views/apps_grid_view.cc:1368: Index* drop_target) { On 2014/09/15 06:12:53, tapted wrote: > It would be nice if these functions were const. But you'll need to sweep through > and ensure GetExpectedTileBounds, CanDropIntoTarget, etc. are all const too. > There might be a dealbreaker along the way though, in which case (if it's not > const) the drop_target argument is probably unwarranted. (I find it pretty > readable with that though, so up to you if you want to keep it) > > rationale: AppsGridView is pretty epic and hard to reason about. `const` will > help a bit. Basically all the Can* and Get* functions should be const. Also at > the back of my mind is that all the DragState-related stuff could move to some > kind of `DragHelper` class (the `ResetDragState` function is kindof a giveaway > that such a thing should exist to encapsulate that). But that's all a bit > orthogonal. Well. That was a lot easier than I thought it would be. https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... ui/app_list/views/apps_grid_view.cc:1407: int column_offset = column_offset_direction * On 2014/09/15 06:12:53, tapted wrote: > nit: maybe a _px or _pixels suffix? Or even just `x_offset`. currently seems a > bit too much like it should be offsetting the column index. Done. https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... File ui/app_list/views/apps_grid_view.h (right): https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... ui/app_list/views/apps_grid_view.h:312: // Same as CalculateDropTarget, but with folder UI enabled. The drop target On 2014/09/15 06:12:53, tapted wrote: > I think this comment is a stray now Done. https://codereview.chromium.org/553753003/diff/160001/ui/app_list/views/apps_... ui/app_list/views/apps_grid_view.h:409: // slot if |point| if outside the page's bounds. On 2014/09/15 06:12:53, tapted wrote: > nit: if outside -> is outside Done.
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/553753003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/553753003/200001
Message was sent while issue was closed.
Committed patchset #5 (id:200001) as 4baabb4123ed7eb550a41ace8daf571790069896
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/df474d8b2e9a64b4dad10512fbadee6169ea077e Cr-Commit-Position: refs/heads/master@{#294811} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
