Use only renderer driven offsets for fullscreen state.

Use only renderer driven offsets for fullscreen state.

BUG=

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

[mdjones, 2016-11-10 20:05:09]

mdjones@chromium.org changed reviewers:
+ mdjones@chromium.org

[mdjones, 2016-11-10 20:05:10]

High level logic looks good. I'll take another pass when you are completely
done. Glad to see this logic isolated!

https://codereview.chromium.org/2484293003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/BrowserStateBrowserControlsVisibilityDelegate.java
(right):

https://codereview.chromium.org/2484293003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/BrowserStateBrowserControlsVisibilityDelegate.java:92:
if (mPersistentControlTokens.size() == 1) {
&& mTab != null?

https://codereview.chromium.org/2484293003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/BrowserStateBrowserControlsVisibilityDelegate.java:100:
if (mTab != null) mTab.updateFullscreenEnabledState();
Maybe include this in the above condition, not sure it will help thought because
of the line length.

https://codereview.chromium.org/2484293003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/BrowserStateBrowserControlsVisibilityDelegate.java:156:
return sDisableForTesting || mPersistentControlTokens.isEmpty();
I think sDisableOverridesForTesting is more clear.

https://codereview.chromium.org/2484293003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
(right):

https://codereview.chromium.org/2484293003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:170:
if (tab != null) mBrowserVisibilityDelegate.showControlsTransient();
Could mBrowserVisibilityDelegate be responsible for the null check or are there
cases where this should work with null?

[Ted C, 2016-11-11 22:28:56]

Tests are added and initial comments addressed.  I need to fix custom tabs and
webapps to use the composed visibility delegate (which is sadly more work due to
their early startup paths)

https://codereview.chromium.org/2484293003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/BrowserStateBrowserControlsVisibilityDelegate.java
(right):

https://codereview.chromium.org/2484293003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/BrowserStateBrowserControlsVisibilityDelegate.java:92:
if (mPersistentControlTokens.size() == 1) {
On 2016/11/10 20:05:10, mdjones wrote:
> && mTab != null?

Done.

https://codereview.chromium.org/2484293003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/BrowserStateBrowserControlsVisibilityDelegate.java:100:
if (mTab != null) mTab.updateFullscreenEnabledState();
On 2016/11/10 20:05:10, mdjones wrote:
> Maybe include this in the above condition, not sure it will help thought
because
> of the line length.

Done.

https://codereview.chromium.org/2484293003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/BrowserStateBrowserControlsVisibilityDelegate.java:156:
return sDisableForTesting || mPersistentControlTokens.isEmpty();
On 2016/11/10 20:05:10, mdjones wrote:
> I think sDisableOverridesForTesting is more clear.

Done.

https://codereview.chromium.org/2484293003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
(right):

https://codereview.chromium.org/2484293003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:170:
if (tab != null) mBrowserVisibilityDelegate.showControlsTransient();
On 2016/11/10 20:05:10, mdjones wrote:
> Could mBrowserVisibilityDelegate be responsible for the null check or are
there
> cases where this should work with null?

This was here to replace the Tab#setFullscreenManager call that showed
a transient token, which would only happen if the tab was non-null.

If I move the null handling into the delegate, I would either need to
drop all show requests if the tab is null, or I potentially be incorrectly
marking mCurrentShowTime with an incorrect value.

For now, I'm leaning of leaving it as it is here because that is more
in line with the previous behavior, and we can think about moving it
later if it starts making more sense or is a common pattern.  Does
that sound reasonable?

[Ted C, 2016-11-11 22:42:48]

CustomTabs should be fixed now.

[mdjones, 2016-11-15 17:42:33]

There will be chrome home related conflicts.

https://codereview.chromium.org/2484293003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
(right):

https://codereview.chromium.org/2484293003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:170:
if (tab != null) mBrowserVisibilityDelegate.showControlsTransient();
On 2016/11/11 22:28:56, Ted C wrote:
> On 2016/11/10 20:05:10, mdjones wrote:
> > Could mBrowserVisibilityDelegate be responsible for the null check or are
> there
> > cases where this should work with null?
> 
> This was here to replace the Tab#setFullscreenManager call that showed
> a transient token, which would only happen if the tab was non-null.
> 
> If I move the null handling into the delegate, I would either need to
> drop all show requests if the tab is null, or I potentially be incorrectly
> marking mCurrentShowTime with an incorrect value.
> 
> For now, I'm leaning of leaving it as it is here because that is more
> in line with the previous behavior, and we can think about moving it
> later if it starts making more sense or is a common pattern.  Does
> that sound reasonable?

sgtm

https://codereview.chromium.org/2484293003/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2484293003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1389: *
@return The fullscreen manager, possibly null
Update doc to reflect this never returns null since it would crash.

https://codereview.chromium.org/2484293003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1392: if
(!mCreatedFullscreenManager) {
Why not just null check mFullscreenManager? Do some tests override
createFullscreenManager to return null?

https://codereview.chromium.org/2484293003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/BrowserStateBrowserControlsVisibilityDelegate.java
(right):

https://codereview.chromium.org/2484293003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/BrowserStateBrowserControlsVisibilityDelegate.java:75:
private long getShownDuration() {
Private, single-use one-liner. An argument could be made for code readability
though; up to you, I just wanted to point it out.

[Ted C, 2016-11-15 17:50:35]

I believe I rebased past all the chrome home conflicts.

https://codereview.chromium.org/2484293003/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2484293003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1389: *
@return The fullscreen manager, possibly null
On 2016/11/15 17:42:33, mdjones wrote:
> Update doc to reflect this never returns null since it would crash.

It can be null.  EmbedContentViewActivity.java returns null for this.  We should
look at removing that activity altogether though and replacing it with custom
tabs.  Out of scope for this change.

https://codereview.chromium.org/2484293003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1392: if
(!mCreatedFullscreenManager) {
On 2016/11/15 17:42:33, mdjones wrote:
> Why not just null check mFullscreenManager? Do some tests override
> createFullscreenManager to return null?

A test does but so does a legitimate activity.

https://codereview.chromium.org/2484293003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/BrowserStateBrowserControlsVisibilityDelegate.java
(right):

https://codereview.chromium.org/2484293003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/BrowserStateBrowserControlsVisibilityDelegate.java:75:
private long getShownDuration() {
On 2016/11/15 17:42:33, mdjones wrote:
> Private, single-use one-liner. An argument could be made for code readability
> though; up to you, I just wanted to point it out.

I think I was using it in multiple places before.  Inlined.

[mdjones, 2016-11-15 18:09:02]

lgtm

[Ted C, 2016-11-15 21:52:46]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2016-11-15 21:52:46]

The patchset sent to the CQ was uploaded after l-g-t-m from mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2484293003/#ps200001
(title: "Partially disable testControlsShownOnUnresponsiveRenderer due to timing
issues with renderer logic …")

[commit-bot: I haz the power, 2016-11-15 21:53:34]

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

[Ted C, 2016-11-15 21:58:13]

Talked offline with mdjones@ and there is more investigation needed to figure
some timing interactions between the browser control state requests and the
rebaselining that occurs in BrowserControlsOffsetManager in the renderer.

In particular, we show the controls on page load, but the default shown ratio in
the renderer is 0.  It is actually only set to 1 in:

BrowserControlsOffsetManager::SetupAnimation

In particular this block of code:
  if (!TopControlsHeight()) {
    client_->SetCurrentBrowserControlsShownRatio(
        direction == HIDING_CONTROLS ? 0.f : 1.f);
    return;
  }

And that only works because the tab constraint request is handled before we tell
the CVC the size of the top controls (i.e. it thinks it is 0 so we force the
shown ratio).

In general, I think there needs to be a larger understanding and cleanup of this
to make this more predictable, and that is outside the scope of this change.

[commit-bot: I haz the power, 2016-11-15 22:10:48]

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

[commit-bot: I haz the power, 2016-11-15 22:10:49]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Ted C, 2016-11-15 23:22:02]

The CQ bit was checked by tedchoc@chromium.org

[commit-bot: I haz the power, 2016-11-15 23:23:04]

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-11-15 23:52:44]

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

[commit-bot: I haz the power, 2016-11-15 23:52:45]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Ted C, 2016-11-16 01:11:52]

The CQ bit was checked by tedchoc@chromium.org

[commit-bot: I haz the power, 2016-11-16 01:12:35]

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-11-16 01:37:14]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-11-16 01:43:38]

Description was changed from

==========
Use only renderer driven offsets for fullscreen state.

BUG=
==========

to

==========
Use only renderer driven offsets for fullscreen state.

BUG=

Committed: https://crrev.com/b026e994a4822fadddccc99cc10cf431e2e97066
Cr-Commit-Position: refs/heads/master@{#432345}
==========

[commit-bot: I haz the power, 2016-11-16 01:43:39]

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