Audio indicator: cross platform drawing and animation

Audio indicator: cross platform drawing and animation

This CL adds a new class, TabAudioIndicator, that does cross-platform
drawing and animating of the audio indicator.

BUG=3541
TEST=Verified that TabAudioIndicatorTest.* passed. UI changes to come in a later patch.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187690

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187720

[cpu_(ooo_6.6-7.5), 2013-03-11 02:40:27]

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_audio_indicator.cc (right):

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.cc:24: const int
kEqualizerFrames[][kEqualizerColumnCount] = {
size_t

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.cc:32: const int kFrameCount =
arraysize(kEqualizerFrames);
nuke this constant and use arraysize in the two calling sites. Looks clearer to
me this way.

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.cc:42: const int kFPS = 25;
So I understand, the duration implies 3fps, but we update at 25 fps. Now, we are
talking about 8 calls each frame to go from
say 7 to 4, then to 4 to 4, then 4, 3 then 3 to 2 then 2, 3 and then 3 to 7, so
max distance of like 4

It would say to me that kFPS should be more like 15 or so?

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.cc:58: animation_.reset(
do you get AnimationEnded?  don't you have to call Stop() before reseting the
animation?

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.cc:91: canvas->DrawImageInt(*image,
x, y);
I thought we had 8 images, one for each level...

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_audio_indicator.h (right):

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.h:10: #include "base/basictypes.h"
what does require basictypes.h in this header?

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.h:57: int frame_index_;
size_t

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.h:67: STATE_NOT_ANIMATING,
it is nice if STATE_NOT_ANIMATING is first since it is the ctor state.

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_audio_indicator_unittest.cc (right):

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator_unittest.cc:53:
indicator.Paint(&canvas, rect);
what are we testing here?

[sail, 2013-03-11 23:16:12]

Addressed review comments.
+sky for chrome/browser/ui/* OWNERS review.

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_audio_indicator.cc (right):

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.cc:24: const int
kEqualizerFrames[][kEqualizerColumnCount] = {
On 2013/03/11 02:40:27, cpu wrote:
> size_t

Done.

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.cc:32: const int kFrameCount =
arraysize(kEqualizerFrames);
On 2013/03/11 02:40:27, cpu wrote:
> nuke this constant and use arraysize in the two calling sites. Looks clearer
to
> me this way.

Done.

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.cc:42: const int kFPS = 25;
On 2013/03/11 02:40:27, cpu wrote:
> So I understand, the duration implies 3fps, but we update at 25 fps. Now, we
are
> talking about 8 calls each frame to go from
> say 7 to 4, then to 4 to 4, then 4, 3 then 3 to 2 then 2, 3 and then 3 to 7,
so
> max distance of like 4
> 
> It would say to me that kFPS should be more like 15 or so?

Done.
Good point. Changed to 15.

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.cc:58: animation_.reset(
On 2013/03/11 02:40:27, cpu wrote:
> do you get AnimationEnded?  don't you have to call Stop() before reseting the
> animation?

To get AnimationEnded we have to call End(). In this case we explicitly don't
want AnimationEnded() to get called.

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.cc:91: canvas->DrawImageInt(*image,
x, y);
On 2013/03/11 02:40:27, cpu wrote:
> I thought we had 8 images, one for each level...

Using just one image was a lot simpler. Because of the ClipRect() we just draw
it lower and get the same effect.

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_audio_indicator.h (right):

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.h:10: #include "base/basictypes.h"
On 2013/03/11 02:40:27, cpu wrote:
> what does require basictypes.h in this header?

Done.
Removed.

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.h:57: int frame_index_;
On 2013/03/11 02:40:27, cpu wrote:
> size_t

Done.

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.h:67: STATE_NOT_ANIMATING,
On 2013/03/11 02:40:27, cpu wrote:
> it is nice if STATE_NOT_ANIMATING is first since it is the ctor state.

Done.

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_audio_indicator_unittest.cc (right):

https://codereview.chromium.org/12744003/diff/2001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator_unittest.cc:53:
indicator.Paint(&canvas, rect);
On 2013/03/11 02:40:27, cpu wrote:
> what are we testing here?

Just exercising the code to make sure nothing leaks for crashes. Added comment.

[sky, 2013-03-12 03:59:43]

https://codereview.chromium.org/12744003/diff/8001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/12744003/diff/8001/chrome/browser/ui/browser....
chrome/browser/ui/browser.h:477: friend class TabAudioIndicatorTest;
Why the friend here?

https://codereview.chromium.org/12744003/diff/8001/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_audio_indicator.h (right):

https://codereview.chromium.org/12744003/diff/8001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.h:30: };
protected: virtual ~Delegate() {}

https://codereview.chromium.org/12744003/diff/8001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.h:32: explicit
TabAudioIndicator(Delegate* delegate);
Can you make this take an AnimationContainer so that the tabs can share the same
AnimationContainer?

https://codereview.chromium.org/12744003/diff/8001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.h:63: enum State {
enums should be first in a section.

[sail, 2013-03-12 04:24:32]

https://codereview.chromium.org/12744003/diff/8001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/12744003/diff/8001/chrome/browser/ui/browser....
chrome/browser/ui/browser.h:477: friend class TabAudioIndicatorTest;
On 2013/03/12 03:59:43, sky wrote:
> Why the friend here?

Done.
Oops, from an old version of this CL, no longer needed. Removed.

https://codereview.chromium.org/12744003/diff/8001/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_audio_indicator.h (right):

https://codereview.chromium.org/12744003/diff/8001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.h:30: };
On 2013/03/12 03:59:43, sky wrote:
> protected: virtual ~Delegate() {}

Done.

https://codereview.chromium.org/12744003/diff/8001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.h:32: explicit
TabAudioIndicator(Delegate* delegate);
On 2013/03/12 03:59:43, sky wrote:
> Can you make this take an AnimationContainer so that the tabs can share the
same
> AnimationContainer?

Done.

https://codereview.chromium.org/12744003/diff/8001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.h:63: enum State {
On 2013/03/12 03:59:43, sky wrote:
> enums should be first in a section.

Done.

[sail, 2013-03-12 06:15:58]

https://codereview.chromium.org/12744003/diff/8001/chrome/browser/ui/tabs/tab...
File chrome/browser/ui/tabs/tab_audio_indicator.h (right):

https://codereview.chromium.org/12744003/diff/8001/chrome/browser/ui/tabs/tab...
chrome/browser/ui/tabs/tab_audio_indicator.h:32: explicit
TabAudioIndicator(Delegate* delegate);
On 2013/03/12 04:24:33, sail wrote:
> On 2013/03/12 03:59:43, sky wrote:
> > Can you make this take an AnimationContainer so that the tabs can share the
> same
> > AnimationContainer?
> 
> Done.

Actually, had to move this to a setter so that the views code could pass it in.

[sky, 2013-03-12 14:19:23]

LGTM

[commit-bot: I haz the power, 2013-03-12 15:57:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12744003/18001

[commit-bot: I haz the power, 2013-03-12 16:59:05]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-12 18:07:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12744003/18001

[commit-bot: I haz the power, 2013-03-12 18:41:05]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-12 18:54:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12744003/18001

[cpu_(ooo_6.6-7.5), 2013-03-12 20:26:38]

lgtm

[sail, 2013-03-12 23:16:08]

Committed patchset #5 manually as r187690 (presubmit successful).

[sail, 2013-03-13 00:38:26]

Committed patchset #6 manually as r187720 (presubmit successful).