Implement top controls show/hide functionality for main thread scrolling

Implement top controls show/hide functionality for main thread scrolling.

This CL connects main-thread scrolls to existing top controls machinery in
compositor. Main-thread now updates top controls "content offset" during
slow-scrolls. This value is synced with compositor during the commit and
ensures that compositor properly animates top controls if necessary.

Also deprecated top_controls_layout_height in favor of two properties:
 - top_controls_height: The height of top controls
 - top_controls_shrink_viewport: flag controlling if blink view port should
 	shrink as a result of top_controls being show.

Chromium side CL: http://crrev.com/814083004

BUG=333143
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190610

[majidvp, 2015-02-09 15:37:19]

Patchset #4 (id:60001) has been deleted

[majidvp, 2015-02-11 22:57:32]

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

[aelias_OOO_until_Jul13, 2015-02-12 00:47:02]

My main comment is that since this is effectively TopControlsManager minus the
animation logic, it should also look almost exactly the same as
TopControlsManager in the relevant lines.  If we're going to duplicate the code,
we should at least make it look similar so that it's immediately clear the
behavior is the same.

https://codereview.chromium.org/893683003/diff/200001/Source/web/TopControls.cpp
File Source/web/TopControls.cpp (right):

https://codereview.chromium.org/893683003/diff/200001/Source/web/TopControls....
Source/web/TopControls.cpp:95: return consumed;
TopControlsManager returns the unconsumed amount instead, we should match it.

https://codereview.chromium.org/893683003/diff/200001/Source/web/TopControls.h
File Source/web/TopControls.h (right):

https://codereview.chromium.org/893683003/diff/200001/Source/web/TopControls....
Source/web/TopControls.h:89: float m_shownRatioAtRebaseline;
It looks like this is used to implement the same behavior as
controls_scroll_begin_offset_ in cc::TopControlsManager.  But you're using
different jargon and a different data format (shown ratio instead of content
offset).  Could you make the CC and Blink side match here?  (If you think the
"baseline" jargon is clearer, then it's fine to change the CC side to use that
wording.)

[majidvp, 2015-02-12 22:54:16]

On 2015/02/12 00:47:02, aelias wrote:
> My main comment is that since this is effectively TopControlsManager minus the
> animation logic, it should also look almost exactly the same as
> TopControlsManager in the relevant lines.  If we're going to duplicate the
code,
> we should at least make it look similar so that it's immediately clear the
> behavior is the same.

Logic and terminology should match TopControlsManager now.

>
https://codereview.chromium.org/893683003/diff/200001/Source/web/TopControls.cpp
> File Source/web/TopControls.cpp (right):
> 
>
https://codereview.chromium.org/893683003/diff/200001/Source/web/TopControls....
> Source/web/TopControls.cpp:95: return consumed;
> TopControlsManager returns the unconsumed amount instead, we should match it.

Done.

> 
> https://codereview.chromium.org/893683003/diff/200001/Source/web/TopControls.h
> File Source/web/TopControls.h (right):
> 
>
https://codereview.chromium.org/893683003/diff/200001/Source/web/TopControls....
> Source/web/TopControls.h:89: float m_shownRatioAtRebaseline;
> It looks like this is used to implement the same behavior as
> controls_scroll_begin_offset_ in cc::TopControlsManager.  But you're using
> different jargon and a different data format (shown ratio instead of content
> offset).  Could you make the CC and Blink side match here?  (If you think the
> "baseline" jargon is clearer, then it's fine to change the CC side to use that
> wording.)

Used rebaseline terminology in cc side. It is in the other patch.

[aelias_OOO_until_Jul13, 2015-02-13 02:03:18]

My main substantive comment is that this doesn't interact correctly with
scrollable subregions.

https://codereview.chromium.org/893683003/diff/220001/public/platform/WebLaye...
File public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/893683003/diff/220001/public/platform/WebLaye...
public/platform/WebLayerTreeView.h:99: virtual void setTopControlsHeight(float
height) { }
Could you maintain the pattern of the setTopControlsHeight call also taking a
boolean?  I think it clarifies the relationship and also may allow some state to
be adopted atomically.  Likewise in the TopControls class.

https://codereview.chromium.org/893683003/diff/260001/Source/web/PageWidgetDe...
File Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/893683003/diff/260001/Source/web/PageWidgetDe...
Source/web/PageWidgetDelegate.cpp:159: case WebInputEvent::GesturePinchBegin:
Not sure what this has to do with the rest of the patch?

https://codereview.chromium.org/893683003/diff/260001/Source/web/PageWidgetDe...
Source/web/PageWidgetDelegate.cpp:159: case WebInputEvent::GesturePinchBegin:
Touchscreen pinch events can't happen on the main thread right now so this is
all dead code for now.  I'd prefer we removed the pinch-related parts of this
patch and revisit when main-thread touchscreen pinch actually has a working
implementation.

https://codereview.chromium.org/893683003/diff/260001/Source/web/TopControls.h
File Source/web/TopControls.h (right):

https://codereview.chromium.org/893683003/diff/260001/Source/web/TopControls....
Source/web/TopControls.h:51: float shownRatio() {return m_shownRatio;};
Should be const, have spaces around the braces, and no semicolon:

float shownRatio() const { return m_shownRatio; }

https://codereview.chromium.org/893683003/diff/260001/Source/web/TopControls....
Source/web/TopControls.h:53: void updateShownRatio(float delta);
I think this alternate way of setting it is more confusing/redundant than it's
convenient, please delete it.

https://codereview.chromium.org/893683003/diff/260001/Source/web/TopControls....
Source/web/TopControls.h:55: void updateState(
Let's just make this updateConstraints() since the other values are unused.

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebInputEven...
File Source/web/WebInputEventConversion.h (right):

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebInputEven...
Source/web/WebInputEventConversion.h:72: PlatformGestureEventBuilder(Widget*,
const WebGestureEvent&, float deltaX, float deltaY);
Please delete this (no longer natural after changing top controls to deal
correctly with scrollable subregions).

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:701: // FIXME: Add a flag in settings that controls
top control feature.
I don't think we should ever add a setting.  Top-controls are no-op if the
height is 0, and I don't think the tiny additional CPU time is a problem.  Let's
remove the TODO.

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:702: float remainigDeltaY = 0.f;
Typo: "remaining"

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:706: // Modify event to adjust vertical scroll delta
before passing to |EventHandler|.
Top controls don't have priority over scrollable subregions.  The delta needs to
be taken away before the main frame scroll but not earlier.  This means the call
into TopControls likely needs to be moved elsewhere.  Given this, it may also be
no longer natural for top controls to "handleGestureEvent" as I don't think
that's how scroll deltas are bubbled elsewhere; TopControls could just take a
delta directly.

After fixing this, please add a unit test that top controls don't absorb scrolls
on scrollable subregions.

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:1730: m_layerTreeView->setTopControlsHeight(height);
Would be more consistent to put this in didUpdateTopControls.

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.h:767: OwnPtr<TopControls> m_topControls;
The lifetime is identical to WebViewImpl, so let's make this just a regular
member field.

https://codereview.chromium.org/893683003/diff/260001/Source/web/tests/TopCon...
File Source/web/tests/TopControlsTest.cpp (right):

https://codereview.chromium.org/893683003/diff/260001/Source/web/tests/TopCon...
Source/web/tests/TopControlsTest.cpp:64: WebViewImpl* initialize()
You don't have most of the typical Android-specific settings here and I wonder
if that may affect the tests.  Take a look at configureAndroidSettings() in
PinchViewportTest.cpp.  In particular, some important top controls behavior is
tied to setMainFrameResizesAreOrientationChanges.

Furthermore, I recommend disabling all scrollbars with MockScrollbarSettings as
non-overlay scrollbars can wreak havoc on the viewport size.

Having done that, check again to see if the Mac bots are fixed.

https://codereview.chromium.org/893683003/diff/260001/Source/web/tests/TopCon...
Source/web/tests/TopControlsTest.cpp:319: // Top controls should honor its
constraints
Please also add a test case that top controls are not immediately clamped when
the constraints change.  It should just prevent scrolling in the banned
direction and rely on the CC-side animation.

https://codereview.chromium.org/893683003/diff/260001/public/platform/WebLaye...
File public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/893683003/diff/260001/public/platform/WebLaye...
public/platform/WebLayerTreeView.h:94: // If this is set to true, then the
embedder shrunk the WebView size by the top
Please update the comment to say that this means Blink shrunk the viewport clip
layers by the top control height, as that's the relevant meaning from the point
of view of CC.

[bokan, 2015-02-13 12:34:32]

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

[bokan, 2015-02-13 12:35:26]

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:706: // Modify event to adjust vertical scroll delta
before passing to |EventHandler|.
On 2015/02/13 02:03:17, aelias wrote:
> Top controls don't have priority over scrollable subregions.  The delta needs
to
> be taken away before the main frame scroll but not earlier.  This means the
call
> into TopControls likely needs to be moved elsewhere.  Given this, it may also
be
> no longer natural for top controls to "handleGestureEvent" as I don't think
> that's how scroll deltas are bubbled elsewhere; TopControls could just take a
> delta directly.
> 
Right, I think this should go in EventHandler::handleGestureScrollUpdate and
EventHandler::handleWheelEvent, after scrolling any subframes/scrollable areas
but before scrolling the main frame or pinch viewport. This also means that
TopControls should live inside Blink core; like PinchViewport, hanging it inside
FrameHost probably makes the most sense.

https://codereview.chromium.org/893683003/diff/260001/Source/web/tests/TopCon...
File Source/web/tests/TopControlsTest.cpp (right):

https://codereview.chromium.org/893683003/diff/260001/Source/web/tests/TopCon...
Source/web/tests/TopControlsTest.cpp:64: WebViewImpl* initialize()
On 2015/02/13 02:03:17, aelias wrote:
> You don't have most of the typical Android-specific settings here and I wonder
> if that may affect the tests.  Take a look at configureAndroidSettings() in
> PinchViewportTest.cpp.  In particular, some important top controls behavior is
> tied to setMainFrameResizesAreOrientationChanges.
> 
> Furthermore, I recommend disabling all scrollbars with MockScrollbarSettings
as
> non-overlay scrollbars can wreak havoc on the viewport size.
> 
> Having done that, check again to see if the Mac bots are fixed.

The bot-only Mac failures are indeed the result of scrollbars affecting viewport
size. They're bot only as having a mouse (and depending on the mouse type) can
affect whether scrollbars are overlay or not.

https://codereview.chromium.org/893683003/diff/260001/public/web/WebTopContro...
File public/web/WebTopControlsState.h (right):

https://codereview.chromium.org/893683003/diff/260001/public/web/WebTopContro...
public/web/WebTopControlsState.h:36: // Represents Top Controls State.
Should note in comment here that these should match the Chromium version (and
vice versa in Chromium).

[majidvp, 2015-02-18 21:03:57]

https://codereview.chromium.org/893683003/diff/260001/Source/web/PageWidgetDe...
File Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/893683003/diff/260001/Source/web/PageWidgetDe...
Source/web/PageWidgetDelegate.cpp:159: case WebInputEvent::GesturePinchBegin:
On 2015/02/13 02:03:17, aelias wrote:
> Touchscreen pinch events can't happen on the main thread right now so this is
> all dead code for now.  I'd prefer we removed the pinch-related parts of this
> patch and revisit when main-thread touchscreen pinch actually has a working
> implementation.

Removed pinch handling logic/tests for not and returned this to previous state.

https://codereview.chromium.org/893683003/diff/260001/Source/web/TopControls.h
File Source/web/TopControls.h (right):

https://codereview.chromium.org/893683003/diff/260001/Source/web/TopControls....
Source/web/TopControls.h:51: float shownRatio() {return m_shownRatio;};
On 2015/02/13 02:03:17, aelias wrote:
> Should be const, have spaces around the braces, and no semicolon:
> 
> float shownRatio() const { return m_shownRatio; }

Done.

https://codereview.chromium.org/893683003/diff/260001/Source/web/TopControls....
Source/web/TopControls.h:53: void updateShownRatio(float delta);
On 2015/02/13 02:03:17, aelias wrote:
> I think this alternate way of setting it is more confusing/redundant than it's
> convenient, please delete it.

Done.

https://codereview.chromium.org/893683003/diff/260001/Source/web/TopControls....
Source/web/TopControls.h:55: void updateState(
On 2015/02/13 02:03:17, aelias wrote:
> Let's just make this updateConstraints() since the other values are unused.

Done.

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebInputEven...
File Source/web/WebInputEventConversion.h (right):

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebInputEven...
Source/web/WebInputEventConversion.h:72: PlatformGestureEventBuilder(Widget*,
const WebGestureEvent&, float deltaX, float deltaY);
On 2015/02/13 02:03:17, aelias wrote:
> Please delete this (no longer natural after changing top controls to deal
> correctly with scrollable subregions).

Done.

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:701: // FIXME: Add a flag in settings that controls
top control feature.
On 2015/02/13 02:03:17, aelias wrote:
> I don't think we should ever add a setting.  Top-controls are no-op if the
> height is 0, and I don't think the tiny additional CPU time is a problem. 
Let's
> remove the TODO.

Done.

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:702: float remainigDeltaY = 0.f;
On 2015/02/13 02:03:17, aelias wrote:
> Typo: "remaining"

Done.

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:706: // Modify event to adjust vertical scroll delta
before passing to |EventHandler|.
On 2015/02/13 12:35:25, bokan wrote:
> On 2015/02/13 02:03:17, aelias wrote:
> > Top controls don't have priority over scrollable subregions.  The delta
needs
> to
> > be taken away before the main frame scroll but not earlier.  This means the
> call
> > into TopControls likely needs to be moved elsewhere.  Given this, it may
also
> be
> > no longer natural for top controls to "handleGestureEvent" as I don't think
> > that's how scroll deltas are bubbled elsewhere; TopControls could just take
a
> > delta directly.
> > 
> Right, I think this should go in EventHandler::handleGestureScrollUpdate and
> EventHandler::handleWheelEvent, after scrolling any subframes/scrollable areas
> but before scrolling the main frame or pinch viewport. This also means that
> TopControls should live inside Blink core; like PinchViewport, hanging it
inside
> FrameHost probably makes the most sense. 

I moved TopControls inside FrameHost and remove its handleGestureEvent method.
Instead appropriate methods are individually invoked by EventHandler.

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:1730: m_layerTreeView->setTopControlsHeight(height);
On 2015/02/13 02:03:17, aelias wrote:
> Would be more consistent to put this in didUpdateTopControls.

Done.

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/893683003/diff/260001/Source/web/WebViewImpl....
Source/web/WebViewImpl.h:767: OwnPtr<TopControls> m_topControls;
On 2015/02/13 02:03:17, aelias wrote:
> The lifetime is identical to WebViewImpl, so let's make this just a regular
> member field.

Done. Moved it inside FrameHost and made it a regular member.

https://codereview.chromium.org/893683003/diff/260001/Source/web/tests/TopCon...
File Source/web/tests/TopControlsTest.cpp (right):

https://codereview.chromium.org/893683003/diff/260001/Source/web/tests/TopCon...
Source/web/tests/TopControlsTest.cpp:64: WebViewImpl* initialize()
On 2015/02/13 02:03:17, aelias wrote:
> You don't have most of the typical Android-specific settings here and I wonder
> if that may affect the tests.  Take a look at configureAndroidSettings() in
> PinchViewportTest.cpp.  In particular, some important top controls behavior is
> tied to setMainFrameResizesAreOrientationChanges.
> 
> Furthermore, I recommend disabling all scrollbars with MockScrollbarSettings
as
> non-overlay scrollbars can wreak havoc on the viewport size.
> 
> Having done that, check again to see if the Mac bots are fixed.

Added missing android configuration settings. Adding |UseMockScrollbarSettings|
did not solve the failing tests on mac bots. The odd thing is that the test pass
on my MBP but fail on the bot! I will keep investigating that but passing tests
on Mac is not really a priority because we do not have moving Top Controls on
Mac.

https://codereview.chromium.org/893683003/diff/260001/Source/web/tests/TopCon...
Source/web/tests/TopControlsTest.cpp:319: // Top controls should honor its
constraints
On 2015/02/13 02:03:17, aelias wrote:
> Please also add a test case that top controls are not immediately clamped when
> the constraints change.  It should just prevent scrolling in the banned
> direction and rely on the CC-side animation.

Done.

https://codereview.chromium.org/893683003/diff/260001/public/platform/WebLaye...
File public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/893683003/diff/260001/public/platform/WebLaye...
public/platform/WebLayerTreeView.h:94: // If this is set to true, then the
embedder shrunk the WebView size by the top
On 2015/02/13 02:03:17, aelias wrote:
> Please update the comment to say that this means Blink shrunk the viewport
clip
> layers by the top control height, as that's the relevant meaning from the
point
> of view of CC.

Done.

https://codereview.chromium.org/893683003/diff/260001/public/web/WebTopContro...
File public/web/WebTopControlsState.h (right):

https://codereview.chromium.org/893683003/diff/260001/public/web/WebTopContro...
public/web/WebTopControlsState.h:36: // Represents Top Controls State.
On 2015/02/13 12:35:25, bokan wrote:
> Should note in comment here that these should match the Chromium version (and
> vice versa in Chromium).

Done.

[aelias_OOO_until_Jul13, 2015-02-18 22:40:58]

lgtm modulo nits.

https://codereview.chromium.org/893683003/diff/280001/Source/core/frame/Frame...
File Source/core/frame/FrameHost.h (right):

https://codereview.chromium.org/893683003/diff/280001/Source/core/frame/Frame...
Source/core/frame/FrameHost.h:105: TopControls m_topControls;
Hmm, I guess let's use "const OwnPtrWillBeMember<>" here after all for
consistency with the other members.

https://codereview.chromium.org/893683003/diff/280001/Source/core/frame/TopCo...
File Source/core/frame/TopControls.cpp (right):

https://codereview.chromium.org/893683003/diff/280001/Source/core/frame/TopCo...
Source/core/frame/TopControls.cpp:71: FloatSize appliedDelta(0.f,
contentOffset() - oldOffset);
nit: Blink style is not to have ".f" for any literals that have no fractional
part, please remove all of them from this file.

https://codereview.chromium.org/893683003/diff/280001/Source/core/frame/TopCo...
Source/core/frame/TopControls.cpp:93: // Clamp
nit: zero-information comment, please remove

https://codereview.chromium.org/893683003/diff/280001/Source/web/WebInputEven...
File Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/893683003/diff/280001/Source/web/WebInputEven...
Source/web/WebInputEventConversion.cpp:292: 
nit: unnecessary newline diff, please remove.

[majidvp, 2015-02-19 15:03:10]

New patchsets have been uploaded after l-g-t-m from aelias@chromium.org

[majidvp, 2015-02-19 15:34:38]

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

[majidvp, 2015-02-19 15:34:40]

+rbyers for core/ public/ owner's review.

https://codereview.chromium.org/893683003/diff/280001/Source/core/frame/Frame...
File Source/core/frame/FrameHost.h (right):

https://codereview.chromium.org/893683003/diff/280001/Source/core/frame/Frame...
Source/core/frame/FrameHost.h:105: TopControls m_topControls;
On 2015/02/18 22:40:58, aelias wrote:
> Hmm, I guess let's use "const OwnPtrWillBeMember<>" here after all for
> consistency with the other members.

Done.

https://codereview.chromium.org/893683003/diff/280001/Source/core/frame/TopCo...
File Source/core/frame/TopControls.cpp (right):

https://codereview.chromium.org/893683003/diff/280001/Source/core/frame/TopCo...
Source/core/frame/TopControls.cpp:71: FloatSize appliedDelta(0.f,
contentOffset() - oldOffset);
On 2015/02/18 22:40:58, aelias wrote:
> nit: Blink style is not to have ".f" for any literals that have no fractional
> part, please remove all of them from this file.

Done except for when using std::[min|max].

https://codereview.chromium.org/893683003/diff/280001/Source/core/frame/TopCo...
Source/core/frame/TopControls.cpp:93: // Clamp
On 2015/02/18 22:40:58, aelias wrote:
> nit: zero-information comment, please remove

Done.

[Rick Byers, 2015-02-19 16:38:39]

Just a few small questions/comments.

https://codereview.chromium.org/893683003/diff/300001/Source/core/frame/TopCo...
File Source/core/frame/TopControls.h (right):

https://codereview.chromium.org/893683003/diff/300001/Source/core/frame/TopCo...
Source/core/frame/TopControls.h:40: // scroll is complete (i.e, ScrollEnd, or
FlingEnd).
nit: Add a comment saying that this duplicates CC's logic in TopControlsManager.

https://codereview.chromium.org/893683003/diff/300001/Source/core/frame/TopCo...
Source/core/frame/TopControls.h:67: FloatSize scrollBy(FloatSize scrollDelta);
conceptually this is a scroll customization (custom 'applyScroll' method on the
root document).  Perhaps once +tdresser lands his scroll customization CLs there
will be an opportunity to decouple this a bit using his new abstraction?

https://codereview.chromium.org/893683003/diff/300001/Source/core/frame/TopCo...
Source/core/frame/TopControls.h:76: // |layoutHeight()| give your the amount
that viewport is shrunk to
nit: "give your" -> "gives you"?  Although I guess this sentence is entirely
redundant with the comment on layoutHeight.

https://codereview.chromium.org/893683003/diff/300001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/893683003/diff/300001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:2549: delta = excessDelta;
should we perhaps be returning early for the case of delta becoming 0?

https://codereview.chromium.org/893683003/diff/300001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:3929: if
(m_frame->view()->scrollPosition().y() <
m_frame->view()->maximumScrollPosition().y())
I don't fully understand the intention here, can you add a comment?  Is this
just how you verify that the frame is actually scrollable?  So if I'm scrolled
all the way to the bottom and I start scrolling up, the first frame will scroll
the content, then the second frame will start hiding top controls?  That seems
weird, is it consistent with the CC logic?

https://codereview.chromium.org/893683003/diff/300001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/893683003/diff/300001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:1704: void
WebViewImpl::setTopControlsShownRatio(float offset)
looks like this no longer has any callers, remove it

https://codereview.chromium.org/893683003/diff/300001/Source/web/tests/TopCon...
File Source/web/tests/TopControlsTest.cpp (right):

https://codereview.chromium.org/893683003/diff/300001/Source/web/tests/TopCon...
Source/web/tests/TopControlsTest.cpp:136: // Top Controls are currently only
used on Android.
This might have something to do with the Mac-specific rubber-band scrolling. 
ccameron@ is the expert.

You should probably keep the test compiling on Mac but just marked as disabled. 
See
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/han...

https://codereview.chromium.org/893683003/diff/300001/Source/web/tests/TopCon...
Source/web/tests/TopControlsTest.cpp:273: // Scrollables subregions should
scroll before top controls
You've also verified this works properly with iframes, right?  The code path is
a little different for iframes so it might be nice to test that, but then again
bugs that cause that to fail but this to pass seem relatively unlikely so
perhaps it's not worth it.  WDYT?

[majidvp, 2015-02-19 19:18:02]

https://codereview.chromium.org/893683003/diff/300001/Source/core/frame/TopCo...
File Source/core/frame/TopControls.h (right):

https://codereview.chromium.org/893683003/diff/300001/Source/core/frame/TopCo...
Source/core/frame/TopControls.h:40: // scroll is complete (i.e, ScrollEnd, or
FlingEnd).
On 2015/02/19 16:38:38, Rick Byers wrote:
> nit: Add a comment saying that this duplicates CC's logic in
TopControlsManager.

Done.

https://codereview.chromium.org/893683003/diff/300001/Source/core/frame/TopCo...
Source/core/frame/TopControls.h:67: FloatSize scrollBy(FloatSize scrollDelta);
On 2015/02/19 16:38:38, Rick Byers wrote:
> conceptually this is a scroll customization (custom 'applyScroll' method on
the
> root document).  Perhaps once +tdresser lands his scroll customization CLs
there
> will be an opportunity to decouple this a bit using his new abstraction?

I agree we should be able to model this as yet another custom scroller in the
chain. I looked into having top controls implement ScrollableArea interface but
its abstraction is heavily tied with graphics layer and scrollbars and not
really a good fit for top controls IMO.

https://codereview.chromium.org/893683003/diff/300001/Source/core/frame/TopCo...
Source/core/frame/TopControls.h:76: // |layoutHeight()| give your the amount
that viewport is shrunk to
On 2015/02/19 16:38:38, Rick Byers wrote:
> nit: "give your" -> "gives you"?  Although I guess this sentence is entirely
> redundant with the comment on layoutHeight.

Removed as it is indeed redundant.

https://codereview.chromium.org/893683003/diff/300001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/893683003/diff/300001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:2549: delta = excessDelta;
On 2015/02/19 16:38:38, Rick Byers wrote:
> should we perhaps be returning early for the case of delta becoming 0?

Done. Originally I didn't do this thinking that we should allow main frame to
consume horizontal scroll but if both direction are zero there is no point in
continuing.

https://codereview.chromium.org/893683003/diff/300001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:3929: if
(m_frame->view()->scrollPosition().y() <
m_frame->view()->maximumScrollPosition().y())
On 2015/02/19 16:38:38, Rick Byers wrote:
> I don't fully understand the intention here, can you add a comment?  Is this
> just how you verify that the frame is actually scrollable?  So if I'm scrolled
> all the way to the bottom and I start scrolling up, the first frame will
scroll
> the content, then the second frame will start hiding top controls?  That seems
> weird, is it consistent with the CC logic?

Note that this is only relevant when scrolling down the page. The logic prevents
top controls to start hiding when there is no room for main frame to scroll
(e.g., frame is not scrollable or it is already scrolled all the way to the
bottom of the document). Added a comment to explain this.

This duplicated CC logic from here:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

https://codereview.chromium.org/893683003/diff/300001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/893683003/diff/300001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:1704: void
WebViewImpl::setTopControlsShownRatio(float offset)
On 2015/02/19 16:38:38, Rick Byers wrote:
> looks like this no longer has any callers, remove it

Done.

https://codereview.chromium.org/893683003/diff/300001/Source/web/tests/TopCon...
File Source/web/tests/TopControlsTest.cpp (right):

https://codereview.chromium.org/893683003/diff/300001/Source/web/tests/TopCon...
Source/web/tests/TopControlsTest.cpp:136: // Top Controls are currently only
used on Android.
On 2015/02/19 16:38:39, Rick Byers wrote:
> This might have something to do with the Mac-specific rubber-band scrolling. 
> ccameron@ is the expert.
> 
> You should probably keep the test compiling on Mac but just marked as
disabled. 
> See
>
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/han...

I will look into the rubber-band as a potential cause. For now I added a MAYBE
macro which disables the tests for Mac OSX until the cause is identified.

https://codereview.chromium.org/893683003/diff/300001/Source/web/tests/TopCon...
Source/web/tests/TopControlsTest.cpp:273: // Scrollables subregions should
scroll before top controls
On 2015/02/19 16:38:39, Rick Byers wrote:
> You've also verified this works properly with iframes, right?  The code path
is
> a little different for iframes so it might be nice to test that, but then
again
> bugs that cause that to fail but this to pass seem relatively unlikely so
> perhaps it's not worth it.  WDYT?

It works with iframes and I added a test to cover them as well.

[majidvp, 2015-02-20 11:31:06]

On 2015/02/19 19:18:02, majidvp wrote:
>
https://codereview.chromium.org/893683003/diff/300001/Source/core/frame/TopCo...
> File Source/core/frame/TopControls.h (right):
> 
>
https://codereview.chromium.org/893683003/diff/300001/Source/core/frame/TopCo...
> Source/core/frame/TopControls.h:40: // scroll is complete (i.e, ScrollEnd, or
> FlingEnd).
> On 2015/02/19 16:38:38, Rick Byers wrote:
> > nit: Add a comment saying that this duplicates CC's logic in
> TopControlsManager.
> 
> Done.
> 
>
https://codereview.chromium.org/893683003/diff/300001/Source/core/frame/TopCo...
> Source/core/frame/TopControls.h:67: FloatSize scrollBy(FloatSize scrollDelta);
> On 2015/02/19 16:38:38, Rick Byers wrote:
> > conceptually this is a scroll customization (custom 'applyScroll' method on
> the
> > root document).  Perhaps once +tdresser lands his scroll customization CLs
> there
> > will be an opportunity to decouple this a bit using his new abstraction?
> 
> I agree we should be able to model this as yet another custom scroller in the
> chain. I looked into having top controls implement ScrollableArea interface
but
> its abstraction is heavily tied with graphics layer and scrollbars and not
> really a good fit for top controls IMO.
> 
>
https://codereview.chromium.org/893683003/diff/300001/Source/core/frame/TopCo...
> Source/core/frame/TopControls.h:76: // |layoutHeight()| give your the amount
> that viewport is shrunk to
> On 2015/02/19 16:38:38, Rick Byers wrote:
> > nit: "give your" -> "gives you"?  Although I guess this sentence is entirely
> > redundant with the comment on layoutHeight.
> 
> Removed as it is indeed redundant.
> 
>
https://codereview.chromium.org/893683003/diff/300001/Source/core/page/EventH...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/893683003/diff/300001/Source/core/page/EventH...
> Source/core/page/EventHandler.cpp:2549: delta = excessDelta;
> On 2015/02/19 16:38:38, Rick Byers wrote:
> > should we perhaps be returning early for the case of delta becoming 0?
> 
> Done. Originally I didn't do this thinking that we should allow main frame to
> consume horizontal scroll but if both direction are zero there is no point in
> continuing.
> 
>
https://codereview.chromium.org/893683003/diff/300001/Source/core/page/EventH...
> Source/core/page/EventHandler.cpp:3929: if
> (m_frame->view()->scrollPosition().y() <
> m_frame->view()->maximumScrollPosition().y())
> On 2015/02/19 16:38:38, Rick Byers wrote:
> > I don't fully understand the intention here, can you add a comment?  Is this
> > just how you verify that the frame is actually scrollable?  So if I'm
scrolled
> > all the way to the bottom and I start scrolling up, the first frame will
> scroll
> > the content, then the second frame will start hiding top controls?  That
seems
> > weird, is it consistent with the CC logic?
> 
> Note that this is only relevant when scrolling down the page. The logic
prevents
> top controls to start hiding when there is no room for main frame to scroll
> (e.g., frame is not scrollable or it is already scrolled all the way to the
> bottom of the document). Added a comment to explain this.
> 
> This duplicated CC logic from here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
> 
>
https://codereview.chromium.org/893683003/diff/300001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/893683003/diff/300001/Source/web/WebViewImpl....
> Source/web/WebViewImpl.cpp:1704: void
> WebViewImpl::setTopControlsShownRatio(float offset)
> On 2015/02/19 16:38:38, Rick Byers wrote:
> > looks like this no longer has any callers, remove it
> 
> Done.
> 
>
https://codereview.chromium.org/893683003/diff/300001/Source/web/tests/TopCon...
> File Source/web/tests/TopControlsTest.cpp (right):
> 
>
https://codereview.chromium.org/893683003/diff/300001/Source/web/tests/TopCon...
> Source/web/tests/TopControlsTest.cpp:136: // Top Controls are currently only
> used on Android.
> On 2015/02/19 16:38:39, Rick Byers wrote:
> > This might have something to do with the Mac-specific rubber-band scrolling.

> > ccameron@ is the expert.
> > 
> > You should probably keep the test compiling on Mac but just marked as
> disabled. 
> > See
> >
>
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/han...
> 
> I will look into the rubber-band as a potential cause. For now I added a MAYBE
> macro which disables the tests for Mac OSX until the cause is identified.
> 
>
https://codereview.chromium.org/893683003/diff/300001/Source/web/tests/TopCon...
> Source/web/tests/TopControlsTest.cpp:273: // Scrollables subregions should
> scroll before top controls
> On 2015/02/19 16:38:39, Rick Byers wrote:
> > You've also verified this works properly with iframes, right?  The code path
> is
> > a little different for iframes so it might be nice to test that, but then
> again
> > bugs that cause that to fail but this to pass seem relatively unlikely so
> > perhaps it's not worth it.  WDYT?
> 
> It works with iframes and I added a test to cover them as well.

rbyers: PTAL. M42 is branched today and I like to land this before then if
possible.

[Rick Byers, 2015-02-21 02:39:11]

Damn, sorry for the delay - just getting caught up on e-mail now (and someone
missed this in my pass through outstanding CRs this morning).  Feel free to ping
me on IM for urgent reviews like this!

LGTM.  I'll put this in the CQ for you now and perhaps it's not too late.  I
have a minor nit, but it's a comment - you can land a separate CL for it.

https://codereview.chromium.org/893683003/diff/300001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/893683003/diff/300001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:3929: if
(m_frame->view()->scrollPosition().y() <
m_frame->view()->maximumScrollPosition().y())
On 2015/02/19 19:18:01, majidvp wrote:
> On 2015/02/19 16:38:38, Rick Byers wrote:
> > I don't fully understand the intention here, can you add a comment?  Is this
> > just how you verify that the frame is actually scrollable?  So if I'm
scrolled
> > all the way to the bottom and I start scrolling up, the first frame will
> scroll
> > the content, then the second frame will start hiding top controls?  That
seems
> > weird, is it consistent with the CC logic?
> 
> Note that this is only relevant when scrolling down the page. The logic
prevents
> top controls to start hiding when there is no room for main frame to scroll
> (e.g., frame is not scrollable or it is already scrolled all the way to the
> bottom of the document). Added a comment to explain this.
> 
> This duplicated CC logic from here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

Duh, sorry - I had the case inverted.  Makes sense, thanks.

https://codereview.chromium.org/893683003/diff/340001/Source/web/tests/TopCon...
File Source/web/tests/TopControlsTest.cpp (right):

https://codereview.chromium.org/893683003/diff/340001/Source/web/tests/TopCon...
Source/web/tests/TopControlsTest.cpp:139: // Crashes on Mac/Win only. 
http://crbug.com/2345
nit: this URL is incomplete (unless this was really a bug in Chrome version 0.2
<grin>).  Also, why the reference to 'win'?

[Rick Byers, 2015-02-21 02:39:19]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2015-02-21 02:39:54]

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

[commit-bot: I haz the power, 2015-02-21 02:43:03]

Committed patchset #17 (id:340001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190610

[majidvp, 2015-02-24 13:52:42]

On 2015/02/21 02:43:03, I haz the power (commit-bot) wrote:
> Committed patchset #17 (id:340001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=190610

aelias, bokan, rbyers: thanks for helpful review and comments. :)