Expose FS show/hide thresholds to Command Line

Expose FS show/hide thresholds to Command Line

Pipe the show/hide thresholds out to the command line.  Allow for them not to be
based on one parameter (there isn't one split point that determines show/hide).
Instead there are two thresholds, one for showing and one for hiding.  The
ambiguous area is handled by determining whether or not the scroll was, as a
whole, moving up or down.

BUG=174449
NOTRY=true


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183870

[David Trainor- moved to gerrit, 2013-02-06 23:54:03]

ptal

[Ted C, 2013-02-07 00:40:26]

https://codereview.chromium.org/12210050/diff/1/cc/switches.cc
File cc/switches.cc (right):

https://codereview.chromium.org/12210050/diff/1/cc/switches.cc#newcode36
cc/switches.cc:36: // How much of the top controls need to be hidden before they
will auto hide.
Maybe specify that this is a percentage?  Same for below.

// Percentage of the top controls that need ...

https://codereview.chromium.org/12210050/diff/1/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/12210050/diff/1/cc/top_controls_manager.cc#ne...
cc/top_controls_manager.cc:196: void
TopControlsManager::StartAnimationIfNecessary() {
Can you add a unittest that verifies the new animation behavior?

https://codereview.chromium.org/12210050/diff/1/cc/top_controls_manager.cc#ne...
cc/top_controls_manager.cc:202: -(top_controls_height_ *
top_controls_hide_threshold_)) {
Looking at this, should we initialize top_controls_hide_threshold_ to be the
precomputed value of

-(top_controls_height * top_controls_show_threshold)

in the constructor?

https://codereview.chromium.org/12210050/diff/1/cc/top_controls_manager.cc#ne...
cc/top_controls_manager.cc:206: -(top_controls_height_ * (1.f -
top_controls_show_threshold_))) {
same for this?

https://codereview.chromium.org/12210050/diff/1/cc/top_controls_manager.h
File cc/top_controls_manager.h (right):

https://codereview.chromium.org/12210050/diff/1/cc/top_controls_manager.h#new...
cc/top_controls_manager.h:76: float top_controls_hide_threshold_;
I would put the floats below with the other ones.  Although we might want to
split configuration vs state variables with a space, but not necessarily.

https://codereview.chromium.org/12210050/diff/1/cc/top_controls_manager_unitt...
File cc/top_controls_manager_unittest.cc (right):

https://codereview.chromium.org/12210050/diff/1/cc/top_controls_manager_unitt...
cc/top_controls_manager_unittest.cc:58: if (!manager_)
need to use braces if the statement is more than one line

[David Trainor- moved to gerrit, 2013-02-11 19:38:28]

Addressed comments.  ptal

https://chromiumcodereview.appspot.com/12210050/diff/1/cc/switches.cc
File cc/switches.cc (right):

https://chromiumcodereview.appspot.com/12210050/diff/1/cc/switches.cc#newcode36
cc/switches.cc:36: // How much of the top controls need to be hidden before they
will auto hide.
On 2013/02/07 00:40:26, Ted C wrote:
> Maybe specify that this is a percentage?  Same for below.
> 
> // Percentage of the top controls that need ...

Done.

https://chromiumcodereview.appspot.com/12210050/diff/1/cc/top_controls_manage...
File cc/top_controls_manager.cc (right):

https://chromiumcodereview.appspot.com/12210050/diff/1/cc/top_controls_manage...
cc/top_controls_manager.cc:196: void
TopControlsManager::StartAnimationIfNecessary() {
On 2013/02/07 00:40:26, Ted C wrote:
> Can you add a unittest that verifies the new animation behavior?

Done.

https://chromiumcodereview.appspot.com/12210050/diff/1/cc/top_controls_manage...
cc/top_controls_manager.cc:202: -(top_controls_height_ *
top_controls_hide_threshold_)) {
On 2013/02/07 00:40:26, Ted C wrote:
> Looking at this, should we initialize top_controls_hide_threshold_ to be the
> precomputed value of
> 
> -(top_controls_height * top_controls_show_threshold)
> 
> in the constructor?

Done.

https://chromiumcodereview.appspot.com/12210050/diff/1/cc/top_controls_manage...
cc/top_controls_manager.cc:206: -(top_controls_height_ * (1.f -
top_controls_show_threshold_))) {
On 2013/02/07 00:40:26, Ted C wrote:
> same for this?

Done.

https://chromiumcodereview.appspot.com/12210050/diff/1/cc/top_controls_manager.h
File cc/top_controls_manager.h (right):

https://chromiumcodereview.appspot.com/12210050/diff/1/cc/top_controls_manage...
cc/top_controls_manager.h:76: float top_controls_hide_threshold_;
On 2013/02/07 00:40:26, Ted C wrote:
> I would put the floats below with the other ones.  Although we might want to
> split configuration vs state variables with a space, but not necessarily.

Done.

https://chromiumcodereview.appspot.com/12210050/diff/1/cc/top_controls_manage...
File cc/top_controls_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/12210050/diff/1/cc/top_controls_manage...
cc/top_controls_manager_unittest.cc:58: if (!manager_)
On 2013/02/07 00:40:26, Ted C wrote:
> need to use braces if the statement is more than one line

Done.

[Ted C, 2013-02-12 15:40:03]

lgtm w/ naming nit

https://chromiumcodereview.appspot.com/12210050/diff/5002/cc/top_controls_man...
File cc/top_controls_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/12210050/diff/5002/cc/top_controls_man...
cc/top_controls_manager_unittest.cc:254: TEST(TopControlsManagerTest,
partialHideWithAmbiguousThresholdShows) {
I guess to be consistent with the above names, it should be
partialHidden

same thing below:
partialShown instead of partialShow

[David Trainor- moved to gerrit, 2013-02-12 18:19:25]

https://chromiumcodereview.appspot.com/12210050/diff/5002/cc/top_controls_man...
File cc/top_controls_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/12210050/diff/5002/cc/top_controls_man...
cc/top_controls_manager_unittest.cc:254: TEST(TopControlsManagerTest,
partialHideWithAmbiguousThresholdShows) {
On 2013/02/12 15:40:03, Ted C wrote:
> I guess to be consistent with the above names, it should be
> partialHidden
> 
> same thing below:
> partialShown instead of partialShow

Done.

[David Trainor- moved to gerrit, 2013-02-14 22:57:58]

Alex: Could you take a look at render_process_host_impl

James: Can you take a look at cc?  It's mainly plumbing, Ted looked at
top_controls pretty extensively

Thanks!

[jamesr, 2013-02-20 08:08:35]

Seems reasonable, but some of these pieces have mode around.

https://codereview.chromium.org/12210050/diff/12003/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/12210050/diff/12003/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:68: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kTopControlsShowThreshold))
{
this code won't do what you want (it isn't actually live in ToT).  you want to
patch the settings initialization code in
content/renderer/gpu/render_widget_compositor.cc to pipe settings in to the
cc::LayerTreeHost used for the render process

[aelias_OOO_until_Jul13, 2013-02-20 08:19:20]

render_process_host_impl.cc flag addition lgtm

[aelias_OOO_until_Jul13, 2013-02-20 08:19:21]

render_process_host_impl.cc flag addition lgtm

[David Trainor- moved to gerrit, 2013-02-21 02:07:36]

rebased and moved settings setup code to render_widget_compositor.cc.

ptal thanks!

https://codereview.chromium.org/12210050/diff/12003/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/12210050/diff/12003/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:68: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kTopControlsShowThreshold))
{
On 2013/02/20 08:08:35, jamesr wrote:
> this code won't do what you want (it isn't actually live in ToT).  you want to
> patch the settings initialization code in
> content/renderer/gpu/render_widget_compositor.cc to pipe settings in to the
> cc::LayerTreeHost used for the render process

Ah yeah rebased and saw this.  Moved the implementation over.  Thanks!

[jamesr, 2013-02-21 02:11:55]

lgtm

[commit-bot: I haz the power, 2013-02-21 06:40:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/12210050/18001

[commit-bot: I haz the power, 2013-02-21 08:05:42]

Retried try job too often on android_clang_dbg for step(s) build
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-02-21 18:05:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/12210050/27003

[commit-bot: I haz the power, 2013-02-21 22:08:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/12210050/27003

[commit-bot: I haz the power, 2013-02-21 22:27:44]

Change committed as 183870