tguilbert captured a screenshot, attached.

we can make ChromeFullscreenManager not request an animation, which fixes the thing shown in the screenshot.  however, it also sometimes causes the tool bar to stay visible in fullscreen if the video is playing when we enter fs.

here's why.  it turns out that the tool bar is handled by the cc impl-side scrolling mechanism.  so, it assumes that things are additive.  unfortunately, it's clamped at [0,1], which isn't a commutative operation.  "clamp -1" looks a lot like "add one".

if video is playing, then the main thread is pushing updates to the impl thread.  that's a requirement.  here's the sequence:

1. browser notifies main thread to hide, which sets the main side "tool bar shown ratio" to 0, meaning "hidden".

2. main thread notifies impl thread that the tool bar constraints are shown.  impl thread applies -1 delta to the active tree, which has base value 1 (shown).

3. main side pushes a frame to the pending tree with base:0.  the pending delta is still -1, since the impl side never told the browser about the impl-side change in response to 2.

4. impl side sends the active delta (-1) to main.

5. impl side activates the pending tree.  as part of a clamp, it clears the delta, since the new active base is 0 and that's what it wants.  it should be -1, but is clamped to zero.

6. main side processes the -1 delta from the impl side.  it already has 0, so it clamps -1 to 0 and sends it back.

7. the impl side gets back the 0.  since it had notified the main thread about the -1, it assumes (correctly) that it is reflected in the main thread's value.  it thus subtracts it from the pending delta.  the pending delta was zero, due to the impl-side clamp, so it now has a pending delta of 1.

8. this gets sent to the browser, which adds 1, sends it back, and everything converges on 1 (controls shown).

note that if the main thread isn't playing video, then it probably doesn't push the 0-based pending tree in 3.  in this case, it all works out since the active delta remains -1, and is correctly adjusted by subtracting the -1 later in step 7.  it also works even if the video is playing, as long as the main thread doesn't happen to push a frame.

Deleted: enter_fs_toolbar_scroll_up_shows_inline_content.xcf 464 KB

enter_fs_toolbar_scroll_up_shows_inline_content.xcf 464 KB Download

to get around this, we can clamp the toolbar ratio on the main thread if it's not being animated.  there is similar code already in BrowserControls.cpp when setting the constraints, but it's not applied to deltas that arrive from the impl side.  it occasionally results in the toolbar flickering one frame back to fully visible, but at least it doesn't get stuck.  it's pretty rare.

it doesn't fix the underlying issue, but it does avoid it.

Clarifying question (bear with me, it's been a while since I've dealt with this),

In step 2, do you mean "main thread notifies impl...constraints are _hidden_"? Also, you say impl thread applies -1 delta, that's  not direct right? Generally all the changes flow through the pending tree, so is this skipping the pending tree somehow or do you have an implicit activation here? The other thing I'm confused about here is why it's applying delta at all. Delta is "changes on the impl thread that main doesn't know about yet". Since the change here is coming from main we should be directly applying it to the base value. This sounds like it might be the underlying bug to me.

sorry for the delay.  was ooo.  thanks for the comments.

| "main thread notifies... hidden"

yes, hidden.  sorry about that.

| Also, you say impl thread applies -1 delta...pending tree

i don't believe that it goes through the pending tree.  i think that BrowserControlsOffsetManager calls LayerTreeHostImpl::SetCurrentBrowserControlsShownRatio, which sets it on |active_tree_| here https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?rcl=497159595a57a1f210a30b9b170945b76232ac93&l=2654

this struck me as weird, since it's out-of-band with respect to the normal main -> pending -> active path.  but, please see below.

| Since the change here is coming from main we should be directly applying it
| to the base value.

this is one of the approaches that i looked at -- make only one side do adjustment, and avoid the double-clamping.  however, i wasn't able to convince myself that the main thread would always send a new pending tree in a timely fashion, leaving the ratio unchanged in the active tree.

if it's as simple as commenting out that one line in SetCurrent..., then cool.  i just don't know enough about how this whole thing works to guarantee that it won't get stuck shown / hidden in other cases.  i wasn't able to make it fail in testing, either with the video playing or hidden.  i'll upload the PS for it.

doing this causes a test failure "BrowserControlsOffsetManagerTest.GrowingHeightKeepsBrowserControlsHidden", since it now no longer notifies the client about the new ratio.

however, looking at the only call path to SetCurrentBrowserControlsShownRatio (starts at WebViewImpl::UpdateBrowserControlsState https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/exported/WebViewImpl.cpp?rcl=c73d5a44d670ec6957d87582129c0ac27b2c68bc&l=1826 ), it's not clear to me that it's really a problem.

Thanks for explaining. Ok, I think I understand what's going on now. Unfortunately it's a more general issue and I'm not sure how to solve it as such.

To clarify, imagine you have 100px of scrolling happen on the main and cc threads at identical moments. We'd normally expect that after all the deltas are propagated through the commit and activate cycle, the final scroll offset will be 200px.

Now suppose 100px is the maximum scroll offset. If we hit the same sequence as you listed above, the 100px base value coming from main will clear out the active delta after it has been sent to main (since we're already at 100px current value). When main commits again and activates the pending tree, we subtract out the reflected value from the delta so we'll end up at 0px. This is exactly the same situation as the URL bar.

Seems to me that this won't be easy to fix so I'm fine fixing the URL bar issue by ensuring we don't setup the race.

+aelias@ (when he returns) in case he has some insight here.

indeed, the core problem is that clamping isn't a commutative operation, and addition isn't semantically the same thing.  :(

to be clear, do you prefer the new approach over the old one?

ah, sorry -- didn't see the CL comments.  thanks for the help!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/486a1f16ac616129842ef7e7725b5ec5676a7394

commit 486a1f16ac616129842ef7e7725b5ec5676a7394
Author: liberato@chromium.org <liberato@chromium.org>
Date: Tue Aug 29 14:59:50 2017

Don't animate toolbar when entering fullscreen.

When the toolbar animates to hide when entering fullscreen, it can
cause a very janky transition.  Part of the inline page is scrolled
into view at the bottom during the animation.  Plus, the fullscreen
content slides up with the toolbar.

This CL turns off the animation, and instead just hides the toolbar
immediately.  This greatly improves the transition.

Unfortunately, it also uncovered a bug: sometimes, the toolbar
would remain visible in fullscreen mode if the renderer is pushing
frames to the cc impl tree during the fs transition.  See the
bug for more details.

To get around this, the impl thread no longer forces the ratio to
zero if it's not being animated.  It is updated when the next
pending tree is pushed, to avoid the double update (and the
subsequent double-clamp that results in it being shown).

Bug:  754346 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I269e2ead0b81db6635231640b28b7aa30ff950e1
Reviewed-on: https://chromium-review.googlesource.com/617922
Reviewed-by: Alexandre Elias <aelias@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Commit-Queue: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498111}
[modify] https://crrev.com/486a1f16ac616129842ef7e7725b5ec5676a7394/cc/input/browser_controls_offset_manager.cc
[modify] https://crrev.com/486a1f16ac616129842ef7e7725b5ec5676a7394/cc/input/browser_controls_offset_manager_unittest.cc
[modify] https://crrev.com/486a1f16ac616129842ef7e7725b5ec5676a7394/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/486a1f16ac616129842ef7e7725b5ec5676a7394/third_party/WebKit/Source/core/frame/BrowserControlsTest.cpp