Hide/show topbar when scrolling page up or down.

Hide/show topbar when scrolling page up or down.

Topbar was not hiding while scrolling down at some pages
starting with a table. It did show up however if previously
hidden when scrolling up. With this change, topbar animation is
triggered for both up & down scrolling.

See for example http://sv.wikipedia.org/wiki/M%C3%B6lnlycke and try
scrolling down, topbar is not hidden.

BUG=438548
Committed: https://crrev.com/01d84d33126f4162c9bd0c7c7f9a2e5970ea2e73
Cr-Commit-Position: refs/heads/master@{#308340}

[elisabets, 2014-12-02 14:27:18]

elisabets@opera.com changed reviewers:
+ aelias@chromium.org, enne@chromium.org, jdduke@chromium.org, joi@chromium.org,
sky@chromium.org

[elisabets, 2014-12-02 14:28:32]

Made a smaller change in top bar behaviour, top bar should consume scroll event
for scrolling both up & down.

[elisabets, 2014-12-02 14:32:58]

On 2014/12/02 14:28:32, elisabets wrote:
> Made a smaller change in top bar behaviour, top bar should consume scroll
event
> for scrolling both up & down.

I see now that I might should have changed the comment above the code.

A question I have as well, is there any cases where scroll = 0? If not, what is
then the purpose of this function?

[jonab, 2014-12-02 15:31:32]

Note that it reproduces only when pan scrolling the page in chrome, when
fling-scrolling, the topbar hides.

[sky, 2014-12-02 16:20:03]

sky@chromium.org changed reviewers:
- sky@chromium.org

[sky, 2014-12-02 16:20:18]

I'm not a good reviewer for this.
-sky

[aelias_OOO_until_Jul13, 2014-12-02 19:44:11]

aelias@chromium.org changed reviewers:
- enne@chromium.org, joi@chromium.org

[aelias_OOO_until_Jul13, 2014-12-02 19:55:03]

Please file a new bug describing the symptom and link BUG= to it, and write a
cc_unittest for this.

https://codereview.chromium.org/769273002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/769273002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2573: if (scroll_delta.y() != 0)
This fix isn't correct as it will hide the top controls even if the page is
nonscrollable (i.e. this change defeats the purpose of the if statement on line
2580).

It looks like the underlying problem you're fixing is that top controls hiding
doesn't work when scrolls bubble up from a scrollable sublayer.  That wikipedia
table is a scrollable sublayer (that happens to not have any room to scroll). 
Maybe a more correct fix would be to remove the CurrentlyScrollingLayer() != if
statement right below, as it seems that the problem is that we're checking that
too early and it's redundant with a later-binding check.

[elisabets, 2014-12-05 13:30:26]

https://codereview.chromium.org/769273002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/769273002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2573: if (scroll_delta.y() != 0)
Ah, so the CurrentlyScrollingLayer is the <scrollable> table (which is not the
same as either viewport). 
It makes a lot more sense to remove the following if-statement :)

On 2014/12/02 19:55:02, aelias wrote:
> This fix isn't correct as it will hide the top controls even if the page is
> nonscrollable (i.e. this change defeats the purpose of the if statement on
line
> 2580).
> 
> It looks like the underlying problem you're fixing is that top controls hiding
> doesn't work when scrolls bubble up from a scrollable sublayer.  That
wikipedia
> table is a scrollable sublayer (that happens to not have any room to scroll). 
> Maybe a more correct fix would be to remove the CurrentlyScrollingLayer() !=
if
> statement right below, as it seems that the problem is that we're checking
that
> too early and it's redundant with a later-binding check.

[aelias_OOO_until_Jul13, 2014-12-05 19:31:22]

lgtm, thanks!

[elisabets, 2014-12-08 07:51:20]

The CQ bit was checked by elisabets@opera.com

[commit-bot: I haz the power, 2014-12-08 07:52:09]

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

[commit-bot: I haz the power, 2014-12-08 07:57:19]

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

[commit-bot: I haz the power, 2014-12-08 07:57:20]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[elisabets, 2014-12-09 14:08:03]

On 2014/12/08 07:57:20, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_gn_dbg on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Moved test case to correct(er) place.

[elisabets, 2014-12-11 08:10:23]

I'm sorry about my sloppiness... 
Do I need another 'lgtm' or can I integrate it right away?

[jdduke (slow), 2014-12-11 23:50:17]

On 2014/12/11 08:10:23, elisabets wrote:
> I'm sorry about my sloppiness... 
> Do I need another 'lgtm' or can I integrate it right away?

Nope, you're all set to commit.

[elisabets, 2014-12-12 07:45:12]

The CQ bit was checked by elisabets@opera.com

[commit-bot: I haz the power, 2014-12-12 07:45:18]

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

[commit-bot: I haz the power, 2014-12-12 07:50:04]

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

[commit-bot: I haz the power, 2014-12-12 07:50:05]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[elisabets, 2014-12-12 08:26:17]

The CQ bit was checked by elisabets@opera.com

[commit-bot: I haz the power, 2014-12-12 08:26:42]

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

[commit-bot: I haz the power, 2014-12-12 08:30:41]

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

[commit-bot: I haz the power, 2014-12-12 08:30:42]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[elisabets, 2014-12-12 08:32:28]

The CQ bit was checked by elisabets@opera.com

[commit-bot: I haz the power, 2014-12-12 08:32:42]

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

[commit-bot: I haz the power, 2014-12-12 08:38:25]

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

[commit-bot: I haz the power, 2014-12-12 08:38:26]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[elisabets, 2014-12-12 08:48:19]

The CQ bit was checked by elisabets@opera.com

[commit-bot: I haz the power, 2014-12-12 08:48:49]

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

[commit-bot: I haz the power, 2014-12-12 08:54:51]

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

[commit-bot: I haz the power, 2014-12-12 08:54:53]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[elisabets, 2014-12-12 08:55:52]

The CQ bit was checked by elisabets@opera.com

[commit-bot: I haz the power, 2014-12-12 08:56:10]

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

[elisabets, 2014-12-12 09:00:31]

The CQ bit was checked by elisabets@opera.com

[commit-bot: I haz the power, 2014-12-12 09:00:52]

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

[commit-bot: I haz the power, 2014-12-12 09:51:57]

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

[commit-bot: I haz the power, 2014-12-12 09:51:58]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[elisabets, 2014-12-15 09:29:12]

The CQ bit was checked by elisabets@opera.com

[commit-bot: I haz the power, 2014-12-15 09:29:58]

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

[commit-bot: I haz the power, 2014-12-15 11:07:18]

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

[commit-bot: I haz the power, 2014-12-15 11:07:19]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[elisabets, 2014-12-15 11:56:02]

The CQ bit was checked by elisabets@opera.com

[commit-bot: I haz the power, 2014-12-15 11:56:21]

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

[commit-bot: I haz the power, 2014-12-15 12:50:14]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-15 12:51:04]

Patchset 7 (id:??) landed as
https://crrev.com/01d84d33126f4162c9bd0c7c7f9a2e5970ea2e73
Cr-Commit-Position: refs/heads/master@{#308340}