Controls offsets computed if either top or bottom are showing

cc/input/browser_controls_offset_manager.cc

Index: cc/input/browser_controls_offset_manager.cc
diff --git a/cc/input/browser_controls_offset_manager.cc b/cc/input/browser_controls_offset_manager.cc
index b705e440adc7826699e1e87a3b1cb20de8473e9c..058c3f2d5fc79daa0b56d3eb87f2d5c27584a4df 100644
--- a/cc/input/browser_controls_offset_manager.cc
+++ b/cc/input/browser_controls_offset_manager.cc
@@ -43,7 +43,8 @@ BrowserControlsOffsetManager::BrowserControlsOffsetManager(
       animation_direction_(NO_ANIMATION),
       permitted_state_(BOTH),
       accumulated_scroll_delta_(0.f),
-      baseline_content_offset_(0.f),
+      baseline_top_content_offset_(0.f),
+      baseline_bottom_content_offset_(0.f),
       controls_show_threshold_(controls_hide_threshold),
       controls_hide_threshold_(controls_show_threshold),
       pinch_gesture_active_(false) {
@@ -120,7 +121,12 @@ void BrowserControlsOffsetManager::ScrollBegin() {
 
 gfx::Vector2dF BrowserControlsOffsetManager::ScrollBy(
     const gfx::Vector2dF& pending_delta) {
-  if (!TopControlsHeight())
+  // If one or both of the top/bottom controls are showing, the shown ratio
+  // needs to be computed.
+  float controls_height =
+      TopControlsHeight() ? TopControlsHeight() : BottomControlsHeight();
+
+  if (!controls_height)
     return pending_delta;
 
   if (pinch_gesture_active_)
@@ -133,10 +139,11 @@ gfx::Vector2dF BrowserControlsOffsetManager::ScrollBy(
 
   accumulated_scroll_delta_ += pending_delta.y();
 
-  float old_offset = ContentTopOffset();
+  float old_top_offset = ContentTopOffset();
+  float baseline_content_offset = TopControlsHeight()
+      ? baseline_top_content_offset_ : baseline_bottom_content_offset_;
   client_->SetCurrentBrowserControlsShownRatio(
-      (baseline_content_offset_ - accumulated_scroll_delta_) /
-      TopControlsHeight());
+      (baseline_content_offset - accumulated_scroll_delta_) / controls_height);
 
   // If the controls are fully visible, treat the current position as the
   // new baseline even if the gesture didn't end.
@@ -145,7 +152,12 @@ gfx::Vector2dF BrowserControlsOffsetManager::ScrollBy(
 
   ResetAnimations();
 
-  gfx::Vector2dF applied_delta(0.f, old_offset - ContentTopOffset());
+  // applied_delta will negate any scroll on the content if the top browser
+  // controls are showing in favor of hiding the controls and resizing the
+  // content. If the top controls have no height, the content should scroll
+  // immediately.
+  gfx::Vector2dF applied_delta(0.f,
+      TopControlsHeight() ? old_top_offset - ContentTopOffset() : 0.0f);
   return pending_delta - applied_delta;
 }
 
@@ -188,7 +200,9 @@ gfx::Vector2dF BrowserControlsOffsetManager::Animate(
   if (IsAnimationComplete(new_ratio))
     ResetAnimations();
 
-  gfx::Vector2dF scroll_delta(0.f, ContentTopOffset() - old_offset);
+  // Don't move the content if the top controls aren't used.
+  gfx::Vector2dF scroll_delta(0.f,
+      TopControlsHeight() ? (ContentTopOffset() - old_offset) : 0.0f);

[aelias_OOO_until_Jul13, 2016/11/04 04:24:50]

ContentTopOffset() method name is now a lie (it doesn't affect content top
offset at all when only bottom controls are present) and you have a bit of
duplicated logic here.

So I suggest introducing a new method called VisibleControlsHeight which has the
exact same implementation as ContentTopOffset() does today.  Then change
ContentTopOffset() implementation to be { TopControlsHeight() ?
VisibleControlsHeight() : 0.f }

[mdjones, 2016/11/05 00:17:26]

I'm not sure I understand what you mean. In the case that only the bottom
controls are showing, ContentTopOffset will correctly return 0. The proposed
change would still result in me needing the logic for scroll_delta.

[aelias_OOO_until_Jul13, 2016/11/05 00:47:15]

OK, right.  Well, the source of my confusion is that assumed you made the change
here in order to fix a bug and drew a logical conclusion without actually
looking again at ContentTopOffset() implementation :).  Maybe you only added the
extra unnecessary complexity for some principle of cleanliness -- if so, I don't
agree with that principle, I prefer code to have as few branches as possible.

[mdjones, 2016/11/07 17:14:45]

Done, added logic to ContentTopOffset and ContentBottomOffset to return 0 if
their respective heights are 0.

   return scroll_delta;
 }
 
@@ -255,7 +269,8 @@ bool BrowserControlsOffsetManager::IsAnimationComplete(float new_ratio) {
 
 void BrowserControlsOffsetManager::ResetBaseline() {
   accumulated_scroll_delta_ = 0.f;
-  baseline_content_offset_ = ContentTopOffset();
+  baseline_top_content_offset_ = ContentTopOffset();
+  baseline_bottom_content_offset_ = ContentBottomOffset();
 }
 
 }  // namespace cc