Schedule cursor update when DOM updates layout.

Schedule cursor update when DOM updates layout.

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

[zhengw, 2015-02-10 07:51:45]

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

[Rick Byers, 2015-02-12 00:07:13]

Thanks Zheng.  This is a good start.

You're going to need some sort of test for this.  Testing timing-dependent
things (without mock timers) can be a little tricky - I think you'll need to
effectively sleep in the test for long enough to ensure the cursor update timer
has had a chance to fire.

https://codereview.chromium.org/915543003/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/915543003/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:1893: frame->eventHandler().scheduleCursorUpdate();
This is a good start.  I think this could trigger to aggressively though - eg.
if frameView->needsLayout is false then we probably don't want to schedule an
update.  The idea here is that we need to update the cursor only if layout
actually changed (not just if something thinks they need to check to see if
layout should be updated).

Digging down through this code, it looks like  FrameView::performPostLayoutTasks
(which gets invoked from the frameView->layout call below) is probably a better
place.

[zhengw, 2015-02-12 22:01:54]

On 2015/02/12 00:07:13, Rick Byers wrote:
> Thanks Zheng.  This is a good start.
> 
> You're going to need some sort of test for this.  Testing timing-dependent
> things (without mock timers) can be a little tricky - I think you'll need to
> effectively sleep in the test for long enough to ensure the cursor update
timer
> has had a chance to fire.
> 
> https://codereview.chromium.org/915543003/diff/1/Source/core/dom/Document.cpp
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/915543003/diff/1/Source/core/dom/Document.cpp...
> Source/core/dom/Document.cpp:1893:
frame->eventHandler().scheduleCursorUpdate();
> This is a good start.  I think this could trigger to aggressively though - eg.
> if frameView->needsLayout is false then we probably don't want to schedule an
> update.  The idea here is that we need to update the cursor only if layout
> actually changed (not just if something thinks they need to check to see if
> layout should be updated).
> 
> Digging down through this code, it looks like 
FrameView::performPostLayoutTasks
> (which gets invoked from the frameView->layout call below) is probably a
better
> place.

Thank you, Rick. Now it only updates the cursor when a layout is needed.

Since it looks like the original issue has been partially fixed, I think we need
a new test to test the remaining case. I'm preparing such a test based on the
one I wrote later:

http://jsbin.com/jijovuhilo

I think `setTimeout` should be able to help here as it's used in other tests as
well.

[Rick Byers, 2015-02-13 08:06:39]

On 2015/02/12 22:01:54, zhengw wrote:
> On 2015/02/12 00:07:13, Rick Byers wrote:
> > Thanks Zheng.  This is a good start.
> > 
> > You're going to need some sort of test for this.  Testing timing-dependent
> > things (without mock timers) can be a little tricky - I think you'll need to
> > effectively sleep in the test for long enough to ensure the cursor update
> timer
> > has had a chance to fire.
> > 
> >
https://codereview.chromium.org/915543003/diff/1/Source/core/dom/Document.cpp
> > File Source/core/dom/Document.cpp (right):
> > 
> >
>
https://codereview.chromium.org/915543003/diff/1/Source/core/dom/Document.cpp...
> > Source/core/dom/Document.cpp:1893:
> frame->eventHandler().scheduleCursorUpdate();
> > This is a good start.  I think this could trigger to aggressively though -
eg.
> > if frameView->needsLayout is false then we probably don't want to schedule
an
> > update.  The idea here is that we need to update the cursor only if layout
> > actually changed (not just if something thinks they need to check to see if
> > layout should be updated).
> > 
> > Digging down through this code, it looks like 
> FrameView::performPostLayoutTasks
> > (which gets invoked from the frameView->layout call below) is probably a
> better
> > place.
> 
> Thank you, Rick. Now it only updates the cursor when a layout is needed.
> 
> Since it looks like the original issue has been partially fixed, I think we
need
> a new test to test the remaining case. I'm preparing such a test based on the
> one I wrote later:
> 
> http://jsbin.com/jijovuhilo
> 
> I think `setTimeout` should be able to help here as it's used in other tests
as
> well.

Yes, exactly.  It sucks having to use timeouts in tests - need to be careful to
ensure they're long enough to never be flaky without also making the test
unnecessarily slow.  But I don't see any alternative here.  Just be careful to
document precisely why your timeout value is enough (eg. if we see flakiness in
the future, we can come easily check that the time values you're copying from
blink are still the same).   See http://crbug.com/319529 for when I tried to get
this done the right way (a mock timer system) but got shot because "the benefit
wasn't worth the added complexity".

[zhengw, 2015-02-13 21:07:33]

On 2015/02/13 08:06:39, Rick Byers wrote:
> On 2015/02/12 22:01:54, zhengw wrote:
> > On 2015/02/12 00:07:13, Rick Byers wrote:
> > > Thanks Zheng.  This is a good start.
> > > 
> > > You're going to need some sort of test for this.  Testing timing-dependent
> > > things (without mock timers) can be a little tricky - I think you'll need
to
> > > effectively sleep in the test for long enough to ensure the cursor update
> > timer
> > > has had a chance to fire.
> > > 
> > >
> https://codereview.chromium.org/915543003/diff/1/Source/core/dom/Document.cpp
> > > File Source/core/dom/Document.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/915543003/diff/1/Source/core/dom/Document.cpp...
> > > Source/core/dom/Document.cpp:1893:
> > frame->eventHandler().scheduleCursorUpdate();
> > > This is a good start.  I think this could trigger to aggressively though -
> eg.
> > > if frameView->needsLayout is false then we probably don't want to schedule
> an
> > > update.  The idea here is that we need to update the cursor only if layout
> > > actually changed (not just if something thinks they need to check to see
if
> > > layout should be updated).
> > > 
> > > Digging down through this code, it looks like 
> > FrameView::performPostLayoutTasks
> > > (which gets invoked from the frameView->layout call below) is probably a
> > better
> > > place.
> > 
> > Thank you, Rick. Now it only updates the cursor when a layout is needed.
> > 
> > Since it looks like the original issue has been partially fixed, I think we
> need
> > a new test to test the remaining case. I'm preparing such a test based on
the
> > one I wrote later:
> > 
> > http://jsbin.com/jijovuhilo
> > 
> > I think `setTimeout` should be able to help here as it's used in other tests
> as
> > well.
> 
> Yes, exactly.  It sucks having to use timeouts in tests - need to be careful
to
> ensure they're long enough to never be flaky without also making the test
> unnecessarily slow.  But I don't see any alternative here.  Just be careful to
> document precisely why your timeout value is enough (eg. if we see flakiness
in
> the future, we can come easily check that the time values you're copying from
> blink are still the same).   See http://crbug.com/319529 for when I tried to
get
> this done the right way (a mock timer system) but got shot because "the
benefit
> wasn't worth the added complexity".

[zhengw, 2015-02-13 21:16:07]

On 2015/02/13 21:07:33, zhengw wrote:
> On 2015/02/13 08:06:39, Rick Byers wrote:
> > On 2015/02/12 22:01:54, zhengw wrote:
> > > On 2015/02/12 00:07:13, Rick Byers wrote:
> > > > Thanks Zheng.  This is a good start.
> > > > 
> > > > You're going to need some sort of test for this.  Testing
timing-dependent
> > > > things (without mock timers) can be a little tricky - I think you'll
need
> to
> > > > effectively sleep in the test for long enough to ensure the cursor
update
> > > timer
> > > > has had a chance to fire.
> > > > 
> > > >
> >
https://codereview.chromium.org/915543003/diff/1/Source/core/dom/Document.cpp
> > > > File Source/core/dom/Document.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/915543003/diff/1/Source/core/dom/Document.cpp...
> > > > Source/core/dom/Document.cpp:1893:
> > > frame->eventHandler().scheduleCursorUpdate();
> > > > This is a good start.  I think this could trigger to aggressively though
-
> > eg.
> > > > if frameView->needsLayout is false then we probably don't want to
schedule
> > an
> > > > update.  The idea here is that we need to update the cursor only if
layout
> > > > actually changed (not just if something thinks they need to check to see
> if
> > > > layout should be updated).
> > > > 
> > > > Digging down through this code, it looks like 
> > > FrameView::performPostLayoutTasks
> > > > (which gets invoked from the frameView->layout call below) is probably a
> > > better
> > > > place.
> > > 
> > > Thank you, Rick. Now it only updates the cursor when a layout is needed.
> > > 
> > > Since it looks like the original issue has been partially fixed, I think
we
> > need
> > > a new test to test the remaining case. I'm preparing such a test based on
> the
> > > one I wrote later:
> > > 
> > > http://jsbin.com/jijovuhilo
> > > 
> > > I think `setTimeout` should be able to help here as it's used in other
tests
> > as
> > > well.
> > 
> > Yes, exactly.  It sucks having to use timeouts in tests - need to be careful
> to
> > ensure they're long enough to never be flaky without also making the test
> > unnecessarily slow.  But I don't see any alternative here.  Just be careful
to
> > document precisely why your timeout value is enough (eg. if we see flakiness
> in
> > the future, we can come easily check that the time values you're copying
from
> > blink are still the same).   See http://crbug.com/319529 for when I tried to
> get
> > this done the right way (a mock timer system) but got shot because "the
> benefit
> > wasn't worth the added complexity".

Tests added. The trade-off you pointed out is correct and I think it's also
general, i.e. appears in other software testing as well. Some of them may have a
mock (or use a special callback to notify) but others don't. When using a
timeout, I usually get some idea from previous work in this case. There are some
existing tests which give 50ms for an update. I tested 10 times with this value
and it looks good enough. Using this value may also help maintain consistency.

[Rick Byers, 2015-02-20 20:54:00]

lgtm with nit

https://codereview.chromium.org/915543003/diff/40001/LayoutTests/fast/events/...
File LayoutTests/fast/events/mouse-cursor-change-after-layout.html (right):

https://codereview.chromium.org/915543003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/mouse-cursor-change-after-layout.html:30: <p><a
href="https://bugs.webkit.org/show_bug.cgi?id=26723">Bug 26723</a></p>
nit: this a chrome bug, not webkit (which is from before we forked blink).  So
please change the URL to crbug.com/26723.

[zhengw, 2015-02-20 21:03:58]

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

[zhengw, 2015-02-20 21:05:10]

https://codereview.chromium.org/915543003/diff/40001/LayoutTests/fast/events/...
File LayoutTests/fast/events/mouse-cursor-change-after-layout.html (right):

https://codereview.chromium.org/915543003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/mouse-cursor-change-after-layout.html:30: <p><a
href="https://bugs.webkit.org/show_bug.cgi?id=26723">Bug 26723</a></p>
On 2015/02/20 20:54:00, Rick Byers wrote:
> nit: this a chrome bug, not webkit (which is from before we forked blink).  So
> please change the URL to crbug.com/26723.

Right, thanks for pointing out!

[zhengw, 2015-02-23 15:52:08]

The CQ bit was checked by zhengw@chromium.org

[commit-bot: I haz the power, 2015-02-23 15:52:40]

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

[commit-bot: I haz the power, 2015-02-23 17:53:08]

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

[commit-bot: I haz the power, 2015-02-23 17:53:09]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.blink (JOB_TIMED_OUT, no build
URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[zhengw, 2015-02-23 18:50:00]

The CQ bit was checked by zhengw@chromium.org

[commit-bot: I haz the power, 2015-02-23 18:50:47]

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

[commit-bot: I haz the power, 2015-02-23 18:51:28]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190676

[Rick Byers, 2015-04-30 14:50:17]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1120653002/ by rbyers@chromium.org.

The reason for reverting is: Exposed mouse cursor flicker regression:
http://crbug.com/468641 (due to a pre-existing bug - http://crbug.com/482971)..