Simplify HomeCardGestureManager::UpdateScrollState()

Simplify HomeCardGestureManager::UpdateScrollState()

This CL:
- Prevents HomeCardGestureManager from calling OnGestureProgressed() with
  |from_state| == |to_state|
  - This fixes the minimized home card's opacity being incorrectly set when
    dragging down from the minimized state
- Removes unnecessary |HomeCardGestureManager::last_state_|

BUG=409666
TEST=HomeCardGestureManagerTest.*

Committed: https://crrev.com/eaf470240e7579c5036b5ffb184815dcdd02f53f
Cr-Commit-Position: refs/heads/master@{#293232}

[pkotwicz, 2014-09-01 16:24:30]

pkotwicz@chromium.org changed reviewers:
+ mukai@chromium.org

[pkotwicz, 2014-09-01 16:24:31]

mukai@ PTAL

[Jun Mukai, 2014-09-02 03:51:43]

Also, please write the tests. I believe this could be captured in
home_card_gesture_manager_unittest.cc

https://codereview.chromium.org/529813002/diff/1/athena/home/home_card_gestur...
File athena/home/home_card_gesture_manager.h (right):

https://codereview.chromium.org/529813002/diff/1/athena/home/home_card_gestur...
athena/home/home_card_gesture_manager.h:31: // |from_state| may be equal to
|to_state| but only if
I think from_state could be equal to to_state and CENTERED when the user drag-up
from CENTERED state.

https://codereview.chromium.org/529813002/diff/1/athena/home/home_card_impl.cc
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/529813002/diff/1/athena/home/home_card_impl.c...
athena/home/home_card_impl.cc:160: GetFocusManager()->ClearFocus();
Not sure why how this change is related to the bug.

https://codereview.chromium.org/529813002/diff/1/athena/home/home_card_impl.c...
athena/home/home_card_impl.cc:405: opacity = (from_state == VISIBLE_MINIMIZED) ?
1.0f - progress : progress;
Can you fix home_card_gesture_manager side in this case?  That could be a recent
regression.

[pkotwicz, 2014-09-02 04:41:32]

Mukai, can you please take another look?

https://codereview.chromium.org/529813002/diff/1/athena/home/home_card_gestur...
File athena/home/home_card_gesture_manager.h (right):

https://codereview.chromium.org/529813002/diff/1/athena/home/home_card_gestur...
athena/home/home_card_gesture_manager.h:31: // |from_state| may be equal to
|to_state| but only if
With the current implementation of UpdateScrollState(), it does not seem
possible for both |from_state| and |to_state| to be CENTERED. Note line 97 in
home_card_gesture_manager.cc
VISIBLE_MINIMIZED > VISIBLE_BOTTOM > VISIBLE_CENTERED > HIDDEN

https://codereview.chromium.org/529813002/diff/1/athena/home/home_card_impl.cc
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/529813002/diff/1/athena/home/home_card_impl.c...
athena/home/home_card_impl.cc:160: GetFocusManager()->ClearFocus();
This change is related to this CL given that if |from_state| is never equal to
|to_state|, then the if statement can be removed.

https://codereview.chromium.org/529813002/diff/1/athena/home/home_card_impl.c...
athena/home/home_card_impl.cc:405: opacity = (from_state == VISIBLE_MINIMIZED) ?
1.0f - progress : progress;
I have now fixed this in home_card_gesture_manager.cc In particular,
|from_state| is never equal to |to_state| anymore.

It is slightly confusing that we set |from_state| to an arbitrary value if
|from_state| would otherwise be equal to |to_state|. However, it makes code
simpler.

[Jun Mukai, 2014-09-02 08:22:42]

Hmmm, sorry, my comment was wrong and fixing on home_card_gesture_manager
doesn't look better...

It is natural to have both 'from_state' and 'to_state' MINIMIZED for swipe-down
from MINIMIZED state... and also, it is natural to have both CENTERED for
swipe-up from CENTERED.

That means, the fix should be in home_card_impl.cc, sorry for my mis-comments.

https://codereview.chromium.org/529813002/diff/1/athena/home/home_card_impl.cc
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/529813002/diff/1/athena/home/home_card_impl.c...
athena/home/home_card_impl.cc:160: GetFocusManager()->ClearFocus();
On 2014/09/02 04:41:32, pkotwicz wrote:
> This change is related to this CL given that if |from_state| is never equal to
> |to_state|, then the if statement can be removed.

Hmm, that sounds a bug then.
In case the user drag up from CENTERED state, I think that's not a trigger to
close the virtual keyboard by itself. When the user drag *down*, on the other
hand, the virtual keyboard should be closed though.

So, I think we should not remove this condition but modify
home_card_gesture_manager.cc to achieve both |from_state| and |to_state| to be
CENTERED in such case.

https://codereview.chromium.org/529813002/diff/20001/athena/home/home_card_im...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/529813002/diff/20001/athena/home/home_card_im...
athena/home/home_card_impl.cc:397: (from_state == VISIBLE_MINIMIZED) ? 1.0f -
progress : progress;
Instead this could be rewritten as:
opacity = (to_state == VISIBLE_MINIMIZED) ? progress : 1.0f - progress;
?

[pkotwicz, 2014-09-03 03:41:51]

mukai@, can you please take another look?

I thought about this some more. The names |from_state| and |to_state| are
confusing. With or without this CL, the home card may never have been in
|from_state|. The old code also allowed the possibility of |from_state| being
VISIBLE_MINIMIZED and |to_state| being VISIBLE_CENTERED. (It is unclear how
|progress| would be computed in such a case)
I came to the conclusion that it is a bad idea to attempt to make |from_state|
and |to_state| make intuitive sense and to focus on code simplicity instead. (I
unfortunately could not think of better parameter names than |from_state| and
|to_state|)

- I could swap |from_state| and |to_state| based on the direction of the most
recent scroll gesture. (I don't think it simplifies any logic and the home card
may never have been in |from_state| anyways.)
- It would be possible to set |from_state|  and |to_state| to the same value
when |progress| == 1. This would also apply to VISIBLE_BOTTOM. I do not see much
point in doing this though.

w.r.t. to dragging up from the VISIBLE_CENTERED state, code can simply:
if (to_state == VISIBLE_CENTERED && progress < 1.0f) {
  // Close virtual keyboard  
}

https://codereview.chromium.org/529813002/diff/40001/athena/home/home_card_ge...
File athena/home/home_card_gesture_manager.cc (left):

https://codereview.chromium.org/529813002/diff/40001/athena/home/home_card_ge...
athena/home/home_card_gesture_manager.cc:100: progress = 1.0f - progress;
Previously when scrolling down and |last_estimated_height_| > kHomeCardHeight,
|from_state| and |to_state| would swap each time that OnGestureProgressed() was
called.

[pkotwicz, 2014-09-03 03:46:58]

Correction:

if (from_state == VISIBLE_CENTERED ||
    (to_state == VISIBLE_CENTERED && progress < 1.0f) {
  // Close virtual keyboard
}

[Jun Mukai, 2014-09-03 07:16:49]

lgtm

Great, thanks.
I also imagine that the name 'from_' and 'to_' are misleading, it's really
appreciated if you could choose better names.

https://codereview.chromium.org/529813002/diff/40001/athena/home/home_card_ge...
File athena/home/home_card_gesture_manager_unittest.cc (right):

https://codereview.chromium.org/529813002/diff/40001/athena/home/home_card_ge...
athena/home/home_card_gesture_manager_unittest.cc:110:
EXPECT_EQ(HomeCard::VISIBLE_MINIMIZED, HomeCard::Get()->GetState());
This might be better to be ASSERT_EQ I guess (and in other tests you've added)

[pkotwicz, 2014-09-03 20:32:07]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2014-09-03 20:32:11]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2014-09-03 20:33:01]

I did not change the parameter names because I could not think of better names.
I'll ask around and maybe someone will have some good names

[pkotwicz, 2014-09-03 20:33:02]

I did not change the parameter names because I could not think of better names.
I'll ask around and maybe someone will have some good names

[pkotwicz, 2014-09-03 20:33:05]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-03 20:34:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/529813002/60001

[pkotwicz, 2014-09-03 21:39:31]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2014-09-03 21:44:42]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-03 23:23:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/529813002/60001

[commit-bot: I haz the power, 2014-09-04 00:17:37]

Committed patchset #4 (id:60001) as c60e2769f9d1c18464a308c8dfca592384d19364

[commit-bot: I haz the power, 2014-09-10 03:28:55]

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