Animate the script badges.

Animate the script badges for cocoa and GTK.


BUG=133139
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144426

[not at google - send to devlin, 2012-06-20 22:43:25]

aa: extensions
thakis: cocoa
estade: gtk

(furthermore, my skia code comes just from reading the header files in
third_party/skia and fiddling around until it works, so any insightful comments
there would be greatly appreciated).

This animates the script badges as they appear. I decided to do it within skia
so that it's easier to write cross-platform (I haven't done views yet because I
don't have a windows box handy).

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:169:
owner_->RedrawDecoration(this);
Minor cleanup. It's unnecessary to relayout/repaint the entire page action area
since it's only the icon for this page action which needs to be updated.

There's the potential for a small cleanup here with the owner_ not existing
thing, but eh.

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1797:
UpdateVisibility(tab_contents->web_contents(), current_url_);
ditto

[Nico, 2012-06-21 17:03:07]

I'll wait with looking until aa is done (I skimmed the cocoa parts, they look
fine, I don't expect to have many comments).

Since you're touching the page action stuff: Did you guys think about HiDPI page
actions at all? Is now a good time to think about that?

[not at google - send to devlin, 2012-06-21 18:51:07]

aa -> yoz

[Aaron Boodman, 2012-06-21 18:54:22]

On Thu, Jun 21, 2012 at 10:03 AM,  <thakis@chromium.org> wrote:
> Since you're touching the page action stuff: Did you guys think about HiDPI
> page
> actions at all? Is now a good time to think about that?

That is going to be a pretty big project by itself. It should be
handled separately.

- a

[Yoyo Zhou, 2012-06-22 00:50:28]

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/location_bar_controller.h (right):

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ext...
chrome/browser/extensions/location_bar_controller.h:41: // A fade-in animation
for the items that appear in the locaion bar. These
typo: location

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ext...
chrome/browser/extensions/location_bar_controller.h:43: // but used by objects
with an unprecictable lifespan in the UI, hence
typo: unpredictable

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/script_badge_controller.h (right):

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_badge_controller.h:130: // Active animations
for each ExtensionAction (keyed by extension ID).
Have you considered storing these on the ExtensionAction instead? It seems like
the lifetime of the two should be closely related.

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:283: // Animation
notification might be for another tab.
How can this happen?

Anyway, if it could, since the icon's owner doesn't ever change, couldn't we
check that the owner is correct at AddObserver time? i.e. it seems like we don't
need the controller parameter to OnIconChanged.

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1865: // Animation notification
might be for another tab.
Same comment as mac.

[Evan Stade, 2012-06-22 00:54:05]

gtk lgtm

[not at google - send to devlin, 2012-06-26 07:30:47]

Now that the ownership has shifted from ScriptBadgeController to
ExtensionAction, the code has changed... still basically the same, but instead
of "extensions::LocationBarController::IconAnimation" it's
"ExtensionAction::IconAnimation".

PTAL everybody.

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/location_bar_controller.h (right):

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ext...
chrome/browser/extensions/location_bar_controller.h:41: // A fade-in animation
for the items that appear in the locaion bar. These
On 2012/06/22 00:50:29, Yoyo Zhou wrote:
> typo: location

Done.

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ext...
chrome/browser/extensions/location_bar_controller.h:43: // but used by objects
with an unprecictable lifespan in the UI, hence
On 2012/06/22 00:50:29, Yoyo Zhou wrote:
> typo: unpredictable

Done.

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ext...
chrome/browser/extensions/location_bar_controller.h:43: // but used by objects
with an unprecictable lifespan in the UI, hence
On 2012/06/22 00:50:29, Yoyo Zhou wrote:
> typo: unpredictable

Done.

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/script_badge_controller.h (right):

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_badge_controller.h:130: // Active animations
for each ExtensionAction (keyed by extension ID).
On 2012/06/22 00:50:29, Yoyo Zhou wrote:
> Have you considered storing these on the ExtensionAction instead? It seems
like
> the lifetime of the two should be closely related.

Done. Ended up being simpler to put it on ExtensionAction.

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

https://chromiumcodereview.appspot.com/10559054/diff/11001/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:283: // Animation
notification might be for another tab.
On 2012/06/22 00:50:29, Yoyo Zhou wrote:
> How can this happen?
> 
> Anyway, if it could, since the icon's owner doesn't ever change, couldn't we
> check that the owner is correct at AddObserver time? i.e. it seems like we
don't
> need the controller parameter to OnIconChanged.

Done.

This check was because there might be multiple animations in flight at once, and
when the same icons are present on tab1 and tab2 then switching between them
doesn't re-create the tabs.

However, what I had wasn't enough to support this anyway; we'd need to add code
to location_bar_view_mac.mm which goes though and adds observers, or something
similar.

I don't think this is worth it for a 100ms animation though. A fine
approximation is to just assume that there's only ever a single tab during the
lifetime (and not crash if there isn't), which is what I've simplified this to
(basically what you suggested).

[Yoyo Zhou, 2012-06-26 18:38:15]

LGTM. I like this ownership much better.

thakis, ready for your review.

https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/common/exte...
File chrome/common/extensions/extension_action.cc (right):

https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/common/exte...
chrome/common/extensions/extension_action.cc:278:
base::WeakPtr<ExtensionAction::IconAnimation> ExtensionAction::GetIconAnimation(
nit: Group ExtensionAnimation functions together in the cc file (don't interpose
the IconAnimationWrapper).

https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/common/exte...
File chrome/common/extensions/extension_action.h (right):

https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/common/exte...
chrome/common/extensions/extension_action.h:197: // Gets the icon animation for
a tab.
, if it is running?

[Yoyo Zhou, 2012-06-26 22:32:04]

Forgot to mention: please add a note that it's Mac and GTK only in the
CL description.

On Tue, Jun 26, 2012 at 11:38 AM,  <yoz@chromium.org> wrote:
> LGTM. I like this ownership much better.
>
> thakis, ready for your review.
>
>
>
https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/common/exte...
> File chrome/common/extensions/extension_action.cc (right):
>
>
https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/common/exte...
> chrome/common/extensions/extension_action.cc:278:
> base::WeakPtr<ExtensionAction::IconAnimation>
> ExtensionAction::GetIconAnimation(
> nit: Group ExtensionAnimation functions together in the cc file (don't
> interpose the IconAnimationWrapper).
>
>
https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/common/exte...
> File chrome/common/extensions/extension_action.h (right):
>
>
https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/common/exte...
> chrome/common/extensions/extension_action.h:197: // Gets the icon
> animation for a tab.
> , if it is running?
>
> https://chromiumcodereview.appspot.com/10559054/

[Nico, 2012-06-26 22:59:21]

One comment, other than that cocoa lgtm

https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:87:
icon_animation_->RemoveObserver(this);
This looks like something that should be done by a RAII helper

[Evan Stade, 2012-06-27 04:42:05]

gtk still lgtm

[not at google - send to devlin, 2012-06-27 06:26:53]

https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:87:
icon_animation_->RemoveObserver(this);
On 2012/06/26 22:59:21, Nico wrote:
> This looks like something that should be done by a RAII helper

Done.

https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/common/exte...
File chrome/common/extensions/extension_action.cc (right):

https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/common/exte...
chrome/common/extensions/extension_action.cc:278:
base::WeakPtr<ExtensionAction::IconAnimation> ExtensionAction::GetIconAnimation(
On 2012/06/26 18:38:15, Yoyo Zhou wrote:
> nit: Group ExtensionAnimation functions together in the cc file (don't
interpose
> the IconAnimationWrapper).

I don't understand what this means; they are grouped together? These are in the
.cc files the same order they are in the .h file.

https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/common/exte...
File chrome/common/extensions/extension_action.h (right):

https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/common/exte...
chrome/common/extensions/extension_action.h:197: // Gets the icon animation for
a tab.
On 2012/06/26 18:38:15, Yoyo Zhou wrote:
> , if it is running?

Done.

[commit-bot: I haz the power, 2012-06-27 06:27:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10559054/47002

[commit-bot: I haz the power, 2012-06-27 06:27:20]

Failed to apply patch for chrome/browser/extensions/page_action_controller.h:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/page_action_controller.h
Hunk #1 FAILED at 25.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/page_action_controller.h.rej

[Yoyo Zhou, 2012-06-27 06:28:25]

https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/common/exte...
File chrome/common/extensions/extension_action.cc (right):

https://chromiumcodereview.appspot.com/10559054/diff/24001/chrome/common/exte...
chrome/common/extensions/extension_action.cc:278:
base::WeakPtr<ExtensionAction::IconAnimation> ExtensionAction::GetIconAnimation(
On 2012/06/27 06:26:54, kalman wrote:
> On 2012/06/26 18:38:15, Yoyo Zhou wrote:
> > nit: Group ExtensionAnimation functions together in the cc file (don't
> interpose
> > the IconAnimationWrapper).
> 
> I don't understand what this means; they are grouped together? These are in
the
> .cc files the same order they are in the .h file.

Between ExtensionAction::PaintBadge and ExtensionAction::GetIconAnimation, you
have a whole class declaration.

[not at google - send to devlin, 2012-06-27 06:29:56]

Oh I see.

They're in the same order as they are in the header file, but ok.

[commit-bot: I haz the power, 2012-06-27 06:35:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10559054/42003

[commit-bot: I haz the power, 2012-06-27 06:56:05]

Try job failure for 10559054-42003 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-06-27 07:33:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10559054/48016

[commit-bot: I haz the power, 2012-06-27 09:35:02]

Change committed as 144426