Add LauncherIconObserver.

Implement LauncherIconObserver.

This will be used to listen for changes in the launcher view so that panels can be correctly drawn over the corresponding launcher icon.

BUG=124115
TEST=aura_shell_unittests

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

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

[dcheng, 2012-04-18 23:23:36]

I've updated this patch based on the last comments in the other review thread at
http://codereview.chromium.org/9808026/. However, I'm a little worried about the
complexity this patch adds; it seems like we are adding redundant
CalculateIdealBounds() just to make the observers work correctly--for example,
in LauncherItemRemoved, we call it once so we can notify the observers, and then
we call it again when the remove animation is finished. I don't really have a
better way to do it though...

[sky, 2012-04-19 00:05:40]

http://codereview.chromium.org/10116011/diff/6001/ash/launcher/launcher_view_...
File ash/launcher/launcher_view_unittest.cc (right):

http://codereview.chromium.org/10116011/diff/6001/ash/launcher/launcher_view_...
ash/launcher/launcher_view_unittest.cc:4: 
tests should follow that of non-test cc files and include the header for the
file being tested first.

http://codereview.chromium.org/10116011/diff/6001/ash/launcher/launcher_view_...
ash/launcher/launcher_view_unittest.cc:71: ash::test::TestLauncherDelegate*
launcher_delegate =
Instead of this, how about a static instance method on TestLauncherDelegate and
an ASSERT(launcher_delegate). That's less fragile and easy to know if something
fails.

http://codereview.chromium.org/10116011/diff/6001/ash/test/test_launcher_dele...
File ash/test/test_launcher_delegate.cc (right):

http://codereview.chromium.org/10116011/diff/6001/ash/test/test_launcher_dele...
ash/test/test_launcher_delegate.cc:66: void
TestLauncherDelegate::AddLauncherItem(aura::Window* window) {
Position should match header.

http://codereview.chromium.org/10116011/diff/6001/ash/test/test_launcher_dele...
ash/test/test_launcher_delegate.cc:69: window_to_id_[window] =
model_->next_id();
How about a DCHECK(window_to_id_.find(window) == window_to_id_.end())

http://codereview.chromium.org/10116011/diff/6001/ash/test/test_launcher_dele...
ash/test/test_launcher_delegate.cc:74: window->parent()->AddObserver(this);
You never remove this observer.

http://codereview.chromium.org/10116011/diff/6001/ash/test/test_launcher_dele...
ash/test/test_launcher_delegate.cc:88: 
nit: only one newline.

[dcheng, 2012-04-19 20:18:15]

http://codereview.chromium.org/10116011/diff/6001/ash/launcher/launcher_view_...
File ash/launcher/launcher_view_unittest.cc (right):

http://codereview.chromium.org/10116011/diff/6001/ash/launcher/launcher_view_...
ash/launcher/launcher_view_unittest.cc:4: 
On 2012/04/19 00:05:40, sky wrote:
> tests should follow that of non-test cc files and include the header for the
> file being tested first.

Done.

http://codereview.chromium.org/10116011/diff/6001/ash/launcher/launcher_view_...
ash/launcher/launcher_view_unittest.cc:71: ash::test::TestLauncherDelegate*
launcher_delegate =
On 2012/04/19 00:05:40, sky wrote:
> Instead of this, how about a static instance method on TestLauncherDelegate
and
> an ASSERT(launcher_delegate). That's less fragile and easy to know if
something
> fails.

Done. Unfortunately ASSERT_NE(NULL, launcher_delegate) doesn't work so I had to
use ASSERT_TRUE.

http://codereview.chromium.org/10116011/diff/6001/ash/test/test_launcher_dele...
File ash/test/test_launcher_delegate.cc (right):

http://codereview.chromium.org/10116011/diff/6001/ash/test/test_launcher_dele...
ash/test/test_launcher_delegate.cc:66: void
TestLauncherDelegate::AddLauncherItem(aura::Window* window) {
On 2012/04/19 00:05:40, sky wrote:
> Position should match header.

Done.

http://codereview.chromium.org/10116011/diff/6001/ash/test/test_launcher_dele...
ash/test/test_launcher_delegate.cc:69: window_to_id_[window] =
model_->next_id();
On 2012/04/19 00:05:40, sky wrote:
> How about a DCHECK(window_to_id_.find(window) == window_to_id_.end())

Done.

http://codereview.chromium.org/10116011/diff/6001/ash/test/test_launcher_dele...
ash/test/test_launcher_delegate.cc:74: window->parent()->AddObserver(this);
On 2012/04/19 00:05:40, sky wrote:
> You never remove this observer.

Done.

http://codereview.chromium.org/10116011/diff/6001/ash/test/test_launcher_dele...
ash/test/test_launcher_delegate.cc:88: 
On 2012/04/19 00:05:40, sky wrote:
> nit: only one newline.

Done.

[sky, 2012-04-19 21:44:30]

LGTM

http://codereview.chromium.org/10116011/diff/13001/ash/test/test_launcher_del...
File ash/test/test_launcher_delegate.h (right):

http://codereview.chromium.org/10116011/diff/13001/ash/test/test_launcher_del...
ash/test/test_launcher_delegate.h:45: static TestLauncherDelegate* GetInstance()
{ return instance_; }
nit: move this above overriden methods and name it instance().

[dcheng, 2012-04-19 21:58:02]

Fixed the build as well. I didn't realize chrome_tests.gypi referenced ash
stuff...

http://codereview.chromium.org/10116011/diff/13001/ash/test/test_launcher_del...
File ash/test/test_launcher_delegate.h (right):

http://codereview.chromium.org/10116011/diff/13001/ash/test/test_launcher_del...
ash/test/test_launcher_delegate.h:45: static TestLauncherDelegate* GetInstance()
{ return instance_; }
On 2012/04/19 21:44:31, sky wrote:
> nit: move this above overriden methods and name it instance().

Done.