Issue 2514733003: Move EventHandler timers to per-frame task queues.

dcheng

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

4 years, 1 month ago (2016-11-18 06:06:38 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2514733003/1

4 years, 1 month ago (2016-11-18 06:07:01 UTC) #2

dcheng

dcheng@chromium.org changed reviewers: + alexclarke@chromium.org, haraken@chromium.org

4 years, 1 month ago (2016-11-18 06:11:36 UTC) #3

dcheng

Based on the results of this CL, I'm likely to go back and revise what ...

4 years, 1 month ago (2016-11-18 06:11:37 UTC) #4

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-18 10:08:49 UTC) #5

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/275465)

4 years, 1 month ago (2016-11-18 10:08:50 UTC) #6

haraken

haraken@chromium.org changed reviewers: + foolip@chromium.org

4 years, 1 month ago (2016-11-18 10:11:55 UTC) #7

haraken

On 2016/11/18 06:11:37, dcheng wrote: > Based on the results of this CL, I'm likely ...

4 years, 1 month ago (2016-11-18 10:11:55 UTC) #8

foolip

On 2016/11/18 10:11:55, haraken wrote: > On 2016/11/18 06:11:37, dcheng wrote: > > Based on ...

4 years, 1 month ago (2016-11-18 10:47:21 UTC) #9

On 2016/11/18 10:11:55, haraken wrote:
> On 2016/11/18 06:11:37, dcheng wrote:
> > Based on the results of this CL, I'm likely to go back and revise what task
> > source we're using for https://codereview.chromium.org/2505543002/ and
> > https://codereview.chromium.org/2457593002 as well.
> > 
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/input/EventHandler.cpp:167:
> > m_hoverTimer(TaskRunnerHelper::get(TaskType::UserInteraction, &frame),
> > The spec doesn't say that this must happen on the user interaction task
queue.
> > However,
> > https://html.spec.whatwg.org/multipage/webappapis.html#generic-task-sources
> > hints that perhaps this is the desired interaction:
> > 
> > The user interaction task source
> > This task source is used for features that react to user interaction, for
> > example keyboard or mouse input.
> > 
> > This is actually only used by fullscreen: entering fullscreen requires a
user
> > gesture (AFAIK); exiting can be done programatically or with a user gesture
as
> > well. Does the user interaction task source make sense?
> > 
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/input/EventHandler.cpp:170:
> > m_cursorUpdateTimer(TaskRunnerHelper::get(TaskType::Internal, &frame),
> > On the other hand, cursor updates aren't really interesting at all, so they
go
> > in Internal.
> > 
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/input/EventHandler.cpp:186:
> > TaskRunnerHelper::get(TaskType::UserInteraction, &frame),
> > This is used when handling gesture events, so for the same reasons, the user
> > interaction task source seems appropriate.
> 
> Yeah, selectors are defined in 4.16.3 but it's not clear what task source we
> should use for the hover events etc.
> 
> https://html.spec.whatwg.org/#selectors
> 
> foolip@: Any thoughts?

There's no event called "hover", do you mean the mouseenter events and similar?
m_hoverTimer seems to concern the timing of when the :hover selector starts and
stops matching, which of course be observed by element.matches(":hover"). I
don't know if this timing is defined for any selector, but for :hover I suppose
it makes sense for it to change just before the mouse event is fired that caused
the change in state.

Am I even looking at the right question? :)

dcheng

On 2016/11/18 10:47:21, foolip wrote: > On 2016/11/18 10:11:55, haraken wrote: > > On 2016/11/18 ...

4 years, 1 month ago (2016-11-18 17:35:13 UTC) #10

On 2016/11/18 10:47:21, foolip wrote:
> On 2016/11/18 10:11:55, haraken wrote:
> > On 2016/11/18 06:11:37, dcheng wrote:
> > > Based on the results of this CL, I'm likely to go back and revise what
task
> > > source we're using for https://codereview.chromium.org/2505543002/ and
> > > https://codereview.chromium.org/2457593002 as well.
> > > 
> > >
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/input/EventHandler.cpp:167:
> > > m_hoverTimer(TaskRunnerHelper::get(TaskType::UserInteraction, &frame),
> > > The spec doesn't say that this must happen on the user interaction task
> queue.
> > > However,
> > >
https://html.spec.whatwg.org/multipage/webappapis.html#generic-task-sources
> > > hints that perhaps this is the desired interaction:
> > > 
> > > The user interaction task source
> > > This task source is used for features that react to user interaction, for
> > > example keyboard or mouse input.
> > > 
> > > This is actually only used by fullscreen: entering fullscreen requires a
> user
> > > gesture (AFAIK); exiting can be done programatically or with a user
gesture
> as
> > > well. Does the user interaction task source make sense?
> > > 
> > >
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/input/EventHandler.cpp:170:
> > > m_cursorUpdateTimer(TaskRunnerHelper::get(TaskType::Internal, &frame),
> > > On the other hand, cursor updates aren't really interesting at all, so
they
> go
> > > in Internal.
> > > 
> > >
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/input/EventHandler.cpp:186:
> > > TaskRunnerHelper::get(TaskType::UserInteraction, &frame),
> > > This is used when handling gesture events, so for the same reasons, the
user
> > > interaction task source seems appropriate.
> > 
> > Yeah, selectors are defined in 4.16.3 but it's not clear what task source we
> > should use for the hover events etc.
> > 
> > https://html.spec.whatwg.org/#selectors
> > 
> > foolip@: Any thoughts?
> 
> There's no event called "hover", do you mean the mouseenter events and
similar?
> m_hoverTimer seems to concern the timing of when the :hover selector starts
and
> stops matching, which of course be observed by element.matches(":hover"). I
> don't know if this timing is defined for any selector, but for :hover I
suppose
> it makes sense for it to change just before the mouse event is fired that
caused
> the change in state.
> 
> Am I even looking at the right question? :)

For the hover timer:
- The spec doesn't say anything about what task source this update should take
place on.
- However, the hover state update is driven by a fullscreen change. This is
something that is reacting to a user interaction (entering / exiting fullscreen)
- Thus, does it make sense to use the user interaction task source?

A similar question applies for active state changes: the behavior is
underspecified. However, we're updating the active/hover chains in response to a
user gesture... so perhaps the user interaction task source makes sense?

Rick Byers

Description was changed from ========== Move EventHandler timers to per-frame task queues. BUG=624694 ========== to ...

4 years, 1 month ago (2016-11-18 17:57:14 UTC) #11

Rick Byers

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

4 years, 1 month ago (2016-11-18 17:57:15 UTC) #12

Rick Byers

On 2016/11/18 17:35:13, dcheng wrote: > On 2016/11/18 10:47:21, foolip wrote: > > On 2016/11/18 ...

4 years, 1 month ago (2016-11-18 18:00:11 UTC) #13

On 2016/11/18 17:35:13, dcheng wrote:
> On 2016/11/18 10:47:21, foolip wrote:
> > On 2016/11/18 10:11:55, haraken wrote:
> > > On 2016/11/18 06:11:37, dcheng wrote:
> > > > Based on the results of this CL, I'm likely to go back and revise what
> task
> > > > source we're using for https://codereview.chromium.org/2505543002/ and
> > > > https://codereview.chromium.org/2457593002 as well.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > > > File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > > > third_party/WebKit/Source/core/input/EventHandler.cpp:167:
> > > > m_hoverTimer(TaskRunnerHelper::get(TaskType::UserInteraction, &frame),
> > > > The spec doesn't say that this must happen on the user interaction task
> > queue.
> > > > However,
> > > >
> https://html.spec.whatwg.org/multipage/webappapis.html#generic-task-sources
> > > > hints that perhaps this is the desired interaction:
> > > > 
> > > > The user interaction task source
> > > > This task source is used for features that react to user interaction,
for
> > > > example keyboard or mouse input.
> > > > 
> > > > This is actually only used by fullscreen: entering fullscreen requires a
> > user
> > > > gesture (AFAIK); exiting can be done programatically or with a user
> gesture
> > as
> > > > well. Does the user interaction task source make sense?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > > > third_party/WebKit/Source/core/input/EventHandler.cpp:170:
> > > > m_cursorUpdateTimer(TaskRunnerHelper::get(TaskType::Internal, &frame),
> > > > On the other hand, cursor updates aren't really interesting at all, so
> they
> > go
> > > > in Internal.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > > > third_party/WebKit/Source/core/input/EventHandler.cpp:186:
> > > > TaskRunnerHelper::get(TaskType::UserInteraction, &frame),
> > > > This is used when handling gesture events, so for the same reasons, the
> user
> > > > interaction task source seems appropriate.
> > > 
> > > Yeah, selectors are defined in 4.16.3 but it's not clear what task source
we
> > > should use for the hover events etc.
> > > 
> > > https://html.spec.whatwg.org/#selectors
> > > 
> > > foolip@: Any thoughts?
> > 
> > There's no event called "hover", do you mean the mouseenter events and
> similar?
> > m_hoverTimer seems to concern the timing of when the :hover selector starts
> and
> > stops matching, which of course be observed by element.matches(":hover"). I
> > don't know if this timing is defined for any selector, but for :hover I
> suppose
> > it makes sense for it to change just before the mouse event is fired that
> caused
> > the change in state.
> > 
> > Am I even looking at the right question? :)
> 
> For the hover timer:
> - The spec doesn't say anything about what task source this update should take
> place on.
> - However, the hover state update is driven by a fullscreen change. This is
> something that is reacting to a user interaction (entering / exiting
fullscreen)
> - Thus, does it make sense to use the user interaction task source?
> 
> A similar question applies for active state changes: the behavior is
> underspecified. However, we're updating the active/hover chains in response to
a
> user gesture... so perhaps the user interaction task source makes sense?

+tdresser for input performance

Active state is definitely "user interaction", the same as any input events
(it's used most to highlight buttons when pressed, etc.).

I think the same argument should apply to hover state changes (logically should
behave the same as mouseenter/mouseleave events).  My only hesitation there is
that we intentionally deprioritize (throttle/delay) hover state changes in some
cases for scrolling performance.  But we do that consistently for
mousemove/enter/leave and hover and that happens upstream (the events / hover
changes are queued from a timer during a scroll) so probably fine to ignore here
and just use the "user interaction" task source for all hover changes.

foolip

> https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/core/input/EventHandler.cpp#newcode186 > > > > third_party/WebKit/Source/core/input/EventHandler.cpp:186: > > > > TaskRunnerHelper::get(TaskType::UserInteraction, &frame), > > ...

4 years, 1 month ago (2016-11-21 09:41:49 UTC) #14

>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > > > third_party/WebKit/Source/core/input/EventHandler.cpp:186:
> > > > TaskRunnerHelper::get(TaskType::UserInteraction, &frame),
> > > > This is used when handling gesture events, so for the same reasons, the
> user
> > > > interaction task source seems appropriate.
> > > 
> > > Yeah, selectors are defined in 4.16.3 but it's not clear what task source
we
> > > should use for the hover events etc.
> > > 
> > > https://html.spec.whatwg.org/#selectors
> > > 
> > > foolip@: Any thoughts?
> > 
> > There's no event called "hover", do you mean the mouseenter events and
> similar?
> > m_hoverTimer seems to concern the timing of when the :hover selector starts
> and
> > stops matching, which of course be observed by element.matches(":hover"). I
> > don't know if this timing is defined for any selector, but for :hover I
> suppose
> > it makes sense for it to change just before the mouse event is fired that
> caused
> > the change in state.
> > 
> > Am I even looking at the right question? :)
> 
> For the hover timer:
> - The spec doesn't say anything about what task source this update should take
> place on.
> - However, the hover state update is driven by a fullscreen change. This is
> something that is reacting to a user interaction (entering / exiting
fullscreen)
> - Thus, does it make sense to use the user interaction task source?
> 
> A similar question applies for active state changes: the behavior is
> underspecified. However, we're updating the active/hover chains in response to
a
> user gesture... so perhaps the user interaction task source makes sense?

OK, so is this specifically about the EventHandler::scheduleCursorUpdate calls
in Fullscreen::didEnterFullscreenForElement and Fullscreen::didExitFullscreen,
or I guess Document::hoveredNodeDetached is equally problematic?

The fullscreen case was added in https://codereview.chromium.org/552903004,
because when entering fullscreen without moving the mouse the incorrect :hover
state could be quite noticeable. In other situations where the cursor stays
still but the layout under it changes, is the :hover state updated?

I would say that ideally :hover state change should happen immediately before
and input even or when a layout change can be observed, and so any task source
is going to be wrong. Any one that happens to fire after the next layout would
do here, but one can probably create a situation where there's a bad frame.
Going with the user interaction task source and adding a TODO would WFM.

dcheng

On 2016/11/21 09:41:49, foolip wrote: > > > https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/core/input/EventHandler.cpp#newcode186 > > > > > third_party/WebKit/Source/core/input/EventHandler.cpp:186: ...

4 years, 1 month ago (2016-11-21 18:16:30 UTC) #15

On 2016/11/21 09:41:49, foolip wrote:
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > > > > third_party/WebKit/Source/core/input/EventHandler.cpp:186:
> > > > > TaskRunnerHelper::get(TaskType::UserInteraction, &frame),
> > > > > This is used when handling gesture events, so for the same reasons,
the
> > user
> > > > > interaction task source seems appropriate.
> > > > 
> > > > Yeah, selectors are defined in 4.16.3 but it's not clear what task
source
> we
> > > > should use for the hover events etc.
> > > > 
> > > > https://html.spec.whatwg.org/#selectors
> > > > 
> > > > foolip@: Any thoughts?
> > > 
> > > There's no event called "hover", do you mean the mouseenter events and
> > similar?
> > > m_hoverTimer seems to concern the timing of when the :hover selector
starts
> > and
> > > stops matching, which of course be observed by element.matches(":hover").
I
> > > don't know if this timing is defined for any selector, but for :hover I
> > suppose
> > > it makes sense for it to change just before the mouse event is fired that
> > caused
> > > the change in state.
> > > 
> > > Am I even looking at the right question? :)
> > 
> > For the hover timer:
> > - The spec doesn't say anything about what task source this update should
take
> > place on.
> > - However, the hover state update is driven by a fullscreen change. This is
> > something that is reacting to a user interaction (entering / exiting
> fullscreen)
> > - Thus, does it make sense to use the user interaction task source?
> > 
> > A similar question applies for active state changes: the behavior is
> > underspecified. However, we're updating the active/hover chains in response
to
> a
> > user gesture... so perhaps the user interaction task source makes sense?
> 
> OK, so is this specifically about the EventHandler::scheduleCursorUpdate calls
> in Fullscreen::didEnterFullscreenForElement and Fullscreen::didExitFullscreen,
> or I guess Document::hoveredNodeDetached is equally problematic?
> 
> The fullscreen case was added in https://codereview.chromium.org/552903004,
> because when entering fullscreen without moving the mouse the incorrect :hover
> state could be quite noticeable. In other situations where the cursor stays
> still but the layout under it changes, is the :hover state updated?
> 
> I would say that ideally :hover state change should happen immediately before
> and input even or when a layout change can be observed, and so any task source
> is going to be wrong. Any one that happens to fire after the next layout would
> do here, but one can probably create a situation where there's a bad frame.
> Going with the user interaction task source and adding a TODO would WFM.

Shouldn't the throttling / non-throttling of the user interaction task source be
directly tied to whether or not layout throttling is happening?

Or is the bug here that we use a timer at all?

Sami

On 2016/11/21 18:16:30, dcheng wrote: > On 2016/11/21 09:41:49, foolip wrote: > > > > ...

4 years, 1 month ago (2016-11-21 18:56:41 UTC) #16

On 2016/11/21 18:16:30, dcheng wrote:
> On 2016/11/21 09:41:49, foolip wrote:
> > >
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > > > > > third_party/WebKit/Source/core/input/EventHandler.cpp:186:
> > > > > > TaskRunnerHelper::get(TaskType::UserInteraction, &frame),
> > > > > > This is used when handling gesture events, so for the same reasons,
> the
> > > user
> > > > > > interaction task source seems appropriate.
> > > > > 
> > > > > Yeah, selectors are defined in 4.16.3 but it's not clear what task
> source
> > we
> > > > > should use for the hover events etc.
> > > > > 
> > > > > https://html.spec.whatwg.org/#selectors
> > > > > 
> > > > > foolip@: Any thoughts?
> > > > 
> > > > There's no event called "hover", do you mean the mouseenter events and
> > > similar?
> > > > m_hoverTimer seems to concern the timing of when the :hover selector
> starts
> > > and
> > > > stops matching, which of course be observed by
element.matches(":hover").
> I
> > > > don't know if this timing is defined for any selector, but for :hover I
> > > suppose
> > > > it makes sense for it to change just before the mouse event is fired
that
> > > caused
> > > > the change in state.
> > > > 
> > > > Am I even looking at the right question? :)
> > > 
> > > For the hover timer:
> > > - The spec doesn't say anything about what task source this update should
> take
> > > place on.
> > > - However, the hover state update is driven by a fullscreen change. This
is
> > > something that is reacting to a user interaction (entering / exiting
> > fullscreen)
> > > - Thus, does it make sense to use the user interaction task source?
> > > 
> > > A similar question applies for active state changes: the behavior is
> > > underspecified. However, we're updating the active/hover chains in
response
> to
> > a
> > > user gesture... so perhaps the user interaction task source makes sense?
> > 
> > OK, so is this specifically about the EventHandler::scheduleCursorUpdate
calls
> > in Fullscreen::didEnterFullscreenForElement and
Fullscreen::didExitFullscreen,
> > or I guess Document::hoveredNodeDetached is equally problematic?
> > 
> > The fullscreen case was added in https://codereview.chromium.org/552903004,
> > because when entering fullscreen without moving the mouse the incorrect
:hover
> > state could be quite noticeable. In other situations where the cursor stays
> > still but the layout under it changes, is the :hover state updated?
> > 
> > I would say that ideally :hover state change should happen immediately
before
> > and input even or when a layout change can be observed, and so any task
source
> > is going to be wrong. Any one that happens to fire after the next layout
would
> > do here, but one can probably create a situation where there's a bad frame.
> > Going with the user interaction task source and adding a TODO would WFM.
> 
> Shouldn't the throttling / non-throttling of the user interaction task source
be
> directly tied to whether or not layout throttling is happening?
> 
> Or is the bug here that we use a timer at all?

My feeling is that it's the latter. The update rate is set to 50 Hz [1] which is
close enough to the typical 60 Hz that I think we should just schedule normal
BeginFrames for these updates.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/Eve...

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2514733003/20001

4 years ago (2016-11-26 00:54:33 UTC) #18

dcheng

On 2016/11/21 18:56:41, Sami wrote: > On 2016/11/21 18:16:30, dcheng wrote: > > On 2016/11/21 ...

4 years ago (2016-11-26 00:54:39 UTC) #19

On 2016/11/21 18:56:41, Sami wrote:
> On 2016/11/21 18:16:30, dcheng wrote:
> > On 2016/11/21 09:41:49, foolip wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2514733003/diff/1/third_party/WebKit/Source/c...
> > > > > > > third_party/WebKit/Source/core/input/EventHandler.cpp:186:
> > > > > > > TaskRunnerHelper::get(TaskType::UserInteraction, &frame),
> > > > > > > This is used when handling gesture events, so for the same
reasons,
> > the
> > > > user
> > > > > > > interaction task source seems appropriate.
> > > > > > 
> > > > > > Yeah, selectors are defined in 4.16.3 but it's not clear what task
> > source
> > > we
> > > > > > should use for the hover events etc.
> > > > > > 
> > > > > > https://html.spec.whatwg.org/#selectors
> > > > > > 
> > > > > > foolip@: Any thoughts?
> > > > > 
> > > > > There's no event called "hover", do you mean the mouseenter events and
> > > > similar?
> > > > > m_hoverTimer seems to concern the timing of when the :hover selector
> > starts
> > > > and
> > > > > stops matching, which of course be observed by
> element.matches(":hover").
> > I
> > > > > don't know if this timing is defined for any selector, but for :hover
I
> > > > suppose
> > > > > it makes sense for it to change just before the mouse event is fired
> that
> > > > caused
> > > > > the change in state.
> > > > > 
> > > > > Am I even looking at the right question? :)
> > > > 
> > > > For the hover timer:
> > > > - The spec doesn't say anything about what task source this update
should
> > take
> > > > place on.
> > > > - However, the hover state update is driven by a fullscreen change. This
> is
> > > > something that is reacting to a user interaction (entering / exiting
> > > fullscreen)
> > > > - Thus, does it make sense to use the user interaction task source?
> > > > 
> > > > A similar question applies for active state changes: the behavior is
> > > > underspecified. However, we're updating the active/hover chains in
> response
> > to
> > > a
> > > > user gesture... so perhaps the user interaction task source makes sense?
> > > 
> > > OK, so is this specifically about the EventHandler::scheduleCursorUpdate
> calls
> > > in Fullscreen::didEnterFullscreenForElement and
> Fullscreen::didExitFullscreen,
> > > or I guess Document::hoveredNodeDetached is equally problematic?
> > > 
> > > The fullscreen case was added in
https://codereview.chromium.org/552903004,
> > > because when entering fullscreen without moving the mouse the incorrect
> :hover
> > > state could be quite noticeable. In other situations where the cursor
stays
> > > still but the layout under it changes, is the :hover state updated?
> > > 
> > > I would say that ideally :hover state change should happen immediately
> before
> > > and input even or when a layout change can be observed, and so any task
> source
> > > is going to be wrong. Any one that happens to fire after the next layout
> would
> > > do here, but one can probably create a situation where there's a bad
frame.
> > > Going with the user interaction task source and adding a TODO would WFM.
> > 
> > Shouldn't the throttling / non-throttling of the user interaction task
source
> be
> > directly tied to whether or not layout throttling is happening?
> > 
> > Or is the bug here that we use a timer at all?
> 
> My feeling is that it's the latter. The update rate is set to 50 Hz [1] which
is
> close enough to the typical 60 Hz that I think we should just schedule normal
> BeginFrames for these updates.
> 
> [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/Eve...

OK, added some TODOs to move to BeginFrame for these updates.

commit-bot: I haz the power

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

4 years ago (2016-11-26 01:05:50 UTC) #20

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/170368)

4 years ago (2016-11-26 01:05:50 UTC) #21

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2514733003/40001

4 years ago (2016-11-26 01:18:01 UTC) #23

commit-bot: I haz the power

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

4 years ago (2016-11-26 03:40:47 UTC) #24

foolip

Explicitly deferring to other reviewers, please poke if there's something I should look at.

4 years ago (2016-11-30 13:08:08 UTC) #28

tdresser

As we're migrating towards following the chromium style guide in blink, it seems a bit ...

4 years ago (2016-11-30 13:18:24 UTC) #29

dcheng

On 2016/11/30 13:18:24, tdresser wrote: > As we're migrating towards following the chromium style guide ...

4 years ago (2016-11-30 17:10:31 UTC) #30

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2514733003/40001

4 years ago (2016-11-30 17:11:09 UTC) #32

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480525847281980, "parent_rev": "0e05c5d283eb44c55ce34c22791ffff27acb9034", "commit_rev": "130f9c7ec47dce2f361b478fb3e051359ae480ea"}

4 years ago (2016-11-30 18:38:36 UTC) #34

commit-bot: I haz the power

Description was changed from ========== Move EventHandler timers to per-frame task queues. BUG=624694 ========== to ...

4 years ago (2016-11-30 18:39:03 UTC) #35

commit-bot: I haz the power

Description was changed from ========== Move EventHandler timers to per-frame task queues. BUG=624694 ========== to ...

4 years ago (2016-11-30 18:42:25 UTC) #37

commit-bot: I haz the power

Patchset 3 (id:??) landed as https://crrev.com/b8252b996657abe9e655c1b78bc15edb03821870 Cr-Commit-Position: refs/heads/master@{#435352}

4 years ago (2016-11-30 18:42:26 UTC) #38

Łukasz Anforowicz

4 years ago (2016-11-30 23:49:13 UTC) #39

Message was sent while issue was closed.

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2541073004/ by lukasza@chromium.org.

The reason for reverting is: After this CL
http/tests/security/opened-document-security-origin-resets-on-navigation.html
layout test times out when run with --site-per-process flag.  See also
https://crbug.com/670105..

Issue 2514733003: Move EventHandler timers to per-frame task queues. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Add TODOs #

Patch Set 3 : Remove stray brace #

Messages