Hook ScrollElasticityController to InputHandlerProxy

Hook ScrollElasticityController to InputHandlerProxy

Move ScrollElasticityControllerClient to ScrollElasticityHelper
in cc, where it interfaces with LayerTreeHostImpl. Rename the
ScrollElasticityController to InputScrollElasticityController.

Send events handled on the impl thread to the
InputScrollElasticityController, to update the scroll animation state.  Do
this asynchronously, since it will need to be asynchronous on the main
thread for non-impl-thread-handled events. (Note that it may be that
we will send events to the elasticity system after they bounce back
to the browser -- this is the more conservative version, and may be
changed as main thread events are added).

With this in place, the InputScrollElasticityController produces the
right over-scroll offsets in ScrollElasticityHelper most of the time
(according to printfs). The next step will be to add a layer transformed
by these offsets.

BUG=133097
Committed: https://crrev.com/4163cc35e206688f75d30a5c6a9a77a93e6d0600
Cr-Commit-Position: refs/heads/master@{#304055}

[ccameron, 2014-11-11 23:50:29]

ccameron@chromium.org changed reviewers:
+ aelias@chromium.org, enne@chromium.org, jdduke@chromium.org

[ccameron, 2014-11-12 00:08:39]

This is filling out the steps in the document:
https://docs.google.com/document/d/1ePfdAkOKh7DMc7UBOyyXrkmf_25e-d8zTJktAGHtc...

And a more clean version of the prototype I sent around earlier at:
https://codereview.chromium.org/704463003/

The next steps I have in mind are to
- add transform layer in blink
- actually set the transform to draw what is being calculated in this patch
- clean up the animation logic copied from blink (timers, using scroll result,
etc)
- hook up the main thread event handling

[jdduke (slow), 2014-11-12 00:55:34]

Is there a tracking bug I can follow that has this all wired up (needn't be
pretty, but something functional)?

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:360:
base::MessageLoop::current()->PostTask(
Is this a speculative solution? Or have you observed severe divergence when this
call is synchronous? Wheel events are already async with respect to their source
(the browser), so I can't say that I completely understand how reposting the
task to the compositor thread will improve divergence from the main thread other
than introducing synthetic lag.

[ccameron, 2014-11-12 02:08:25]

The most complete version is this one:
https://codereview.chromium.org/704463003/

But it's very hacky (grabs some layers that happen to be around, alters
transforms in inappropriate places, etc).

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:360:
base::MessageLoop::current()->PostTask(
On 2014/11/12 00:55:34, jdduke wrote:
> Is this a speculative solution? Or have you observed severe divergence when
this
> call is synchronous? Wheel events are already async with respect to their
source
> (the browser), so I can't say that I completely understand how reposting the
> task to the compositor thread will improve divergence from the main thread
other
> than introducing synthetic lag.

This was aelias' suggestion (aelais: please verify that this is what you meant)
-- not so much to fix bugs, but to make sure that I wasn't relying on any
side-effects of doing the call synchronously. For instance, if calling
ObserveWheelEvent before any callbacks caused by InputHandler::ScrollBy hides
bugs, then we'd only see them when handling main-thread events. This will at
least make those sorts of things happen for both main- and impl-thread paths.

There is some discussion of bouncing this all the way back to the browser (so
that we can have the main and impl paths be identical), but I'm loathe to add
that plumbing until we see that it's needed in the context of the rest of the
work (namely, the main thread overscroll support).

[aelias_OOO_until_Jul13, 2014-11-12 02:27:41]

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:360:
base::MessageLoop::current()->PostTask(
On 2014/11/12 at 02:08:24, ccameron1 wrote:
> On 2014/11/12 00:55:34, jdduke wrote:
> > Is this a speculative solution? Or have you observed severe divergence when
this
> > call is synchronous? Wheel events are already async with respect to their
source
> > (the browser), so I can't say that I completely understand how reposting the
> > task to the compositor thread will improve divergence from the main thread
other
> > than introducing synthetic lag.
> 
> This was aelias' suggestion (aelais: please verify that this is what you
meant) -- not so much to fix bugs, but to make sure that I wasn't relying on any
side-effects of doing the call synchronously. For instance, if calling
ObserveWheelEvent before any callbacks caused by InputHandler::ScrollBy hides
bugs, then we'd only see them when handling main-thread events. This will at
least make those sorts of things happen for both main- and impl-thread paths.
> 
> There is some discussion of bouncing this all the way back to the browser (so
that we can have the main and impl paths be identical), but I'm loathe to add
that plumbing until we see that it's needed in the context of the rest of the
work (namely, the main thread overscroll support).

Yes, I suggested it based on my past experience that having the same code called
synchronously sometimes and async other times can have surprising consequences. 
Although I don't have any more specific reasons than that, for this use case.  I
agree it does look unnatural to do it this way and my preference would be to
always bounce the messages to the browser.

[jdduke (slow), 2014-11-12 02:32:24]

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:354: // thread, the event and its
disposition will be sent after being handled
Maybe tweak this to say something like "... and its disposition will be sent to
the elasticity controller after being..." just to make it explicit where exactly
they will be forwarded?

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:360:
base::MessageLoop::current()->PostTask(
On 2014/11/12 02:08:24, ccameron1 wrote:
> On 2014/11/12 00:55:34, jdduke wrote:
> > Is this a speculative solution? Or have you observed severe divergence when
> this
> > call is synchronous? Wheel events are already async with respect to their
> source
> > (the browser), so I can't say that I completely understand how reposting the
> > task to the compositor thread will improve divergence from the main thread
> other
> > than introducing synthetic lag.
> 
> This was aelias' suggestion (aelais: please verify that this is what you
meant)
> -- not so much to fix bugs, but to make sure that I wasn't relying on any
> side-effects of doing the call synchronously. For instance, if calling
> ObserveWheelEvent before any callbacks caused by InputHandler::ScrollBy hides
> bugs, then we'd only see them when handling main-thread events. This will at
> least make those sorts of things happen for both main- and impl-thread paths.
> 
> There is some discussion of bouncing this all the way back to the browser (so
> that we can have the main and impl paths be identical), but I'm loathe to add
> that plumbing until we see that it's needed in the context of the rest of the
> work (namely, the main thread overscroll support).

OK, thanks for the explanation, seems reasonable enough.

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
File content/renderer/input/input_handler_proxy_unittest.cc (right):

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
content/renderer/input/input_handler_proxy_unittest.cc:108:
GetScrollElasticityControllerClient() {
Would it be crazy to mock this and add a brief smoke test for
ObserveWheelEventAndResult? Might need to add a message loop to the test harness
to verify the async callback (not sure though).

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
File content/renderer/input/input_scroll_elasticity_controller.h (right):

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
content/renderer/input/input_scroll_elasticity_controller.h:50: virtual
~InputScrollElasticityController();
Nit: ~InputScrollElasticityController() override;

[jdduke (slow), 2014-11-12 02:45:59]

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
File content/renderer/input/input_scroll_elasticity_controller.h (right):

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
content/renderer/input/input_scroll_elasticity_controller.h:50: virtual
~InputScrollElasticityController();
On 2014/11/12 02:32:24, jdduke wrote:
> Nit: ~InputScrollElasticityController() override;

Oh, hmm, I didn't notice that the base class doesn't have a virtual destructor.

[ccameron, 2014-11-12 08:15:30]

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:354: // thread, the event and its
disposition will be sent after being handled
On 2014/11/12 02:32:24, jdduke wrote:
> Maybe tweak this to say something like "... and its disposition will be sent
to
> the elasticity controller after being..." just to make it explicit where
exactly
> they will be forwarded?

Good point -- changed to " event and its disposition will be sent to the
elasticity controller after being handled on the main thread."

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:360:
base::MessageLoop::current()->PostTask(
On 2014/11/12 02:27:41, aelias wrote:
> my preference would be to always bounce the messages to the browser.

To be clear, that may be the right thing to do, I just don't want to do it until
we have the other half (the main thread part) sorted out. When I get to that,
I'll probably write up the two different paths, and we can choose which of the
to CLs look better (I just lack the foresight to know which will be better ahead
of time).

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
File content/renderer/input/input_handler_proxy_unittest.cc (right):

https://codereview.chromium.org/713413002/diff/20001/content/renderer/input/i...
content/renderer/input/input_handler_proxy_unittest.cc:108:
GetScrollElasticityControllerClient() {
On 2014/11/12 02:32:24, jdduke wrote:
> Would it be crazy to mock this and add a brief smoke test for
> ObserveWheelEventAndResult? Might need to add a message loop to the test
harness
> to verify the async callback (not sure though).

This definitely needs to be done, but I want to clean up some other things first
(like, make the elasticity controller deterministic first).

[jdduke (slow), 2014-11-12 20:13:38]

The content/renderer/input changes look fine, I'll defer to aelias@ and enne@
for further review.

https://codereview.chromium.org/713413002/diff/40001/cc/input/scroll_elastici...
File cc/input/scroll_elasticity_controller.h (right):

https://codereview.chromium.org/713413002/diff/40001/cc/input/scroll_elastici...
cc/input/scroll_elasticity_controller.h:16: virtual void WillShutdown() = 0;
It's unfortunate to have yet another Bind/WillShutdown pairing. We could
probably achieve this implicitly with the existing
InputHandler/InputHandlerClient relationship (i.e., use the existing
InputHandlerProxy for WillShutdown/Animate routing), but maybe its' worth it to
be explicit here... Up to you.

[aelias_OOO_until_Jul13, 2014-11-12 22:18:00]

https://codereview.chromium.org/713413002/diff/40001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/713413002/diff/40001/cc/input/input_handler.h...
cc/input/input_handler.h:154: // Retrieve the client to interface with a
ScrollElasticityController.
This comment adds no information, please delete it.

https://codereview.chromium.org/713413002/diff/40001/cc/input/scroll_elastici...
File cc/input/scroll_elasticity_controller.h (right):

https://codereview.chromium.org/713413002/diff/40001/cc/input/scroll_elastici...
cc/input/scroll_elasticity_controller.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
This double license header is not a pattern used anywhere else, could you just
remove this and put the WebKit-style license at the top?  If you want to express
the fact that not all of the code originates from Apple anymore, the normal
pattern in WebKit licenses seems to be to add a second Copyright line, for
example see Source/core/css/CSSViewportRule.h

https://codereview.chromium.org/713413002/diff/40001/cc/trees/layer_tree_scro...
File cc/trees/layer_tree_scroll_elasticity_client.cc (right):

https://codereview.chromium.org/713413002/diff/40001/cc/trees/layer_tree_scro...
cc/trees/layer_tree_scroll_elasticity_client.cc:47: // TODO(ccameron): Get this
information from the LayerTreeHostImpl.
It's difficult to tell whether the way you've set up the classes is the right
factoring without these functions being implemented.  I suspect that you'll end
up duplicating most of these methods in LTHI anyway.  At which point this
isolated class is largely redundant.

Either way, this factoring doesn't really make sense to me.  You currently have
an abstract client class that will only ever have this subclass.  You should
either have the abstract client class with LTHI deriving from it, or you should
have a concrete helper separate from LTHI with no client base class.

I'd suggest you implement these methods and then I can review again; I'm fine
with reviewing a somewhat larger patch, I'm not the kind of reviewer who
necessarily prefers piecemeal ones (in fact I dislike nonfunctional piecemeal
patches because I need to predict the future to know whether something is a good
idea).

https://codereview.chromium.org/713413002/diff/40001/cc/trees/layer_tree_scro...
File cc/trees/layer_tree_scroll_elasticity_client.h (right):

https://codereview.chromium.org/713413002/diff/40001/cc/trees/layer_tree_scro...
cc/trees/layer_tree_scroll_elasticity_client.h:28: private:
I don't think this should be private in the subclass if it's public in the base
class.

[ccameron, 2014-11-13 00:24:39]

Patchset #4 (id:60001) has been deleted

[ccameron, 2014-11-13 00:28:37]

Thanks for the feedback. You're right about the structure being a bit strained
due to pulling through too many abstract classes.

I think that this does point to the problem that there  exists no formal concept
of a "public" interface to cc, and perhaps there should.

https://codereview.chromium.org/713413002/diff/40001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/713413002/diff/40001/cc/input/input_handler.h...
cc/input/input_handler.h:154: // Retrieve the client to interface with a
ScrollElasticityController.
On 2014/11/12 22:17:59, aelias wrote:
> This comment adds no information, please delete it.

Done.

https://codereview.chromium.org/713413002/diff/40001/cc/input/scroll_elastici...
File cc/input/scroll_elasticity_controller.h (right):

https://codereview.chromium.org/713413002/diff/40001/cc/input/scroll_elastici...
cc/input/scroll_elasticity_controller.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/11/12 22:17:59, aelias wrote:
> This double license header is not a pattern used anywhere else

The double license header pattern is used almost everywhere that a non-standard
license is needed in the chromium repository.

I copied the style from:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/...

See also :
content/browser/renderer_host/render_widget_host_view_mac.mm
content/browser/appcache/appcache_manifest_parser.cc
(and about 30 other files that come up if you do a git grep for "Apple Inc. All
rights reserved"

> could you just
> remove this and put the WebKit-style license at the top?  If you want to
express
> the fact that not all of the code originates from Apple anymore, the normal
> pattern in WebKit licenses seems to be to add a second Copyright line, for
> example see Source/core/css/CSSViewportRule.h

That example is from Webkit, this code is not in Webkit.

Some files put the 2 licenses next to each other, some embed the original
license next to the relevant code. I went with the second style, but am willing
to go with the first style.

Let me know if you have any other questions on this.

https://codereview.chromium.org/713413002/diff/40001/cc/input/scroll_elastici...
cc/input/scroll_elasticity_controller.h:16: virtual void WillShutdown() = 0;
On 2014/11/12 20:13:37, jdduke wrote:
> It's unfortunate to have yet another Bind/WillShutdown pairing. We could
> probably achieve this implicitly with the existing
> InputHandler/InputHandlerClient relationship (i.e., use the existing
> InputHandlerProxy for WillShutdown/Animate routing), but maybe its' worth it
to
> be explicit here... Up to you.

Agree -- pulled this through the existing InputHandlerProxy routing instead.

https://codereview.chromium.org/713413002/diff/40001/cc/trees/layer_tree_scro...
File cc/trees/layer_tree_scroll_elasticity_client.cc (right):

https://codereview.chromium.org/713413002/diff/40001/cc/trees/layer_tree_scro...
cc/trees/layer_tree_scroll_elasticity_client.cc:47: // TODO(ccameron): Get this
information from the LayerTreeHostImpl.
> Either way, this factoring doesn't really make sense to me.  You currently
have
> an abstract client class that will only ever have this subclass.  You should
> either have the abstract client class with LTHI deriving from it, or you
should
> have a concrete helper separate from LTHI with no client base class.

This was a reaction against what I perceive to be an excessive surface area of
cc:: being exposed to content:: et al. I'd prefer that we have a cc/public area
where we make a clear delineation of what may be relied on across that border
and what may not.

But, since we're not doing that (at least not yet, and anticipating it is a bad
idea), you're right.

I went with the concrete class as opposed to having LTHI implement a few
interface, because I feel that LTHI is having too much functionality stuffed
into it.

> I'd suggest you implement these methods and then I can review again; I'm fine
> with reviewing a somewhat larger patch, I'm not the kind of reviewer who
> necessarily prefers piecemeal ones (in fact I dislike nonfunctional piecemeal
> patches because I need to predict the future to know whether something is a
good
> idea).

Most of those TODOs should read "this function is actually not needed anymore"
(mostly because the WebKit implementation making over-scroll be a per-layer
option, which complicated that implementation immensely). I'll flesh these out.
There's only one TODO needed at that point (which is the one where we actually
set the transform on a layer that does't exist yet b/c it needs to be added in
WebKit).

It's true that there are a lot of functions here that may be pruned, but much
prefer to make the big changes to ScrollElasticityController in one
self-contained CL (which will remove its attempts to do scrolling, its
dependence on base::Time::Now(), etc).

https://codereview.chromium.org/713413002/diff/40001/cc/trees/layer_tree_scro...
File cc/trees/layer_tree_scroll_elasticity_client.h (right):

https://codereview.chromium.org/713413002/diff/40001/cc/trees/layer_tree_scro...
cc/trees/layer_tree_scroll_elasticity_client.h:28: private:
On 2014/11/12 22:17:59, aelias wrote:
> I don't think this should be private in the subclass if it's public in the
base
> class.

This issue went away because of using a concrete cc:: class to interface with
the LTHI.

[aelias_OOO_until_Jul13, 2014-11-13 02:11:03]

OK, thanks for the explanations.  Please update the patch description with the
new names.

https://codereview.chromium.org/713413002/diff/80001/cc/input/scroll_elastici...
File cc/input/scroll_elasticity_helper.cc (right):

https://codereview.chromium.org/713413002/diff/80001/cc/input/scroll_elastici...
cc/input/scroll_elasticity_helper.cc:39: LayerImpl* layer =
layer_tree_->InnerViewportScrollLayer();
Please use layer_tree_->TotalScrollOffset() and
layer_tree_->TotalMaxScrollOffset() instead, or this will break if/when pinch
virtual viewport ships on Mac.

https://codereview.chromium.org/713413002/diff/80001/cc/input/scroll_elastici...
cc/input/scroll_elasticity_helper.cc:53: return
layer_tree_->InnerViewportScrollLayer()->user_scrollable(HORIZONTAL);
user_scrollable() just denotes whether the layer is overflow: hidden, whereupon
it can be scrolled by Javascript but not by the user.  This is  not what you
want (it's a rare condition, and can only be set on the outer viewport).  I'd
suggest instead checking if layer_tree_->TotalMaxScrollOffset().x/y() == 0.

https://codereview.chromium.org/713413002/diff/80001/cc/input/scroll_elastici...
File cc/input/scroll_elasticity_helper.h (right):

https://codereview.chromium.org/713413002/diff/80001/cc/input/scroll_elastici...
cc/input/scroll_elasticity_helper.h:77: LayerTreeHostImpl* layer_tree_;
Please rename this to layer_tree_host_impl_.  There are three CC classes
starting with "LayerTree" so it needs disambiguation.

https://codereview.chromium.org/713413002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/713413002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:456: ScrollElasticityHelper*
LayerTreeHostImpl::GetScrollElasticityHelper() {
Could you rename this to CreateScrollElasticityHelper and DCHECK that it's only
called once?

https://codereview.chromium.org/713413002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/713413002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:49: class LayerTreeScrollElasticityClient;
obsolete

[ccameron, 2014-11-13 02:45:57]

Thanks!

https://codereview.chromium.org/713413002/diff/80001/cc/input/scroll_elastici...
File cc/input/scroll_elasticity_helper.cc (right):

https://codereview.chromium.org/713413002/diff/80001/cc/input/scroll_elastici...
cc/input/scroll_elasticity_helper.cc:39: LayerImpl* layer =
layer_tree_->InnerViewportScrollLayer();
On 2014/11/13 02:11:03, aelias wrote:
> Please use layer_tree_->TotalScrollOffset() and
> layer_tree_->TotalMaxScrollOffset() instead, or this will break if/when pinch
> virtual viewport ships on Mac.

Done. Btw, I'm helping get pinch virtual viewport enabled on Mac, because I'm
writing the WebKit parts of this against it (as opposed to the deprecated
structures).

https://codereview.chromium.org/713413002/diff/80001/cc/input/scroll_elastici...
cc/input/scroll_elasticity_helper.cc:53: return
layer_tree_->InnerViewportScrollLayer()->user_scrollable(HORIZONTAL);
On 2014/11/13 02:11:03, aelias wrote:
> user_scrollable() just denotes whether the layer is overflow: hidden,
whereupon
> it can be scrolled by Javascript but not by the user.  This is  not what you
> want (it's a rare condition, and can only be set on the outer viewport).  I'd
> suggest instead checking if layer_tree_->TotalMaxScrollOffset().x/y() == 0.

Thanks, done.

https://codereview.chromium.org/713413002/diff/80001/cc/input/scroll_elastici...
File cc/input/scroll_elasticity_helper.h (right):

https://codereview.chromium.org/713413002/diff/80001/cc/input/scroll_elastici...
cc/input/scroll_elasticity_helper.h:77: LayerTreeHostImpl* layer_tree_;
On 2014/11/13 02:11:03, aelias wrote:
> Please rename this to layer_tree_host_impl_.  There are three CC classes
> starting with "LayerTree" so it needs disambiguation.

Done.

https://codereview.chromium.org/713413002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/713413002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:456: ScrollElasticityHelper*
LayerTreeHostImpl::GetScrollElasticityHelper() {
On 2014/11/13 02:11:03, aelias wrote:
> Could you rename this to CreateScrollElasticityHelper and DCHECK that it's
only
> called once?

Done.

https://codereview.chromium.org/713413002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/713413002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:49: class LayerTreeScrollElasticityClient;
On 2014/11/13 02:11:03, aelias wrote:
> obsolete

Done.

[aelias_OOO_until_Jul13, 2014-11-13 02:57:34]

lgtm

[ccameron, 2014-11-13 02:59:44]

ccameron@chromium.org changed reviewers:
+ boliu@chromium.org

[ccameron, 2014-11-13 02:59:45]

Adding boliu to stamp the license change.

[jdduke (slow), 2014-11-13 03:03:09]

https://codereview.chromium.org/713413002/diff/140001/content/renderer/input/...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/713413002/diff/140001/content/renderer/input/...
content/renderer/input/input_handler_proxy.cc:180: if
(scroll_elasticity_controller_)
Can we just reset the variable and get rid of the WillShutdown?

[boliu, 2014-11-13 03:03:15]

lgtm

[ccameron, 2014-11-13 03:06:33]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2014-11-13 03:07:00]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2014-11-13 03:08:09]

https://codereview.chromium.org/713413002/diff/140001/content/renderer/input/...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/713413002/diff/140001/content/renderer/input/...
content/renderer/input/input_handler_proxy.cc:180: if
(scroll_elasticity_controller_)
On 2014/11/13 03:03:09, jdduke wrote:
> Can we just reset the variable and get rid of the WillShutdown?

Good point, yeah, that's simpler.

[jdduke (slow), 2014-11-13 03:14:02]

On 2014/11/13 03:08:09, ccameron1 wrote:
>
https://codereview.chromium.org/713413002/diff/140001/content/renderer/input/...
> File content/renderer/input/input_handler_proxy.cc (right):
> 
>
https://codereview.chromium.org/713413002/diff/140001/content/renderer/input/...
> content/renderer/input/input_handler_proxy.cc:180: if
> (scroll_elasticity_controller_)
> On 2014/11/13 03:03:09, jdduke wrote:
> > Can we just reset the variable and get rid of the WillShutdown?
> 
> Good point, yeah, that's simpler.

lgtm too with that change, thanks.

[ccameron, 2014-11-13 07:29:51]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-11-13 07:30:20]

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

[commit-bot: I haz the power, 2014-11-13 09:20:26]

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

[commit-bot: I haz the power, 2014-11-13 09:20:27]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ccameron, 2014-11-13 17:57:06]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-11-13 17:58:41]

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

[commit-bot: I haz the power, 2014-11-13 19:06:50]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2014-11-13 19:07:32]

Patchset 9 (id:??) landed as
https://crrev.com/4163cc35e206688f75d30a5c6a9a77a93e6d0600
Cr-Commit-Position: refs/heads/master@{#304055}