Added views::ViewModelT<T>, a type-safe template version of ViewModel.

Added views::ViewModelT<T>, a type-safe template version of ViewModel.

A common use of views::ViewModel is to call view_at() then static_cast
down to the correct type, which is unsafe. ViewModel clients are now
encouraged to use ViewModelT<T> for compile-time type-checked access to
the views inside. ViewModel still exists for clients that don't care
about the view type, or with more complex downcasting logic.

Updated clients, where possible, to use ViewModelT:
TabStrip: ViewModelT<Tab>
AppsGridView: ViewModelT<AppListItemView>, ViewModelT<PulsingBlockView>.

BUG=418461
Committed: https://crrev.com/eee145893892eb15a42409c3d2e525b9f1e837e2
Cr-Commit-Position: refs/heads/master@{#297979}

[Matt Giuca, 2014-09-29 07:15:52]

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

[Matt Giuca, 2014-09-29 07:15:53]

Hi Scott,

What do you think of this proposal. See the bug for a bit of analysis on where
this is helpful (summary: 2 classes benefit greatly from the additional type
safety; 2 classes don't care, and 1 class might one day but can't use a more
specific type right now).

If you think it's OK, I'll clean it up a bit and assign other OWNERS for review.

Matt

PS. I'm not sure if I've done the templating right. Is it correct to simply move
all the .cc code into the .h file for a templated class / templated functions?

[sky, 2014-09-29 17:54:52]

What are the effects on code size here?

https://codereview.chromium.org/598013003/diff/1/ui/views/view_model.h
File ui/views/view_model.h (right):

https://codereview.chromium.org/598013003/diff/1/ui/views/view_model.h#newcode99
ui/views/view_model.h:99: ViewModel<T>::ViewModel() {
Why don't you just inline all of these now?

[Matt Giuca, 2014-09-30 04:30:41]

tapted suggested that I make a ViewModelBase class (non-templated) and then
provide a ViewModel<T> subclass, which does the casting. This means most of the
code moves back into the .cc file and gets compiled just once.

(Unfortunately that doesn't apply to view_model_utils.)

Now as a matter of opinion, I could call this templated class ViewModelT<T> and
provide a typedef ViewModelT<View> ViewModel. Then I could revert all the
changes to the classes that don't want to specialize. I prefer to require them
to explicitly ask for a ViewModel<View> but I'm happy to change it.

https://codereview.chromium.org/598013003/diff/1/ui/views/view_model.h
File ui/views/view_model.h (right):

https://codereview.chromium.org/598013003/diff/1/ui/views/view_model.h#newcode99
ui/views/view_model.h:99: ViewModel<T>::ViewModel() {
No longer relevant.

[sky, 2014-09-30 13:17:25]

I think I think tapted's suggestion, but I'm not sure it addresses
what you are after. If you upload a new patchset i'll be able to see
for sure. I don't feel strongly on the names. Why don't you upload
what you would like and I'll review that.

On Mon, Sep 29, 2014 at 9:30 PM,  <mgiuca@chromium.org> wrote:
> tapted suggested that I make a ViewModelBase class (non-templated) and then
> provide a ViewModel<T> subclass, which does the casting. This means most of
> the
> code moves back into the .cc file and gets compiled just once.
>
> (Unfortunately that doesn't apply to view_model_utils.)
>
> Now as a matter of opinion, I could call this templated class ViewModelT<T>
> and
> provide a typedef ViewModelT<View> ViewModel. Then I could revert all the
> changes to the classes that don't want to specialize. I prefer to require
> them
> to explicitly ask for a ViewModel<View> but I'm happy to change it.
>
>
> https://codereview.chromium.org/598013003/diff/1/ui/views/view_model.h
> File ui/views/view_model.h (right):
>
>
https://codereview.chromium.org/598013003/diff/1/ui/views/view_model.h#newcode99
> ui/views/view_model.h:99: ViewModel<T>::ViewModel() {
> No longer relevant.
>
> https://codereview.chromium.org/598013003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Matt Giuca, 2014-10-01 03:07:03]

On 2014/09/30 13:17:25, sky wrote:
> I think I think tapted's suggestion, but I'm not sure it addresses
> what you are after. If you upload a new patchset i'll be able to see
> for sure. I don't feel strongly on the names. Why don't you upload
> what you would like and I'll review that.

I did upload it yesterday...

Anyway, tapted suggested a further change which is to have ViewModelUtils
directly use ViewModelBase (thus not needing to be templated). I've uploaded all
the patches. PTAL.

Please comment on whether you think all the ViewModel usage should be templated
(ie. ViewModel<views::View>) or whether there should be a typedef so that any
code that doesn't care about specific types can go unchanged. Also feel free to
suggest better names for these things.

Thanks

Matt

[sky, 2014-10-01 19:20:38]

https://codereview.chromium.org/598013003/diff/140001/ui/views/view_model.cc
File ui/views/view_model.cc (left):

https://codereview.chromium.org/598013003/diff/140001/ui/views/view_model.cc#...
ui/views/view_model.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights
reserved.
codereview is showing this file as deleted. Make sure that does't happen.

https://codereview.chromium.org/598013003/diff/140001/ui/views/view_model.h
File ui/views/view_model.h (right):

https://codereview.chromium.org/598013003/diff/140001/ui/views/view_model.h#n...
ui/views/view_model.h:51: View* ViewAtBase(int index) const {
Can we name this view_at and the subclass implementation t_at or view_t_at?
Alternatively make this protected and friend ViewModelUtils and leave the names
you have.

https://codereview.chromium.org/598013003/diff/140001/ui/views/view_model.h#n...
ui/views/view_model.h:106: class ViewModel : public ViewModelBase {
I like naming this ViewModelT and having classes that use this typedef to
ViewModel.

[Matt Giuca, 2014-10-02 01:23:51]

https://codereview.chromium.org/598013003/diff/140001/ui/views/view_model.cc
File ui/views/view_model.cc (left):

https://codereview.chromium.org/598013003/diff/140001/ui/views/view_model.cc#...
ui/views/view_model.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights
reserved.
What the hell?

OK, this is https://code.google.com/p/rietveld/issues/detail?id=84; I've worked
around it by a weird dance of Patch Sets 9 and 10.

https://codereview.chromium.org/598013003/diff/140001/ui/views/view_model.h
File ui/views/view_model.h (right):

https://codereview.chromium.org/598013003/diff/140001/ui/views/view_model.h#n...
ui/views/view_model.h:51: View* ViewAtBase(int index) const {
I think I prefer the friends approach (since "normal code" should not be
accessing ViewAtBase, only this special class).

https://codereview.chromium.org/598013003/diff/140001/ui/views/view_model.h#n...
ui/views/view_model.h:106: class ViewModel : public ViewModelBase {
On 2014/10/01 19:20:38, sky wrote:
> I like naming this ViewModelT and having classes that use this typedef to
> ViewModel.

Done.

[sky, 2014-10-02 02:28:47]

LGTM

[Matt Giuca, 2014-10-02 02:38:03]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-10-02 02:39:11]

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

[commit-bot: I haz the power, 2014-10-02 03:26:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-02 03:26:35]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Matt Giuca, 2014-10-02 08:00:19]

FYI, I made a slight change to StackedTabStripLayout in Patch Set 12 to fix a
unit_tests compile error. I can't pass a real ViewModelT<Tab> from the test;
that would require creating real Tab objects which requires a TabController
which requires a whole FakeTabController subclass. Too much code. So instead,
StackedTabStripLayout just takes a ViewModelBase (which is fine because it
doesn't need Add or view_at, and if it did, the tests would be broken anyway).

[Matt Giuca, 2014-10-02 08:00:24]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-10-02 08:01:10]

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

[commit-bot: I haz the power, 2014-10-02 08:08:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-02 08:08:42]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Matt Giuca, 2014-10-02 09:38:54]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-10-02 09:40:07]

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

[commit-bot: I haz the power, 2014-10-02 09:53:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-02 09:53:20]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Matt Giuca, 2014-10-02 11:57:56]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-10-02 11:59:21]

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

[commit-bot: I haz the power, 2014-10-02 12:30:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-02 12:30:50]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Matt Giuca, 2014-10-02 14:21:58]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-10-02 14:22:46]

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

[commit-bot: I haz the power, 2014-10-02 15:07:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-02 15:07:22]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Matt Giuca, 2014-10-03 00:45:23]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-10-03 00:46:34]

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

[commit-bot: I haz the power, 2014-10-03 01:53:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-03 01:53:55]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Matt Giuca, 2014-10-03 02:39:10]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-10-03 02:40:21]

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

[commit-bot: I haz the power, 2014-10-03 02:51:09]

Committed patchset #13 (id:240001) as 97f03e451a031e5e0f533a7eb3e265d89db60951

[commit-bot: I haz the power, 2014-10-03 02:51:48]

Patchset 13 (id:??) landed as
https://crrev.com/eee145893892eb15a42409c3d2e525b9f1e837e2
Cr-Commit-Position: refs/heads/master@{#297979}