Fix possible overlapping icons in shelf on sync.

Fix possible overlapping icons in shelf on sync.

This fixes overlapping icons in shelf on sync, refactored pin applying
functionality.

BUG=649717
BUG=b/31632644
TEST=unit_tests
TEST=Manually on device, no glitches with shelf icon were found.

Committed: https://crrev.com/57990fca3140d334ac17b2ba2c1412b5b51ee93d
Cr-Commit-Position: refs/heads/master@{#421613}

[khmel, 2016-09-23 16:50:33]

khmel@chromium.org changed reviewers:
+ oshima@chromium.org, skuhne@chromium.org

[khmel, 2016-09-23 16:50:34]

Hi Stefan and Oshima,

PTAL

https://codereview.chromium.org/2364983002/diff/1/ash/common/shelf/shelf_view.cc
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2364983002/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:1713: bounds_animator_->AnimateViewTo(new_view,
old_ideal_bounds);
We drop animation on line 1699 but not always recreate new. That caused
situation when item was left in position from unfinished animation.

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1234: for
(const auto& pref_app_id : pinned_apps) {
I refactored pin applying code. It was hard to debug and analyze due multi
indexes and refs. I tried to make it simpler and straightforward. I also discard
special Chrome handling that is not required because we always have it in pin
list. This algorithm also reduces number of pin recreation.

[Mr4D (OOO till 08-26), 2016-09-25 03:21:29]

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1234: for
(const auto& pref_app_id : pinned_apps) {
I remember that when I added this, it was a very hairy thing which caused loads
of special cases: Lists get synced among different systems, not all systems
might have all apps, apps might update (in which case they might go away and
come back), ...

I did create a lot of unit tests to test this. I suppose they still run through
without problems?

Last but not least - it might of course be that requirements have changed.

[khmel, 2016-09-26 15:15:55]

Hi Stefan,

Thanks for taking a look, please find my comment.

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1234: for
(const auto& pref_app_id : pinned_apps) {
On 2016/09/25 03:21:29, Mr4D wrote:
> I remember that when I added this, it was a very hairy thing which caused
loads
> of special cases: Lists get synced among different systems, not all systems
> might have all apps, apps might update (in which case they might go away and
> come back), ...
> 
> I did create a lot of unit tests to test this. I suppose they still run
through
> without problems?
> 
> Last but not least - it might of course be that requirements have changed.

Yes, unit_tests was my main tool to validate correctness. I also passed  set of
browser_tests. You might remember that pins now are part of app sync model, no
longer based on prefs and this code was lightly refactored during that change.
If you think that risk of this part is unacceptable I can discard this part of
CL. Bug as it sounds, is fixed by the change in shelf view.

[Mr4D (OOO till 08-26), 2016-09-26 21:59:54]

lgtm

[oshima, 2016-09-27 23:27:45]

lgtm

sorry for delay. It's been a while since I looked at the code, and took time to
go through.

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1231: // Apply
pins in two steps. At the first step, go througp the list of apps to
nit: through

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1266:
DCHECK(model_->ItemIndexByID(item.id) == index);
nit: DCHECK_EQ

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1286:
DCHECK(controller->app_id() != extension_misc::kChromeAppId);
nit: DCHECK_NE

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1292:
DCHECK(model_->ItemIndexByID(item.id) >= index);
nit: DCHECK_GE

[khmel, 2016-09-28 19:26:20]

Thank you for review!

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1231: // Apply
pins in two steps. At the first step, go througp the list of apps to
On 2016/09/27 23:27:45, oshima wrote:
> nit: through

Done.

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1266:
DCHECK(model_->ItemIndexByID(item.id) == index);
On 2016/09/27 23:27:45, oshima wrote:
> nit: DCHECK_EQ

Done.

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1286:
DCHECK(controller->app_id() != extension_misc::kChromeAppId);
On 2016/09/27 23:27:44, oshima wrote:
> nit: DCHECK_NE

Done.

https://codereview.chromium.org/2364983002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1292:
DCHECK(model_->ItemIndexByID(item.id) >= index);
On 2016/09/27 23:27:45, oshima wrote:
> nit: DCHECK_GE

Done.

[khmel, 2016-09-28 19:26:52]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-09-28 19:26:52]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/2364983002/#ps20001
(title: "nits")

[commit-bot: I haz the power, 2016-09-28 19:27:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 19:55:39]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-28 19:59:29]

Description was changed from

==========
Fix possible overlapping icons in shelf on sync.

This fixes overlapping icons in shelf on sync, refactored pin applying
functionality.

BUG=649717
BUG=b/31632644
TEST=unit_tests
TEST=Manually on device, no glitches with shelf icon were found.
==========

to

==========
Fix possible overlapping icons in shelf on sync.

This fixes overlapping icons in shelf on sync, refactored pin applying
functionality.

BUG=649717
BUG=b/31632644
TEST=unit_tests
TEST=Manually on device, no glitches with shelf icon were found.

Committed: https://crrev.com/57990fca3140d334ac17b2ba2c1412b5b51ee93d
Cr-Commit-Position: refs/heads/master@{#421613}
==========

[commit-bot: I haz the power, 2016-09-28 19:59:30]

Patchset 2 (id:??) landed as
https://crrev.com/57990fca3140d334ac17b2ba2c1412b5b51ee93d
Cr-Commit-Position: refs/heads/master@{#421613}