Fix folder icon animation in app list.

Fix folder icon animation in app list.

This CL fixes the animation of folders opening in the app list. This
regressed in r301308 because the ContentsView layer moved, exposing
a latent issue with the calculation of the top icon animation target.

The fix here is to use the view bounds as the target rather than the
layer bounds. The layer bounds are in the coordinate system of the
parent layer which is not what we want.

BUG=429079
Committed: https://crrev.com/dd661f30d65756a59f48670749bd7d1426714aaf
Cr-Commit-Position: refs/heads/master@{#302579}

[calamity, 2014-11-03 05:45:05]

Patchset #1 (id:1) has been deleted

[calamity, 2014-11-03 05:46:32]

calamity@chromium.org changed reviewers:
+ mgiuca@chromium.org

[calamity, 2014-11-03 05:46:32]

This needs to be fixed by M40 and should take review priority.

[Matt Giuca, 2014-11-04 01:05:18]

Looks good, but please split this up into two CLs:
1. fix the bug (remove "layer()->").
2. refactor (especially since the refactor involves changing some quite
unrelated code; it shouldn't be part of an app list bug fix).

It is a nice refactor.

https://codereview.chromium.org/694743002/diff/20001/ui/gfx/transform_util.cc
File ui/gfx/transform_util.cc (right):

https://codereview.chromium.org/694743002/diff/20001/ui/gfx/transform_util.cc...
ui/gfx/transform_util.cc:288: transform.Scale(static_cast<double>(to.width()) /
from.width(),
Going to have infinities if from.width() or from.height() is 0. I don't know
what will happen in that case but it's probably not an intended mode of
operation (and certainly won't get the intended result).

DCHECK_NE(0, from.width());
DCHECK_NE(0, from.height());

and document that from's width and height must be non-zero.

(Also think about what happens if width and height are negative; maybe just
DCHECK that too.)

https://codereview.chromium.org/694743002/diff/20001/ui/gfx/transform_util.h
File ui/gfx/transform_util.h (right):

https://codereview.chromium.org/694743002/diff/20001/ui/gfx/transform_util.h#...
ui/gfx/transform_util.h:19: // Returns a translate and scale transform which
transforms from one rect to
Drop the "translate and scale" --- that's an implementation detail. The key is
that it transforms one rect to another.

https://codereview.chromium.org/694743002/diff/20001/ui/gfx/transform_util_un...
File ui/gfx/transform_util_unittest.cc (right):

https://codereview.chromium.org/694743002/diff/20001/ui/gfx/transform_util_un...
ui/gfx/transform_util_unittest.cc:7: #include "base/logging.h"
Don't think you need this.

https://codereview.chromium.org/694743002/diff/20001/ui/gfx/transform_util_un...
ui/gfx/transform_util_unittest.cc:36: TEST(TransformUtilTest, GetRectTransform)
{
Good test.

[calamity, 2014-11-04 01:40:30]

It's been split in the reverse order that you posted.

I am skeptical of the value of splitting this up. I think that reviews should be
split so that there is a clear purpose and the review is manageable.

The only benefit I see is making an already simple revert 1 degree easier in the
off-chance that someone else uses GetRectTransform().

OTOH, there is now more indirection in ascertaining the purpose of
GetRectTransform() via blame and someone looking at this CL would be more likely
to think GetRectTransform() was pre-existing, hardened code.

https://codereview.chromium.org/694743002/diff/20001/ui/gfx/transform_util.cc
File ui/gfx/transform_util.cc (right):

https://codereview.chromium.org/694743002/diff/20001/ui/gfx/transform_util.cc...
ui/gfx/transform_util.cc:288: transform.Scale(static_cast<double>(to.width()) /
from.width(),
On 2014/11/04 01:05:17, Matt Giuca wrote:
> Going to have infinities if from.width() or from.height() is 0. I don't know
> what will happen in that case but it's probably not an intended mode of
> operation (and certainly won't get the intended result).
> 
> DCHECK_NE(0, from.width());
> DCHECK_NE(0, from.height());
> 
> and document that from's width and height must be non-zero.
> 
> (Also think about what happens if width and height are negative; maybe just
> DCHECK that too.)

Done.

https://codereview.chromium.org/694743002/diff/20001/ui/gfx/transform_util.h
File ui/gfx/transform_util.h (right):

https://codereview.chromium.org/694743002/diff/20001/ui/gfx/transform_util.h#...
ui/gfx/transform_util.h:19: // Returns a translate and scale transform which
transforms from one rect to
On 2014/11/04 01:05:17, Matt Giuca wrote:
> Drop the "translate and scale" --- that's an implementation detail. The key is
> that it transforms one rect to another.

Done.

https://codereview.chromium.org/694743002/diff/20001/ui/gfx/transform_util_un...
File ui/gfx/transform_util_unittest.cc (right):

https://codereview.chromium.org/694743002/diff/20001/ui/gfx/transform_util_un...
ui/gfx/transform_util_unittest.cc:7: #include "base/logging.h"
On 2014/11/04 01:05:18, Matt Giuca wrote:
> Don't think you need this.

Done.

[Matt Giuca, 2014-11-04 03:00:17]

OK, the new split looks good.

As discussed, the GetRectTransform code is good but the usage of it (which has
been removed from this CL) is broken.

The current scaling logic is mental and I have no idea how it works (and should
be made sane in the follow-up), but for now, this particular CL does not change
the scaling logic and fixes the bug, so lgtm.

[calamity, 2014-11-04 05:07:57]

Added a comment indicating what the transform is relative to.

[calamity, 2014-11-04 05:12:57]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-11-04 05:13:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/694743002/80001

[commit-bot: I haz the power, 2014-11-04 06:04:17]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-11-04 06:04:56]

Patchset 4 (id:??) landed as
https://crrev.com/dd661f30d65756a59f48670749bd7d1426714aaf
Cr-Commit-Position: refs/heads/master@{#302579}