MouseMove when the mouse is down to signal compositor priority

MouseMove when the mouse is down to signal compositor priority

When the user is moving the mouse with the mouse button down it's quite
likely they're scrolling the page, or otherwise doing something where
they would expect the animation to be smooth.  Use this as a signal that
we should enter compositor prioirty mode.  Note this does mean that with
the mouse down every MouseMove event will cause us to lock a mutex and
call Now() (which should be OK). A policy update task is only posted
every 100ms.

TBR=sievers@chromium.org for the FakeRenderScheduler
BUG=391005
Committed: https://crrev.com/1924435ea3211a836b7ef4f8a576285c3cf365f7
Cr-Commit-Position: refs/heads/master@{#315566}

[alex clarke (OOO till 29th), 2015-02-05 15:20:45]

alexclarke@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2015-02-05 15:27:27]

skyostil@chromium.org changed reviewers:
+ jdduke@chromium.org

[Sami, 2015-02-05 15:27:28]

Should we do this for wheel events first or at the same time? They seem like a
much stronger signal that we're scrolling.

https://codereview.chromium.org/902783003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/902783003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:136: if (type ==
blink::WebInputEvent::MouseMove && mouse_down_) {
I think you can just look at the modifiers for the MouseMove to see if any
button is down.

https://codereview.chromium.org/902783003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/902783003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.h:142: bool mouse_down_;
This doesn't seem to be actually protected by the mutex.

[jdduke (slow), 2015-02-05 16:03:04]

On 2015/02/05 15:27:28, Sami wrote:
> Should we do this for wheel events first or at the same time? They seem like a
> much stronger signal that we're scrolling.

I believe wheel events are already covered here (they pass through the
keyboard/mouse filter).

[rmcilroy, 2015-02-05 16:14:58]

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

[rmcilroy, 2015-02-05 16:15:00]

https://codereview.chromium.org/902783003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/902783003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:129: // compositor
priority for them.
This comment needs updating (or moved and a new comment for this new block)

[Sami, 2015-02-05 16:51:26]

On 2015/02/05 16:03:04, jdduke wrote:
> On 2015/02/05 15:27:28, Sami wrote:
> > Should we do this for wheel events first or at the same time? They seem like
a
> > much stronger signal that we're scrolling.
> 
> I believe wheel events are already covered here (they pass through the
> keyboard/mouse filter).

Ah, isMouseEventType returns false for wheel events. What a twist.

[alex clarke (OOO till 29th), 2015-02-05 17:47:17]

PTAL

https://codereview.chromium.org/902783003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/902783003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:129: // compositor
priority for them.
On 2015/02/05 16:14:59, rmcilroy wrote:
> This comment needs updating (or moved and a new comment for this new block)

Done.

https://codereview.chromium.org/902783003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:136: if (type ==
blink::WebInputEvent::MouseMove && mouse_down_) {
On 2015/02/05 15:27:28, Sami wrote:
> I think you can just look at the modifiers for the MouseMove to see if any
> button is down.

Done.

https://codereview.chromium.org/902783003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/902783003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.h:142: bool mouse_down_;
On 2015/02/05 15:27:28, Sami wrote:
> This doesn't seem to be actually protected by the mutex.

Acknowledged.

[alex clarke (OOO till 29th), 2015-02-05 17:56:34]

Patchset #3 (id:40001) has been deleted

[picksi, 2015-02-06 09:23:46]

picksi@google.com changed reviewers:
+ picksi@google.com

[picksi, 2015-02-06 09:23:47]

Looks good. One nit/suggestion.

https://codereview.chromium.org/902783003/diff/60001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/902783003/diff/60001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:129:
web_input_event.modifiers & blink::WebInputEvent::LeftButtonDown) {
nit: Should we add blink::WebInputEvent::isLeftMouseButton(web_input_event) to
hide the implementation details and to stylistically match the calls to
blink::WebInputEvent::isMouseEventType(web_input_event.type) later on?

If you think that is too heavy handed, it might add to readability to bracket
the bitwise & parameters, as at first glance it looked to me like there were 3
checks in this if.

[alex clarke (OOO till 29th), 2015-02-06 10:12:21]

https://codereview.chromium.org/902783003/diff/60001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/902783003/diff/60001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:129:
web_input_event.modifiers & blink::WebInputEvent::LeftButtonDown) {
On 2015/02/06 09:23:47, picksi wrote:
> nit: Should we add blink::WebInputEvent::isLeftMouseButton(web_input_event) to
> hide the implementation details and to stylistically match the calls to
> blink::WebInputEvent::isMouseEventType(web_input_event.type) later on?
> 
> If you think that is too heavy handed, it might add to readability to bracket
> the bitwise & parameters, as at first glance it looked to me like there were 3
> checks in this if.

I can't change WebInputEvent in this patch since that's in the blink repository.
 I've added some brackets instead.

[rmcilroy, 2015-02-06 10:19:34]

lgtm, thanks.

https://codereview.chromium.org/902783003/diff/80001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/902783003/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:126: // We regard
MouseMove events with the left mouse button down as a signsal
/s/signsal/signal

https://codereview.chromium.org/902783003/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:133: // Ignore all other
mouse events becuase they probably don't signal user
/s/becuase/because

[alex clarke (OOO till 29th), 2015-02-06 10:25:24]

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

[alex clarke (OOO till 29th), 2015-02-06 10:25:32]

https://codereview.chromium.org/902783003/diff/80001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/902783003/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:126: // We regard
MouseMove events with the left mouse button down as a signsal
On 2015/02/06 10:19:34, rmcilroy wrote:
> /s/signsal/signal

Done.

https://codereview.chromium.org/902783003/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:133: // Ignore all other
mouse events becuase they probably don't signal user
On 2015/02/06 10:19:33, rmcilroy wrote:
> /s/becuase/because

Done.

[alex clarke (OOO till 29th), 2015-02-06 10:29:48]

Patchset #5 (id:100001) has been deleted

[alex clarke (OOO till 29th), 2015-02-06 10:31:32]

alexclarke@chromium.org changed reviewers:
+ sievers@chromium.org

[alex clarke (OOO till 29th), 2015-02-06 10:31:33]

sievers@chromium.org: Please review changes in 

content/test/fake_renderer_scheduler.h
content/test/fake_renderer_scheduler.cc

Thanks!

[jdduke (slow), 2015-02-09 16:15:45]

content/renderer/input lgtm.

I guess the only question I have is whether we have any perf metrics that would
reflect this change's impact on responsiveness? And if not, do we want/need any?
I suppose we have UMA for the time it takes for a MouseMove to hit the main
thread...

Also note that this will kick in when the user is selecting text with the mouse,
which I suppose is fine.

[alex clarke (OOO till 29th), 2015-02-09 16:22:01]

On 2015/02/09 16:15:45, jdduke wrote:
> content/renderer/input lgtm.
> 
> I guess the only question I have is whether we have any perf metrics that
would
> reflect this change's impact on responsiveness? And if not, do we want/need
any?
> I suppose we have UMA for the time it takes for a MouseMove to hit the main
> thread...
> 
> Also note that this will kick in when the user is selecting text with the
mouse,
> which I suppose is fine.

As far as I'm aware we don't have any perf metrics that would track this, which
seems like something that needs addressing.  Subjectively it makes maps smoother
on my desktop.

I know they have some telemetry based testing, I wonder if we could borrow
something.

[alex clarke (OOO till 29th), 2015-02-10 11:52:52]

On 2015/02/09 16:22:01, alexclarke1 wrote:
> On 2015/02/09 16:15:45, jdduke wrote:
> > content/renderer/input lgtm.
> > 
> > I guess the only question I have is whether we have any perf metrics that
> would
> > reflect this change's impact on responsiveness? And if not, do we want/need
> any?
> > I suppose we have UMA for the time it takes for a MouseMove to hit the main
> > thread...
> > 
> > Also note that this will kick in when the user is selecting text with the
> mouse,
> > which I suppose is fine.
> 
> As far as I'm aware we don't have any perf metrics that would track this,
which
> seems like something that needs addressing.  Subjectively it makes maps
smoother
> on my desktop.
> 
> I know they have some telemetry based testing, I wonder if we could borrow
> something.

I filed http://crbug.com/457148 to track this.

[Sami, 2015-02-10 14:02:45]

lgtm. Good idea about adding a benchmark for this.

https://codereview.chromium.org/902783003/diff/120001/content/renderer/schedu...
File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/902783003/diff/120001/content/renderer/schedu...
content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:457: // Note
compositor tasks are not prioritized.
s/are not/are/

[alex clarke (OOO till 29th), 2015-02-10 14:28:58]

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

[alex clarke (OOO till 29th), 2015-02-10 14:33:03]

On 2015/02/10 14:02:45, Sami wrote:
> lgtm. Good idea about adding a benchmark for this.
> 
>
https://codereview.chromium.org/902783003/diff/120001/content/renderer/schedu...
> File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/902783003/diff/120001/content/renderer/schedu...
> content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:457: // Note
> compositor tasks are not prioritized.
> s/are not/are/

As discussed with Sami, adding a TBR for the
content/test/fake_renderer_scheduler.cc & content/test/fake_renderer_scheduler.h
change.

[alex clarke (OOO till 29th), 2015-02-10 14:33:09]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2015-02-10 14:33:58]

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

[commit-bot: I haz the power, 2015-02-10 15:22:37]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-02-10 15:23:28]

Patchset 6 (id:??) landed as
https://crrev.com/1924435ea3211a836b7ef4f8a576285c3cf365f7
Cr-Commit-Position: refs/heads/master@{#315566}