Don't set shownRatio in main thread top controls on animated state change.

Don't set shownRatio in main thread top controls on animated state change.

In r399036 I made the main thread set the shown ratio as soon as a state change
came in so that the updated window height would be available to script
immediately. This causes problems when the state change is animated since the
compositor thread will try to animate to the state and it'll fight with the
commited value coming from the main thread.

This CL simply disables the main thread behavior when the state change should be
animated, relying on the compositor to drive the animation.

BUG=623183
Committed: https://crrev.com/849f8df4aa51c333cd14286b51673acc90e9b862
Cr-Commit-Position: refs/heads/master@{#402867}

[bokan, 2016-06-27 22:12:20]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-27 22:12:56]

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

[bokan, 2016-06-27 22:12:56]

Description was changed from

==========
Don't set shownRatio in main thread top controls on animated state change.

In r401588 I made the main thread set the shown ratio as soon as a state change
came in so that the updated window height would be available to script
immediately. This causes problems when the state change is animated since the
compositor thread will try to animate to the state and it'll fight with the
commited value coming from the main thread.

This CL simply disables the main thread behavior when the state change should be
animated, relying on the compositor to drive the animation.

BUG=623183
==========

to

==========
Don't set shownRatio in main thread top controls on animated state change.

In r401588 I made the main thread set the shown ratio as soon as a state change
came in so that the updated window height would be available to script
immediately. This causes problems when the state change is animated since the
compositor thread will try to animate to the state and it'll fight with the
commited value coming from the main thread.

This CL simply disables the main thread behavior when the state change should be
animated, relying on the compositor to drive the animation.

BUG=623183
==========

[bokan, 2016-06-27 22:12:57]

bokan@chromium.org changed reviewers:
+ majidvp@chromium.org

[bokan, 2016-06-27 22:13:03]

Majid, ptal.

[commit-bot: I haz the power, 2016-06-28 01:37:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 01:37:20]

Dry run: This issue passed the CQ dry run.

[majidvp, 2016-06-28 19:52:20]

Your CL description refers to the wrong commit. I think you meant r399036.

https://codereview.chromium.org/2105523002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2105523002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:1842:
topControls().setShownRatio(0.f);
Maybe it is worth it to specifically call out the TopControlsBoth case is a
no-op.
Perhaps this should become a switch statement instead?

https://codereview.chromium.org/2105523002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:1843: }
Why move the clamping logic out of top controls? I think this is internal logic
to TopControls and it is better to pass in animate boolean to the method
instead.

[bokan, 2016-06-28 19:53:52]

Description was changed from

==========
Don't set shownRatio in main thread top controls on animated state change.

In r401588 I made the main thread set the shown ratio as soon as a state change
came in so that the updated window height would be available to script
immediately. This causes problems when the state change is animated since the
compositor thread will try to animate to the state and it'll fight with the
commited value coming from the main thread.

This CL simply disables the main thread behavior when the state change should be
animated, relying on the compositor to drive the animation.

BUG=623183
==========

to

==========
Don't set shownRatio in main thread top controls on animated state change.

In r399036 I made the main thread set the shown ratio as soon as a state change
came in so that the updated window height would be available to script
immediately. This causes problems when the state change is animated since the
compositor thread will try to animate to the state and it'll fight with the
commited value coming from the main thread.

This CL simply disables the main thread behavior when the state change should be
animated, relying on the compositor to drive the animation.

BUG=623183
==========

[bokan, 2016-06-28 23:15:33]

https://codereview.chromium.org/2105523002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2105523002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:1842:
topControls().setShownRatio(0.f);
On 2016/06/28 19:52:20, majidvp wrote:
> Maybe it is worth it to specifically call out the TopControlsBoth case is a
> no-op.
> Perhaps this should become a switch statement instead?

Done.

https://codereview.chromium.org/2105523002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:1843: }
On 2016/06/28 19:52:20, majidvp wrote:
> Why move the clamping logic out of top controls? I think this is internal
logic
> to TopControls and it is better to pass in animate boolean to the method
> instead.

Done.

[bokan, 2016-06-28 23:15:46]

On 2016/06/28 19:52:20, majidvp wrote:
> Your CL description refers to the wrong commit. I think you meant r399036.

Whoops. Fixed.

[majidvp, 2016-06-29 13:56:09]

https://codereview.chromium.org/2105523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/TopControls.cpp (right):

https://codereview.chromium.org/2105523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/TopControls.cpp:109: switch (current) {
Sorry to go back and forth and not noticing this earlier but this should
actually be |constraints| not the |current|. In fact, |current| is always |Both|
which makes it a no-op.

In CC we make the decision based on both and assert that they cannot be
contradictory. Perhaps we should just replicate the same logic here and not
assume current is always BOTH.

[bokan, 2016-06-29 16:23:01]

https://codereview.chromium.org/2105523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/TopControls.cpp (right):

https://codereview.chromium.org/2105523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/TopControls.cpp:109: switch (current) {
On 2016/06/29 13:56:09, majidvp wrote:
> Sorry to go back and forth and not noticing this earlier but this should
> actually be |constraints| not the |current|. In fact, |current| is always
|Both|
> which makes it a no-op.
> 
> In CC we make the decision based on both and assert that they cannot be
> contradictory. Perhaps we should just replicate the same logic here and not
> assume current is always BOTH.
> 

Good catch, I hadn't noticed this doesn't fix the race it was meant for since it
apparently doesn't occur in Debug builds. I've fixed this to mirror CC more
closely.

[bokan, 2016-06-29 16:23:13]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 16:23:44]

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

[majidvp, 2016-06-29 16:50:58]

On 2016/06/29 16:23:44, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

lgtm.

[commit-bot: I haz the power, 2016-06-29 17:42:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-29 17:42:38]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-06-29 17:45:06]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-06-29 17:46:14]

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-06-29 17:57:53]

Description was changed from

==========
Don't set shownRatio in main thread top controls on animated state change.

In r399036 I made the main thread set the shown ratio as soon as a state change
came in so that the updated window height would be available to script
immediately. This causes problems when the state change is animated since the
compositor thread will try to animate to the state and it'll fight with the
commited value coming from the main thread.

This CL simply disables the main thread behavior when the state change should be
animated, relying on the compositor to drive the animation.

BUG=623183
==========

to

==========
Don't set shownRatio in main thread top controls on animated state change.

In r399036 I made the main thread set the shown ratio as soon as a state change
came in so that the updated window height would be available to script
immediately. This causes problems when the state change is animated since the
compositor thread will try to animate to the state and it'll fight with the
commited value coming from the main thread.

This CL simply disables the main thread behavior when the state change should be
animated, relying on the compositor to drive the animation.

BUG=623183
==========

[commit-bot: I haz the power, 2016-06-29 17:57:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-29 17:58:19]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 17:59:18]

Description was changed from

==========
Don't set shownRatio in main thread top controls on animated state change.

In r399036 I made the main thread set the shown ratio as soon as a state change
came in so that the updated window height would be available to script
immediately. This causes problems when the state change is animated since the
compositor thread will try to animate to the state and it'll fight with the
commited value coming from the main thread.

This CL simply disables the main thread behavior when the state change should be
animated, relying on the compositor to drive the animation.

BUG=623183
==========

to

==========
Don't set shownRatio in main thread top controls on animated state change.

In r399036 I made the main thread set the shown ratio as soon as a state change
came in so that the updated window height would be available to script
immediately. This causes problems when the state change is animated since the
compositor thread will try to animate to the state and it'll fight with the
commited value coming from the main thread.

This CL simply disables the main thread behavior when the state change should be
animated, relying on the compositor to drive the animation.

BUG=623183

Committed: https://crrev.com/849f8df4aa51c333cd14286b51673acc90e9b862
Cr-Commit-Position: refs/heads/master@{#402867}
==========

[commit-bot: I haz the power, 2016-06-29 17:59:19]

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