ash: Fix launcher icon overlaps with status.

ash: Fix launcher icon overlaps with status.

- Make LauncherView::CalculateIdealBounds to return last visible index;
- In LauncherView::LauncherItemAdded, use the last visible index to determine if
  we need the animation;

BUG=122482
TEST=Verify fix for issue 122482.


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

[xiyuan, 2012-04-12 21:56:28]

The overlapping and problems in insertion/removal are related to using
overflow_button_'s visibility to change item view visiblity in
CalculateIdealBounds. Once overflow button is shown, we never change the item
view's visibility and causing problems.

e.g. the overlapping problem is because LauncherView's first layout is have a
size of 8px and thus overflow_button_ is visible. And from that on, we don't
hide item views that should be hidden.

This CL makes CalculateIdealBounds to return last_visible_index and use that to
check in LauncherItemAdded and LauncherItemRemoved. And also make
CalculateIdealBounds not to change item view's visibility directly but set an
ideal_visibility flag in ViewModel and let the caller to do the visibility
update on item views.

[sky, 2012-04-12 22:23:53]

Can you wait for http://codereview.chromium.org/9808026/ to land. I think it'll
result in changing around your patch a bit. If you want to incorporate the
relevant bits into your patch and update that's fine too.

I would also like to see some tests for this behavior.

[xiyuan, 2012-04-12 22:39:32]

On 2012/04/12 22:23:53, sky wrote:
> Can you wait for http://codereview.chromium.org/9808026/ to land. I think
it'll
> result in changing around your patch a bit. If you want to incorporate the
> relevant bits into your patch and update that's fine too.
> 
> I would also like to see some tests for this behavior.

Okay. I'll merge after http://codereview.chromium.org/9808026/ lands and add
some tests.

[xiyuan, 2012-04-13 21:37:18]

CL updated:
- Add last_visible_index_ as a member as in CL 9808026;
- Fixed a couple problems in original CL
  - Fix an out-of-bounds problem in set-ideal-visiblity for-loop in
CalculateIdeaBounds;
  - Reverted to always run AnimateToIdealBounds in LauncherItemAdded as we need
it to set overflow button position;
- Added LauncherViewTest;

I'll still wait for 9808026. But want to keep you posted.  :)

[sky, 2012-04-13 22:00:04]

http://codereview.chromium.org/10068027/diff/12002/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

http://codereview.chromium.org/10068027/diff/12002/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:735: // Hides view that should become invisible
immediately.
If possible, I would rather change the visibility immediately. It's less error
prone and simpler.

How about changing CalculateIdealBounds to always change the visibility, and
then change this method so that it resets the visibility after calculate? I
believe that would mean no need to cache the ideal visibility and you could
remove this and the same from remove.

http://codereview.chromium.org/10068027/diff/12002/ash/launcher/launcher_view.h
File ash/launcher/launcher_view.h (right):

http://codereview.chromium.org/10068027/diff/12002/ash/launcher/launcher_view...
ash/launcher/launcher_view.h:44: class ASH_EXPORT TestAPI {
Can you move this into a separate file and make it a friend?

[xiyuan, 2012-04-13 22:49:53]

http://codereview.chromium.org/10068027/diff/12002/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

http://codereview.chromium.org/10068027/diff/12002/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:735: // Hides view that should become invisible
immediately.
On 2012/04/13 22:00:04, sky wrote:
> If possible, I would rather change the visibility immediately. It's less error
> prone and simpler.
> 
> How about changing CalculateIdealBounds to always change the visibility, and
> then change this method so that it resets the visibility after calculate? I
> believe that would mean no need to cache the ideal visibility and you could
> remove this and the same from remove.

This causes flashes. When CalculateIdealBounds sets it to visible, it is visible
immediately. Reset hides it and then it shows up again after animation. :(

[xiyuan, 2012-04-23 18:08:52]

CL updated:
- Use opacity 0 instead of SetVisible(false) in LauncherItemAdded as suggested
seems working fine with no flashes and allows us to change visibility directly
in CalculateIdealBounds;
- ideal visibility is removed from ViewModel;
- TestAPI is moved into its own file;

Please take another look. Thanks.

http://codereview.chromium.org/10068027/diff/12002/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

http://codereview.chromium.org/10068027/diff/12002/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:735: // Hides view that should become invisible
immediately.
Done. Set opacity to 0 work as expected and thus removed ideal_visibility etc
from ViewModel.

http://codereview.chromium.org/10068027/diff/12002/ash/launcher/launcher_view.h
File ash/launcher/launcher_view.h (right):

http://codereview.chromium.org/10068027/diff/12002/ash/launcher/launcher_view...
ash/launcher/launcher_view.h:44: class ASH_EXPORT TestAPI {
On 2012/04/13 22:00:04, sky wrote:
> Can you move this into a separate file and make it a friend?

Done.

[sky, 2012-04-23 20:47:24]

LGTM

http://codereview.chromium.org/10068027/diff/16001/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/10068027/diff/16001/ash/ash.gyp#newcode351
ash/ash.gyp:351: 'test/launcher_view_test_api.cc',
sort

http://codereview.chromium.org/10068027/diff/16001/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

http://codereview.chromium.org/10068027/diff/16001/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:660: view->layer()->SetOpacity(0);
Add a comment as to why opacity is used here.

http://codereview.chromium.org/10068027/diff/16001/ash/launcher/launcher_view...
File ash/launcher/launcher_view_unittest.cc (right):

http://codereview.chromium.org/10068027/diff/16001/ash/launcher/launcher_view...
ash/launcher/launcher_view_unittest.cc:7: #include
"ash/launcher/launcher_view.h"
launcher_view should be the first include (test files should have same include
order as non-test .cc files).

http://codereview.chromium.org/10068027/diff/16001/ash/test/launcher_view_tes...
File ash/test/launcher_view_test_api.cc (right):

http://codereview.chromium.org/10068027/diff/16001/ash/test/launcher_view_tes...
ash/test/launcher_view_test_api.cc:75: // This nest loop will quit when
TestAPIAnimationObserver's
nested

[xiyuan, 2012-04-23 21:01:20]

All done and will have fun rebasing now. :)

http://codereview.chromium.org/10068027/diff/16001/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/10068027/diff/16001/ash/ash.gyp#newcode351
ash/ash.gyp:351: 'test/launcher_view_test_api.cc',
On 2012/04/23 20:47:25, sky wrote:
> sort

Done.

http://codereview.chromium.org/10068027/diff/16001/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

http://codereview.chromium.org/10068027/diff/16001/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:660: view->layer()->SetOpacity(0);
On 2012/04/23 20:47:25, sky wrote:
> Add a comment as to why opacity is used here.

Done.

http://codereview.chromium.org/10068027/diff/16001/ash/launcher/launcher_view...
File ash/launcher/launcher_view_unittest.cc (right):

http://codereview.chromium.org/10068027/diff/16001/ash/launcher/launcher_view...
ash/launcher/launcher_view_unittest.cc:7: #include
"ash/launcher/launcher_view.h"
On 2012/04/23 20:47:25, sky wrote:
> launcher_view should be the first include (test files should have same include
> order as non-test .cc files).

Done.

http://codereview.chromium.org/10068027/diff/16001/ash/test/launcher_view_tes...
File ash/test/launcher_view_test_api.cc (right):

http://codereview.chromium.org/10068027/diff/16001/ash/test/launcher_view_tes...
ash/test/launcher_view_test_api.cc:75: // This nest loop will quit when
TestAPIAnimationObserver's
On 2012/04/23 20:47:25, sky wrote:
> nested

Done.

[commit-bot: I haz the power, 2012-04-23 23:11:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/10068027/31001

[commit-bot: I haz the power, 2012-04-24 02:08:56]

Change committed as 133603