AppsGridView: Use specific types instead of views::View*.

AppsGridView: Use specific types instead of views::View*.

Almost every views::View* in this class is actually an AppListItemView*,
and we are now specific about that. Also made a few explicit
PulsingBlockViews. This removes a mess of static_casts throughout the
file, and makes the interface to many methods type-safe and explicit.

Committed: https://crrev.com/0ee0ce7028862d0c8fbc44880947cc25cd2b7cd8
Cr-Commit-Position: refs/heads/master@{#297341}

[Matt Giuca, 2014-09-25 06:18:11]

mgiuca@chromium.org changed reviewers:
+ calamity@chromium.org

[Matt Giuca, 2014-09-25 06:18:12]

This could go further and replace all use of views::View* in this module with
AppListItemView, but the remaining stuff doesn't actually care what type it is
dealing with so I think it's fine to leave as-is.

[tapted, 2014-09-25 06:35:28]

https://codereview.chromium.org/599933004/diff/1/ui/app_list/views/apps_grid_...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/599933004/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.cc:766: return
static_cast<AppListItemView*>(view_model_.view_at(index));
I wish we could go further by extending ViewModel with subclass... but I think
it's too brittle anyway.

// Subclass of ViewModel that attempts to hide type-unsafe Adds using
// overload resolution rules, while providing an auto-casting item_at.
class ItemViewModel : public ViewModel {
 public:
  void Add(AppListItemView* view, int index) {
    ViewModel::Add(view, index);
  }
  AppListItemView* view_at(int index) const {
    return static_cast<AppListItemView*>(ViewModel::view_at(index));
  }
};

#hides

[calamity, 2014-09-26 01:47:26]

lgtm

https://codereview.chromium.org/599933004/diff/1/ui/app_list/views/apps_grid_...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/599933004/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.cc:766: return
static_cast<AppListItemView*>(view_model_.view_at(index));
On 2014/09/25 06:35:28, tapted wrote:
> I wish we could go further by extending ViewModel with subclass... but I think
> it's too brittle anyway.
> 
> // Subclass of ViewModel that attempts to hide type-unsafe Adds using
> // overload resolution rules, while providing an auto-casting item_at.
> class ItemViewModel : public ViewModel {
>  public:
>   void Add(AppListItemView* view, int index) {
>     ViewModel::Add(view, index);
>   }
>   AppListItemView* view_at(int index) const {
>     return static_cast<AppListItemView*>(ViewModel::view_at(index));
>   }
> };
> 
> #hides

Could go make ViewModel templated.

#doublehides

https://codereview.chromium.org/599933004/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.cc:2117: AppListItemView*
AppsGridView::GetViewAtSlotOnCurrentPage(int slot) {
Ugh. This needs to get a better name that indicates slot may not match the
model. Maybe GetViewDisplayedAtGridSlot()?

Also, add a comment in the header that this will return the view that is
currently displayed at the given slot, which may not match the model.

https://codereview.chromium.org/599933004/diff/1/ui/app_list/views/apps_grid_...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/599933004/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.h:273: AppListItemView* GetViewAtIndex(const
Index& index) const;
This should have a comment mentioning that these methods are only for
AppListItemViews (as opposed to PulsingBlockViews).

[Matt Giuca, 2014-09-26 03:48:39]

Q about GetIndexOfView.

https://codereview.chromium.org/599933004/diff/1/ui/app_list/views/apps_grid_...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/599933004/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.cc:766: return
static_cast<AppListItemView*>(view_model_.view_at(index));
I think the template is a good idea. Follow-up patch?

https://codereview.chromium.org/599933004/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.cc:2117: AppListItemView*
AppsGridView::GetViewAtSlotOnCurrentPage(int slot) {
Done.

Note that I called it GetViewDisplayedAtSlotOnCurrentPage -- got to keep the "on
current page" part because that's an important facet of its functionality.

https://codereview.chromium.org/599933004/diff/1/ui/app_list/views/apps_grid_...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/599933004/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.h:273: AppListItemView* GetViewAtIndex(const
Index& index) const;
If GetIndexOfView is only supposed to be passed an AppListItemView, then I can
just change its argument type to AppListItemView*. (Then don't need a comment.)

(I'd rather type safety than comments.)

[Matt Giuca, 2014-09-26 04:33:56]

Patchset #3 (id:40001) has been deleted

[Matt Giuca, 2014-09-26 04:35:38]

OK huge change -- everything is now type safe, not just the static cast parts.
PTAL.

[calamity, 2014-09-30 01:22:23]

SLGTM

https://codereview.chromium.org/599933004/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/599933004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:424: // the model (as it may have been
temporarily moved during a drag operation).
Noice.

[Matt Giuca, 2014-09-30 01:32:06]

https://codereview.chromium.org/599933004/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/599933004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:424: // the model (as it may have been
temporarily moved during a drag operation).
On 2014/09/30 01:22:23, calamity wrote:
> Noice.

Acknowledged.

[Matt Giuca, 2014-09-30 01:32:10]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-09-30 01:32:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/599933004/60001

[commit-bot: I haz the power, 2014-09-30 02:26:55]

Committed patchset #3 (id:60001) as 06988f5442eb75983f3f8c366cc2c2cd7026fb61

[commit-bot: I haz the power, 2014-09-30 02:27:36]

Patchset 3 (id:??) landed as
https://crrev.com/0ee0ce7028862d0c8fbc44880947cc25cd2b7cd8
Cr-Commit-Position: refs/heads/master@{#297341}