Use ash NetworkIconAnimation in NetworkMenuIcon code

Use ash NetworkIconAnimation in NetworkMenuIcon code

This addresses two probles:
1. Icon animations are not always in sync
2. ash_system_tray_delegate uses 3 NetworkMenuIcon instances, which
   can cause 3 * 60 = 180 animation updates per second. Limit this
   by only sending a message when the index changes (5 times per
   second). This isn't a perfect fix, but NetworkMenuIcon is being
   deprecated soon...

BUG=173156
For ash.gyp:
TBR=sky@chromium.org

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

[stevenjb, 2013-01-31 03:53:47]

Unfortunately, this is important for R25 because the 180 updates per second is
causing some significant performance issues when coming out of suspend. 

I plan to do this differently in the new code, so I've keep the changes as small
as possible.

[jennyz, 2013-01-31 19:16:50]

https://codereview.chromium.org/12094072/diff/1/ash/system/chromeos/network/n...
File ash/system/chromeos/network/network_icon_animation.cc (right):

https://codereview.chromium.org/12094072/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon_animation.cc:54: new
NetworkIconAnimation();
A little bit concern about using static for singleton instance here, since the
object will stay in memory once it is used, and no one is going to clean it up
until the system exit. I am not sure if any of the our memory bot will complain
about this or not.

https://codereview.chromium.org/12094072/diff/1/ash/system/chromeos/network/n...
File ash/system/chromeos/network/network_icon_animation.h (right):

https://codereview.chromium.org/12094072/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon_animation.h:24: double GetAnimation();
Please add a comment for GetAnimation method.

https://codereview.chromium.org/12094072/diff/1/chrome/browser/chromeos/statu...
File chrome/browser/chromeos/status/network_menu_icon.cc (right):

https://codereview.chromium.org/12094072/diff/1/chrome/browser/chromeos/statu...
chrome/browser/chromeos/status/network_menu_icon.cc:802: NOTREACHED();
Why should we still SetDisconnectedIconAndText if this flow should NOT be
reached?

https://codereview.chromium.org/12094072/diff/1/chrome/browser/chromeos/statu...
chrome/browser/chromeos/status/network_menu_icon.cc:811: // If not connecting to
a network, show the active/connected VPN.
Should the comment be: "If not connecting to a vpn,"?

[stevenjb, 2013-01-31 19:37:42]

https://codereview.chromium.org/12094072/diff/1/ash/system/chromeos/network/n...
File ash/system/chromeos/network/network_icon_animation.cc (right):

https://codereview.chromium.org/12094072/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon_animation.cc:54: new
NetworkIconAnimation();
On 2013/01/31 19:16:50, jennyz wrote:
> A little bit concern about using static for singleton instance here, since the
> object will stay in memory once it is used, and no one is going to clean it up
> until the system exit. I am not sure if any of the our memory bot will
complain
> about this or not. 

We already use this same model in a number of places, e.g.
NetworkMenuIcon::GetImage(). I'll double check with oshima, but this is just
moved from network_icon.cc.

https://codereview.chromium.org/12094072/diff/1/chrome/browser/chromeos/statu...
File chrome/browser/chromeos/status/network_menu_icon.cc (right):

https://codereview.chromium.org/12094072/diff/1/chrome/browser/chromeos/statu...
chrome/browser/chromeos/status/network_menu_icon.cc:802: NOTREACHED();
On 2013/01/31 19:16:50, jennyz wrote:
> Why should we still SetDisconnectedIconAndText if this flow should NOT be
> reached?
Didn't you add this? :P Yes, this should be a warning.

https://codereview.chromium.org/12094072/diff/1/chrome/browser/chromeos/statu...
chrome/browser/chromeos/status/network_menu_icon.cc:811: // If not connecting to
a network, show the active/connected VPN.
On 2013/01/31 19:16:50, jennyz wrote:
> Should the comment be: "If not connecting to a vpn,"?
Ditto :) Changed.

[stevenjb, 2013-01-31 19:41:03]

https://codereview.chromium.org/12094072/diff/1/ash/system/chromeos/network/n...
File ash/system/chromeos/network/network_icon_animation.cc (right):

https://codereview.chromium.org/12094072/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon_animation.cc:54: new
NetworkIconAnimation();
On 2013/01/31 19:37:42, stevenjb (chromium) wrote:
> On 2013/01/31 19:16:50, jennyz wrote:
> > A little bit concern about using static for singleton instance here, since
the
> > object will stay in memory once it is used, and no one is going to clean it
up
> > until the system exit. I am not sure if any of the our memory bot will
> complain
> > about this or not. 
> 
> We already use this same model in a number of places, e.g.
> NetworkMenuIcon::GetImage(). I'll double check with oshima, but this is just
> moved from network_icon.cc.
> 
Confirmed with oshima that this is OK.

[jennyz, 2013-01-31 19:44:46]

lgtm. Please upload the patch for fixing nits.

https://codereview.chromium.org/12094072/diff/1/chrome/browser/chromeos/statu...
File chrome/browser/chromeos/status/network_menu_icon.cc (right):

https://codereview.chromium.org/12094072/diff/1/chrome/browser/chromeos/statu...
chrome/browser/chromeos/status/network_menu_icon.cc:802: NOTREACHED();
On 2013/01/31 19:37:42, stevenjb (chromium) wrote:
> On 2013/01/31 19:16:50, jennyz wrote:
> > Why should we still SetDisconnectedIconAndText if this flow should NOT be
> > reached?
> Didn't you add this? :P Yes, this should be a warning.
Really, my bad then.

https://codereview.chromium.org/12094072/diff/1/chrome/browser/chromeos/statu...
chrome/browser/chromeos/status/network_menu_icon.cc:811: // If not connecting to
a network, show the active/connected VPN.
On 2013/01/31 19:37:42, stevenjb (chromium) wrote:
> On 2013/01/31 19:16:50, jennyz wrote:
> > Should the comment be: "If not connecting to a vpn,"?
> Ditto :) Changed.
Thanks for changing, I didn't see the new patch, have you uploaded?

[jennyz, 2013-01-31 19:53:46]

lgtm

[commit-bot: I haz the power, 2013-01-31 19:58:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/12094072/7001

[commit-bot: I haz the power, 2013-01-31 19:58:27]

Presubmit check for 12094072-7001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ash

Presubmit checks took 1.1s to calculate.

[commit-bot: I haz the power, 2013-01-31 20:14:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/12094072/7001

[commit-bot: I haz the power, 2013-02-01 00:18:53]

Change committed as 180012

[tfarina, 2013-02-20 02:48:48]

https://chromiumcodereview.appspot.com/12094072/diff/7001/ash/system/chromeos...
File ash/system/chromeos/network/network_icon_animation.cc (right):

https://chromiumcodereview.appspot.com/12094072/diff/7001/ash/system/chromeos...
ash/system/chromeos/network/network_icon_animation.cc:26: void
NetworkIconAnimation::AnimationProgressed(const ui::Animation* animation) {
the order of this definition does not match the order declared in the header
file.

https://chromiumcodereview.appspot.com/12094072/diff/7001/ash/system/chromeos...
File ash/system/chromeos/network/network_icon_animation.h (right):

https://chromiumcodereview.appspot.com/12094072/diff/7001/ash/system/chromeos...
ash/system/chromeos/network/network_icon_animation.h:11: #include
"ui/base/animation/throb_animation.h"
you could have forward declared it instead

https://chromiumcodereview.appspot.com/12094072/diff/7001/ash/system/chromeos...
ash/system/chromeos/network/network_icon_animation.h:14: namespace network_icon
{
why put this in this 'random' namespace?

https://chromiumcodereview.appspot.com/12094072/diff/7001/ash/system/chromeos...
ash/system/chromeos/network/network_icon_animation.h:31: virtual void
AnimationProgressed(const ui::Animation* animation) OVERRIDE;
this could have been private

https://chromiumcodereview.appspot.com/12094072/diff/7001/ash/system/chromeos...
ash/system/chromeos/network/network_icon_animation.h:36: ui::ThrobAnimation
animation_;
and used scoped_ptr<ui::ThrobAnimation> ...

https://chromiumcodereview.appspot.com/12094072/diff/7001/ash/system/chromeos...
ash/system/chromeos/network/network_icon_animation.h:38: };
DISALLOW_COPY_AND_ASSIGN