Rewind the DocumentLifecycle when dirtying state

Rewind the DocumentLifecycle when dirtying state

Before this CL, we didn't actually rewind the DocumentLifecycle when we
scheduled layout or compositing updates. This CL adds the rewind and finds some
cases where we're doing something sketchy (e.g., scheduling a layout after
detaching a document from its frame).

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

[abarth-chromium, 2014-04-09 21:27:36]

https://codereview.chromium.org/232013002/diff/1/Source/core/frame/LocalFrame...
File Source/core/frame/LocalFrame.cpp (left):

https://codereview.chromium.org/232013002/diff/1/Source/core/frame/LocalFrame...
Source/core/frame/LocalFrame.cpp:399: ASSERT(document()->lifecycle().state() >=
DocumentLifecycle::CompositingClean);
RenderLayerCompositor's layerTreeAsText has this ASSERT already.

[eseidel, 2014-04-09 21:31:46]

lgtm

[commit-bot: I haz the power, 2014-04-09 21:31:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/232013002/1

[eseidel, 2014-04-09 21:31:59]

Good to relate a meta bug if you hav eone.

[esprehn, 2014-04-09 21:32:24]

lgtm. Ideally we'd stop calling scheduleVisualUpdate() except by doing
lifecycle().scheduleVisualUpdate() which would change the state to
UpdatePending.

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

https://codereview.chromium.org/232013002/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:2636: || (documentElement() &&
!isHTMLHtmlElement(*documentElement()));
Can we split these too so it's easier to read?

if (isRenderingReady() && body())
  return true;
if (documentElement() && !isHTMLHtmlElement(*documentElement()))
  return true;
return false

https://codereview.chromium.org/232013002/diff/1/Source/core/frame/FrameView.cpp
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/232013002/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:1820: if (!m_frame->document()->isActive())
Ugh yeah, we need to have a single "UpdateScheduler" that does all this.

https://codereview.chromium.org/232013002/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:1858:
lifecycle().ensureStateAtMost(DocumentLifecycle::StyleClean);
Ideally we'd just schedule an update. This should be at lifecycle of
UpdatePending. No one should call scheduleVisualUpdate() directly and not change
the lifecycle.

https://codereview.chromium.org/232013002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/232013002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:95: // the
compositing update, but that happens in
compositing/visibility/hidden-iframe.html.
This is scheduleLayerUpdate() at work.

[abarth-chromium, 2014-04-09 21:34:38]

The CQ bit was unchecked by abarth@chromium.org

[abarth-chromium, 2014-04-09 21:41:26]

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

https://codereview.chromium.org/232013002/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:2636: || (documentElement() &&
!isHTMLHtmlElement(*documentElement()));
On 2014/04/09 21:32:25, esprehn wrote:
> Can we split these too so it's easier to read?
> 
> if (isRenderingReady() && body())
>   return true;
> if (documentElement() && !isHTMLHtmlElement(*documentElement()))
>   return true;
> return false

Done.

https://codereview.chromium.org/232013002/diff/1/Source/core/frame/FrameView.cpp
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/232013002/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:1858:
lifecycle().ensureStateAtMost(DocumentLifecycle::StyleClean);
On 2014/04/09 21:32:25, esprehn wrote:
> Ideally we'd just schedule an update. This should be at lifecycle of
> UpdatePending. No one should call scheduleVisualUpdate() directly and not
change
> the lifecycle.

Yeah, I tried that when I originally added the DocumentLifecycle.  I can try it
again, but probably not in this CL.

https://codereview.chromium.org/232013002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/232013002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:95: // the
compositing update, but that happens in
compositing/visibility/hidden-iframe.html.
On 2014/04/09 21:32:25, esprehn wrote:
> This is scheduleLayerUpdate() at work.

Craziness.

[abarth-chromium, 2014-04-09 21:57:27]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-04-09 21:57:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/232013002/20001

[commit-bot: I haz the power, 2014-04-09 22:34:32]

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

[commit-bot: I haz the power, 2014-04-09 22:34:32]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[abarth-chromium, 2014-04-09 22:38:16]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-04-09 22:38:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/232013002/20001

[commit-bot: I haz the power, 2014-04-09 22:41:32]

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

[commit-bot: I haz the power, 2014-04-09 22:41:33]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[ojan, 2014-04-09 22:43:26]

lgtm

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

https://codereview.chromium.org/232013002/diff/20001/Source/core/dom/Document...
Source/core/dom/DocumentLifecycle.h:47: UpdatePending,
This name is a bit too general, e.g. InCompositingUpdate also does an update.
Mabye this should be called UpdateRenderTreePending to match the method name in
Document?

[abarth-chromium, 2014-04-09 22:48:42]

On 2014/04/09 22:43:26, ojan wrote:
>
https://codereview.chromium.org/232013002/diff/20001/Source/core/dom/Document...
> File Source/core/dom/DocumentLifecycle.h (right):
> 
>
https://codereview.chromium.org/232013002/diff/20001/Source/core/dom/Document...
> Source/core/dom/DocumentLifecycle.h:47: UpdatePending,
> This name is a bit too general, e.g. InCompositingUpdate also does an update.

When we're in UpdatePending, we'll eventually go through InCompositingUpdate.

> Mabye this should be called UpdateRenderTreePending to match the method name
in
> Document?

Maybe we should call this state VisualUpdatePending?

[ojan, 2014-04-09 22:50:48]

On 2014/04/09 22:48:42, abarth wrote:
> On 2014/04/09 22:43:26, ojan wrote:
> >
>
https://codereview.chromium.org/232013002/diff/20001/Source/core/dom/Document...
> > File Source/core/dom/DocumentLifecycle.h (right):
> > 
> >
>
https://codereview.chromium.org/232013002/diff/20001/Source/core/dom/Document...
> > Source/core/dom/DocumentLifecycle.h:47: UpdatePending,
> > This name is a bit too general, e.g. InCompositingUpdate also does an
update.
> 
> When we're in UpdatePending, we'll eventually go through InCompositingUpdate.

I guess that's true. It's just a bit weird to me.

> > Mabye this should be called UpdateRenderTreePending to match the method name
> in
> > Document?
> 
> Maybe we should call this state VisualUpdatePending?

Sure. That seems better to me.

[abarth-chromium, 2014-04-10 00:22:49]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-04-10 00:22:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/232013002/40001

[commit-bot: I haz the power, 2014-04-10 01:29:21]

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

[commit-bot: I haz the power, 2014-04-10 01:29:22]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[esprehn, 2014-04-10 01:35:01]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-04-10 01:35:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/232013002/40001

[commit-bot: I haz the power, 2014-04-10 02:43:31]

Change committed as 171221