Advance document lifecycle to CompositingClean prior to hit testing

Advance document lifecycle to CompositingClean prior to hit testing

This CL is motivated by https://codereview.chromium.org/976543002.
In that CL, we need to query CompositingState in collectFragments()
which is also called when doing hit testing. 

BUG=None

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

[qiankun, 2015-03-11 15:10:14]

qiankun.miao@intel.com changed reviewers:
+ chrishtr@chromium.org, leviw@chromium.org

[qiankun, 2015-03-11 15:10:14]

I worked out an initial version for advancing document lifecycle to
CompositingClean prior to hit testing. I am not sure about it. Could you give
some advice?

[leviw_travelin_and_unemployed, 2015-03-11 21:00:55]

On 2015/03/11 at 15:10:14, qiankun.miao wrote:
> I worked out an initial version for advancing document lifecycle to
CompositingClean prior to hit testing. I am not sure about it. Could you give
some advice?

You're on the right track! I'd add also like to see an assert that we don't
enter hit testing code when the DocumentLifecycle isn't at least
CompositingClean.

[qiankun, 2015-03-12 09:50:01]

Updated the patch. PTAL.

[leviw_travelin_and_unemployed, 2015-03-12 20:09:32]

https://codereview.chromium.org/999033002/diff/20001/Source/core/dom/Document...
File Source/core/dom/DocumentLifecycle.cpp (right):

https://codereview.chromium.org/999033002/diff/20001/Source/core/dom/Document...
Source/core/dom/DocumentLifecycle.cpp:133: if (state == InPreLayout)
Why this change?

https://codereview.chromium.org/999033002/diff/20001/Source/core/layout/Layou...
File Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/999033002/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutView.cpp:102: ASSERT(document().lifecycle().state() >=
DocumentLifecycle::CompositingClean);
This assert should go somewhere deeper in Blink, like Layer::hitTestLayer

[qiankun, 2015-03-13 03:23:10]

https://codereview.chromium.org/999033002/diff/20001/Source/core/dom/Document...
File Source/core/dom/DocumentLifecycle.cpp (right):

https://codereview.chromium.org/999033002/diff/20001/Source/core/dom/Document...
Source/core/dom/DocumentLifecycle.cpp:133: if (state == InPreLayout)
On 2015/03/12 20:09:32, leviw wrote:
> Why this change?

When running layout test, LayoutTreeAsText::writeLayers() will call layout()
again after hit testing. If we don't allow changing state from CompositingClean
to InPreLayout, layout test will crash. Attached crash stack for running
fast/css/resize-corner-tracking.html:
#0 0x0000008bd59e base::debug::StackTrace::StackTrace()
#1 0x0000008bd0d3 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7ffbe87e4210 <unknown>
#3 0x00000283e7d4 blink::DocumentLifecycle::advanceTo()
#4 0x000002ee3b4f blink::FrameView::performPreLayoutTasks()
#5 0x000002ee4b39 blink::FrameView::layout()
#6 0x0000033d9b89 blink::write()
#7 0x0000033daf31 blink::write()
#8 0x0000033da3f0 blink::LayoutTreeAsText::writeLayers()
#9 0x0000033da52d blink::LayoutTreeAsText::writeLayers()
#10 0x0000033da685 blink::LayoutTreeAsText::writeLayers()
#11 0x0000033db300 blink::externalRepresentation()
#12 0x0000033db220 blink::externalRepresentation()
#13 0x000002212cff blink::WebLocalFrameImpl::layoutTreeAsText()
#14 0x000000527d7c content::WebTestProxyBase::CaptureTree()
#15 0x00000051be34 content::WebKitTestRunner::CaptureDump()
#16 0x00000051bb1c content::WebKitTestRunner::TestFinished()
#17 0x00000051c0ac content::WebKitTestRunner::TestFinished()
#18 0x00000058fab4 content::TestRunner::WorkQueue::ProcessWorkSoon()
#19 0x000000590e65 content::TestRunner::LocationChangeDone()
#20 0x000000590dbe content::TestRunner::setTopLoadingFrame()

https://codereview.chromium.org/999033002/diff/20001/Source/core/layout/Layou...
File Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/999033002/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutView.cpp:102: ASSERT(document().lifecycle().state() >=
DocumentLifecycle::CompositingClean);
On 2015/03/12 20:09:32, leviw wrote:
> This assert should go somewhere deeper in Blink, like Layer::hitTestLayer

Done.

[qiankun, 2015-03-19 04:46:13]

Hi Levi, please take a look at it when you free. Thanks.

[leviw_travelin_and_unemployed, 2015-03-19 15:42:21]

Can you link this to the other codereview in the description for context?

LGTM.

[qiankun, 2015-03-19 16:31:25]

On 2015/03/19 15:42:21, leviw wrote:
> Can you link this to the other codereview in the description for context?
> 
> LGTM.

Done.

[qiankun, 2015-03-19 23:56:32]

The CQ bit was checked by qiankun.miao@intel.com

[commit-bot: I haz the power, 2015-03-19 23:57:10]

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

[commit-bot: I haz the power, 2015-03-20 00:00:38]

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

[commit-bot: I haz the power, 2015-03-20 00:00:40]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/48314)

[qiankun, 2015-03-20 00:27:35]

The CQ bit was checked by qiankun.miao@intel.com

[qiankun, 2015-03-20 00:27:35]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/999033002/#ps60001 (title:
"rebase to ToT")

[commit-bot: I haz the power, 2015-03-20 00:28:16]

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

[commit-bot: I haz the power, 2015-03-20 01:14:50]

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

[commit-bot: I haz the power, 2015-03-20 01:14:52]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/48322)

[qiankun, 2015-03-20 07:50:22]

https://codereview.chromium.org/999033002/diff/80001/Source/web/WebLocalFrame...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/999033002/diff/80001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1305:
frame()->document()->layoutView()->hitTest(request, result);
Also advance to CompositingClean state here.

[qiankun, 2015-03-23 23:19:22]

On 2015/03/20 07:50:22, qiankun wrote:
>
https://codereview.chromium.org/999033002/diff/80001/Source/web/WebLocalFrame...
> File Source/web/WebLocalFrameImpl.cpp (right):
> 
>
https://codereview.chromium.org/999033002/diff/80001/Source/web/WebLocalFrame...
> Source/web/WebLocalFrameImpl.cpp:1305:
> frame()->document()->layoutView()->hitTest(request, result);
> Also advance to CompositingClean state here.

levi@, please help to review this change.

[leviw_travelin_and_unemployed, 2015-03-24 22:55:19]

You'll need a stamp from Chris for Source/web (I don't own that subdir)

[esprehn, 2015-03-24 23:01:02]

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

[esprehn, 2015-03-24 23:01:03]

Is there a bug for this? What about hit testing needs compositing state?

The change description could have a lot more detail. :)

https://codereview.chromium.org/999033002/diff/100001/Source/core/dom/Documen...
File Source/core/dom/DocumentLifecycle.cpp (right):

https://codereview.chromium.org/999033002/diff/100001/Source/core/dom/Documen...
Source/core/dom/DocumentLifecycle.cpp:133: if (state == InPreLayout)
If we're in pre layout tasks why would we advance directly to CompositingClean?
This patch doesn't seem to explain the reason for this.

[esprehn, 2015-03-24 23:06:05]

https://codereview.chromium.org/999033002/diff/20001/Source/core/dom/Document...
File Source/core/dom/DocumentLifecycle.cpp (right):

https://codereview.chromium.org/999033002/diff/20001/Source/core/dom/Document...
Source/core/dom/DocumentLifecycle.cpp:133: if (state == InPreLayout)
On 2015/03/13 at 03:23:10, qiankun wrote:
> On 2015/03/12 20:09:32, leviw wrote:
> > Why this change?
> 
> When running layout test, LayoutTreeAsText::writeLayers() will call layout()
again after hit testing. If we don't allow changing state from CompositingClean
to InPreLayout, layout test will crash. Attached crash stack for running
fast/css/resize-corner-tracking.html:
> #0 0x0000008bd59e base::debug::StackTrace::StackTrace()
> #1 0x0000008bd0d3 base::debug::(anonymous namespace)::StackDumpSignalHandler()
> #2 0x7ffbe87e4210 <unknown>
> #3 0x00000283e7d4 blink::DocumentLifecycle::advanceTo()
> #4 0x000002ee3b4f blink::FrameView::performPreLayoutTasks()
> #5 0x000002ee4b39 blink::FrameView::layout()
> #6 0x0000033d9b89 blink::write()
> #7 0x0000033daf31 blink::write()
> #8 0x0000033da3f0 blink::LayoutTreeAsText::writeLayers()

I think this is a bug in LayoutTreeAsText::writeLayers, it wants to do
view->document()->updateLayout() instead.

[qiankun, 2015-03-25 03:13:00]

On 2015/03/24 23:06:05, esprehn wrote:
>
https://codereview.chromium.org/999033002/diff/20001/Source/core/dom/Document...
> File Source/core/dom/DocumentLifecycle.cpp (right):
> 
>
https://codereview.chromium.org/999033002/diff/20001/Source/core/dom/Document...
> Source/core/dom/DocumentLifecycle.cpp:133: if (state == InPreLayout)
> On 2015/03/13 at 03:23:10, qiankun wrote:
> > On 2015/03/12 20:09:32, leviw wrote:
> > > Why this change?
> > 
> > When running layout test, LayoutTreeAsText::writeLayers() will call layout()
> again after hit testing. If we don't allow changing state from
CompositingClean
> to InPreLayout, layout test will crash. Attached crash stack for running
> fast/css/resize-corner-tracking.html:
> > #0 0x0000008bd59e base::debug::StackTrace::StackTrace()
> > #1 0x0000008bd0d3 base::debug::(anonymous
namespace)::StackDumpSignalHandler()
> > #2 0x7ffbe87e4210 <unknown>
> > #3 0x00000283e7d4 blink::DocumentLifecycle::advanceTo()
> > #4 0x000002ee3b4f blink::FrameView::performPreLayoutTasks()
> > #5 0x000002ee4b39 blink::FrameView::layout()
> > #6 0x0000033d9b89 blink::write()
> > #7 0x0000033daf31 blink::write()
> > #8 0x0000033da3f0 blink::LayoutTreeAsText::writeLayers()
> 
> I think this is a bug in LayoutTreeAsText::writeLayers, it wants to do
> view->document()->updateLayout() instead.

I will take a look the code in LayoutTreeAsText::writeLayers.

[qiankun, 2015-03-25 03:13:59]

I will add context for this CL in description.

https://codereview.chromium.org/999033002/diff/100001/Source/core/dom/Documen...
File Source/core/dom/DocumentLifecycle.cpp (right):

https://codereview.chromium.org/999033002/diff/100001/Source/core/dom/Documen...
Source/core/dom/DocumentLifecycle.cpp:133: if (state == InPreLayout)
On 2015/03/24 23:01:03, esprehn wrote:
> If we're in pre layout tasks why would we advance directly to
CompositingClean?
> This patch doesn't seem to explain the reason for this.
It allows advancing document life cycle from CompositingClean to preLayout. I
fixed the case calls layout() after hit testing.

There is no bug for hit testing. This change is motivated by
https://codereview.chromium.org/976543002. In order to query compositingState in
collectFragments(), levi@ and chrishtr@ suggested that we should ensure that the
document lifecycle is advanced to CompositingClean prior to hit testing.

[qiankun, 2015-04-07 10:02:26]

Hi esprehn, I updated the patch which removes state change from CompositingClean
to InPreLayout after https://codereview.chromium.org/1054423002/ landed. Could
you take a look at the new CL. Thanks!

[qiankun, 2015-04-09 15:21:23]

On 2015/04/07 10:02:26, qiankun wrote:
> Hi esprehn, I updated the patch which removes state change from
CompositingClean
> to InPreLayout after https://codereview.chromium.org/1054423002/ landed. Could
> you take a look at the new CL. Thanks!

Ping esprehn@

[esprehn, 2015-04-09 19:38:21]

lgtm

[qiankun, 2015-04-09 23:29:43]

The CQ bit was checked by qiankun.miao@intel.com

[qiankun, 2015-04-09 23:29:43]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/999033002/#ps120001
(title: "remove state change from CompositingClean to InPreLayout")

[commit-bot: I haz the power, 2015-04-09 23:30:03]

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

[commit-bot: I haz the power, 2015-04-10 03:35:24]

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

[commit-bot: I haz the power, 2015-04-10 03:35:26]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/58642)

[qiankun, 2015-04-10 05:50:22]

qiankun.miao@intel.com changed reviewers:
+ yunchao.he@intel.com

[qiankun, 2015-04-10 07:19:28]

Move compositor()->updateIfNeededRecursive down to layer->hitTest() from
LayoutView::hitTest(). With the change, all direct
DeprecatedPaintLayer::hitTest() call can advance document lifecycle to
CompositingClean prior to hit testing.

leviw@, please review it again. Thanks.

[qiankun, 2015-04-10 07:20:26]

qiankun.miao@intel.com changed reviewers:
- yunchao.he@intel.com

[leviw_travelin_and_unemployed, 2015-04-10 19:27:09]

On 2015/04/10 at 07:19:28, qiankun.miao wrote:
> Move compositor()->updateIfNeededRecursive down to layer->hitTest() from
LayoutView::hitTest(). With the change, all direct
DeprecatedPaintLayer::hitTest() call can advance document lifecycle to
CompositingClean prior to hit testing.
> 
> leviw@, please review it again. Thanks.

Looks like we were failing when called from Accessibility code before, is that
right?

LGTM.

[qiankun, 2015-04-10 23:58:08]

On 2015/04/10 19:27:09, leviw wrote:
> On 2015/04/10 at 07:19:28, qiankun.miao wrote:
> > Move compositor()->updateIfNeededRecursive down to layer->hitTest() from
> LayoutView::hitTest(). With the change, all direct
> DeprecatedPaintLayer::hitTest() call can advance document lifecycle to
> CompositingClean prior to hit testing.
> > 
> > leviw@, please review it again. Thanks.
> 
> Looks like we were failing when called from Accessibility code before, is that
> right?
> 
> LGTM.

Yes. Layout tests failed on some Accessibility code.

[qiankun, 2015-04-10 23:58:42]

The CQ bit was checked by qiankun.miao@intel.com

[qiankun, 2015-04-10 23:58:42]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/999033002/#ps140001
(title: "move compositor()->updateIfNeededRecursive down to layer->hitTest()")

[commit-bot: I haz the power, 2015-04-10 23:59:16]

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

[commit-bot: I haz the power, 2015-04-11 02:57:59]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193585