Added a views::test::TestInkDropAnimationObserverHelper

Added a views::test::TestInkDropAnimationObserverHelper

This change is a foundation for a follow-up CL that will be adding an
InkDropHoverObserver and a TestInkDropHoverObserver. This change will allow
code re-use between the InkDropHoverObserver/InkDropAnimationObserver and
TestInkDropHoverObserver/TestInkDropAnimationObserver classes.

BUG=604415
Committed: https://crrev.com/0cf6c96f36bd9a5a6bbfebb13285d5166f193881
Cr-Commit-Position: refs/heads/master@{#388923}

[bruthig, 2016-04-20 14:30:19]

Description was changed from

==========
Added a views::AnimationObserver<T>

Extracted the InkDropAnimationObserver implementation into the more
general AnimationObserver<T> class so that it can be re-used for an
InkDropHover observer.

BUG=604415
==========

to

==========
Added a views::AnimationObserver<ContextType>

This change is a foundation for a follow-up CL that will be adding an
InkDropHoverObserver and a TestInkDropHoverObserver. This change will allow
code re-use between the InkDropHoverObserver/InkDropAnimationObserver and
TestInkDropHoverObserver/TestInkDropAnimationObserver classes.

BUG=604415
==========

[bruthig, 2016-04-20 15:14:01]

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

[bruthig, 2016-04-20 15:14:01]

sky@, can you PTAL?

[sky, 2016-04-20 19:38:48]

Can you elaborate on why you need the template parameter? Can't observer
implementations internally keep a reference to whatever state they may need and
not have it passed in?

https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ani...
File ui/views/animation/animation_observer.h (right):

https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ani...
ui/views/animation/animation_observer.h:49: template <typename ContextType>
Seems like the only reason you need the template parameter here is because the
enum is inside the class. If you move the enum outside the class you won't need
the template param.

https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ani...
ui/views/animation/animation_observer.h:54: return std::string("SUCCESS");
nit: no std::string(), just "SUCCESS".

https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_observer.h (right):

https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_observer.h:15: typedef
AnimationObserver<InkDropState> InkDropAnimationObserver;
typedef->using

[bruthig, 2016-04-20 21:20:09]

On 2016/04/20 19:38:48, sky wrote:
> Can you elaborate on why you need the template parameter? Can't observer
> implementations internally keep a reference to whatever state they may need
and
> not have it passed in?

I will be following up this CL with an InkDropHoverObserver and more importantly
a TestInkDropHoverObserver.  I'm using the ContextType template parameter so
that the TestInkDropAnimationObserver and the TestInkDropHoverObserver will be
able to re-use the code in TestAnimationObserver<ContextType> instead of having
to re-implement it.

https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ani...
File ui/views/animation/animation_observer.h (right):

https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ani...
ui/views/animation/animation_observer.h:49: template <typename ContextType>
On 2016/04/20 19:38:47, sky wrote:
> Seems like the only reason you need the template parameter here is because the
> enum is inside the class. If you move the enum outside the class you won't
need
> the template param.

Done.

https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ani...
ui/views/animation/animation_observer.h:54: return std::string("SUCCESS");
On 2016/04/20 19:38:47, sky wrote:
> nit: no std::string(), just "SUCCESS".

Done.

https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_observer.h (right):

https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_observer.h:15: typedef
AnimationObserver<InkDropState> InkDropAnimationObserver;
On 2016/04/20 19:38:47, sky wrote:
> typedef->using

Done.

[sky, 2016-04-20 22:25:46]

On Wed, Apr 20, 2016 at 2:20 PM,  <bruthig@chromium.org> wrote:
> On 2016/04/20 19:38:48, sky wrote:
>> Can you elaborate on why you need the template parameter? Can't observer
>> implementations internally keep a reference to whatever state they may
>> need
> and
>> not have it passed in?
>
> I will be following up this CL with an InkDropHoverObserver and more
> importantly
> a TestInkDropHoverObserver. I'm using the ContextType template parameter so
> that the TestInkDropAnimationObserver and the TestInkDropHoverObserver will
> be
> able to re-use the code in TestAnimationObserver<ContextType> instead of
> having
> to re-implement it.

Can't the two tests just as well delegate to a helper class? I get
that the template arg can save you code, but it seems unnecessary and
adds complexity purely for a test. Maybe I'm missing something?

  -Scott

>
>
>
https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ani...
> File ui/views/animation/animation_observer.h (right):
>
>
https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ani...
> ui/views/animation/animation_observer.h:49: template <typename
> ContextType>
> On 2016/04/20 19:38:47, sky wrote:
>> Seems like the only reason you need the template parameter here is
> because the
>> enum is inside the class. If you move the enum outside the class you
> won't need
>> the template param.
>
> Done.
>
>
https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ani...
> ui/views/animation/animation_observer.h:54: return
> std::string("SUCCESS");
> On 2016/04/20 19:38:47, sky wrote:
>> nit: no std::string(), just "SUCCESS".
>
> Done.
>
>
https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ink...
> File ui/views/animation/ink_drop_animation_observer.h (right):
>
>
https://codereview.chromium.org/1896953003/diff/100001/ui/views/animation/ink...
> ui/views/animation/ink_drop_animation_observer.h:15: typedef
> AnimationObserver<InkDropState> InkDropAnimationObserver;
> On 2016/04/20 19:38:47, sky wrote:
>> typedef->using
>
> Done.
>
> https://codereview.chromium.org/1896953003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bruthig, 2016-04-21 18:21:33]

On 2016/04/20 22:25:46, sky wrote:
> On Wed, Apr 20, 2016 at 2:20 PM,  <mailto:bruthig@chromium.org> wrote:
> > On 2016/04/20 19:38:48, sky wrote:
> >> Can you elaborate on why you need the template parameter? Can't observer
> >> implementations internally keep a reference to whatever state they may
> >> need
> > and
> >> not have it passed in?
> >
> > I will be following up this CL with an InkDropHoverObserver and more
> > importantly
> > a TestInkDropHoverObserver. I'm using the ContextType template parameter so
> > that the TestInkDropAnimationObserver and the TestInkDropHoverObserver will
> > be
> > able to re-use the code in TestAnimationObserver<ContextType> instead of
> > having
> > to re-implement it.
> 
> Can't the two tests just as well delegate to a helper class? I get
> that the template arg can save you code, but it seems unnecessary and
> adds complexity purely for a test. Maybe I'm missing something?
> 
>   -Scott

I've removed the template parameter from the production code but have left it in
for the TestInkDropAnimationObserverHelper. WDYT?

[sky, 2016-04-21 20:19:06]

LGTM

https://codereview.chromium.org/1896953003/diff/140001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_ended_reason.cc (right):

https://codereview.chromium.org/1896953003/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_ended_reason.cc:20: return "";
nit: "" -> std::string()

[bruthig, 2016-04-21 20:53:29]

Description was changed from

==========
Added a views::AnimationObserver<ContextType>

This change is a foundation for a follow-up CL that will be adding an
InkDropHoverObserver and a TestInkDropHoverObserver. This change will allow
code re-use between the InkDropHoverObserver/InkDropAnimationObserver and
TestInkDropHoverObserver/TestInkDropAnimationObserver classes.

BUG=604415
==========

to

==========
Added a views::test::TestInkDropAnimationObserverHelper

This change is a foundation for a follow-up CL that will be adding an
InkDropHoverObserver and a TestInkDropHoverObserver. This change will allow
code re-use between the InkDropHoverObserver/InkDropAnimationObserver and
TestInkDropHoverObserver/TestInkDropAnimationObserver classes.

BUG=604415
==========

[bruthig, 2016-04-21 21:13:22]

https://codereview.chromium.org/1896953003/diff/140001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_ended_reason.cc (right):

https://codereview.chromium.org/1896953003/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_ended_reason.cc:20: return "";
On 2016/04/21 20:19:06, sky wrote:
> nit: "" -> std::string()

Done.

[bruthig, 2016-04-21 21:45:41]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-04-21 21:45:42]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1896953003/#ps160001
(title: "Fixed build issue and nit.")

[commit-bot: I haz the power, 2016-04-21 21:46:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1896953003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1896953003/160001

[commit-bot: I haz the power, 2016-04-21 22:29:33]

Description was changed from

==========
Added a views::test::TestInkDropAnimationObserverHelper

This change is a foundation for a follow-up CL that will be adding an
InkDropHoverObserver and a TestInkDropHoverObserver. This change will allow
code re-use between the InkDropHoverObserver/InkDropAnimationObserver and
TestInkDropHoverObserver/TestInkDropAnimationObserver classes.

BUG=604415
==========

to

==========
Added a views::test::TestInkDropAnimationObserverHelper

This change is a foundation for a follow-up CL that will be adding an
InkDropHoverObserver and a TestInkDropHoverObserver. This change will allow
code re-use between the InkDropHoverObserver/InkDropAnimationObserver and
TestInkDropHoverObserver/TestInkDropAnimationObserver classes.

BUG=604415
==========

[commit-bot: I haz the power, 2016-04-21 22:29:34]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-22 19:40:28]

Description was changed from

==========
Added a views::test::TestInkDropAnimationObserverHelper

This change is a foundation for a follow-up CL that will be adding an
InkDropHoverObserver and a TestInkDropHoverObserver. This change will allow
code re-use between the InkDropHoverObserver/InkDropAnimationObserver and
TestInkDropHoverObserver/TestInkDropAnimationObserver classes.

BUG=604415
==========

to

==========
Added a views::test::TestInkDropAnimationObserverHelper

This change is a foundation for a follow-up CL that will be adding an
InkDropHoverObserver and a TestInkDropHoverObserver. This change will allow
code re-use between the InkDropHoverObserver/InkDropAnimationObserver and
TestInkDropHoverObserver/TestInkDropAnimationObserver classes.

BUG=604415

Committed: https://crrev.com/0cf6c96f36bd9a5a6bbfebb13285d5166f193881
Cr-Commit-Position: refs/heads/master@{#388923}
==========

[commit-bot: I haz the power, 2016-04-22 19:40:29]

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