Node::lazyAttach shouldn't lie about being attached

Node::lazyAttach shouldn't lie about being attached

lazyAttach just needs to mark the node as needing a style recalc, there's no reason
to mark the node as attached, or mark all the descendants as being attached. We also
don't need to mark all descendants as needing a style recalc since Element::recalcStyle
is going to transform the StyleChange into a Force when it goes to descendants.

Now lazyAttach just marks you as needing a FullStyleChange and the later recalcStyle
will mark you as attached. This means that after this patch attached() is equivalent to
"ever had style recalc". In a future patch we should rename that flag.

A benefit of this patch is that doing appendChild(divWithLotsOfDescendants); now does 2
fewer walks of the descendants because we don't need to mark them all as attached in
lazyAttach, and we don't need to run detach() in recalcStyle() when it calls reattach.

Another benefit is that Node::attach is no longer n^2 when you append many elements
without any whitespace between them because the loop to fix up whitespace can bail
out early when it sees !next->attached().

R=eseidel@chromium.org

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

[esprehn, 2013-05-16 22:13:35]

The future is near.

[eseidel, 2013-05-16 22:48:25]

lgtm

I expect you're going to have fallout from this change which may take us weeks
to fully shake out.

That said, this is about the most amazing patch I've seen in months.  :)

https://codereview.chromium.org/15159008/diff/1001/Source/core/dom/shadow/Sha...
File Source/core/dom/shadow/ShadowRoot.cpp (right):

https://codereview.chromium.org/15159008/diff/1001/Source/core/dom/shadow/Sha...
Source/core/dom/shadow/ShadowRoot.cpp:145: if (styleChangeType() ==
FullStyleChange)
I'm confused by this line.  Can you give more context?  Presumably +dglazkov or
+aklein, etc. should review this part?

[commit-bot: I haz the power, 2013-05-16 22:48:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/15159008/1001

[commit-bot: I haz the power, 2013-05-16 23:33:17]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[dglazkov, 2013-05-17 02:58:44]

Adding morrita, who'd seen/wrote a lot of this code.

https://chromiumcodereview.appspot.com/15159008/diff/1001/LayoutTests/fast/do...
File LayoutTests/fast/dom/shadow/shadow-root-attached.html (right):

https://chromiumcodereview.appspot.com/15159008/diff/1001/LayoutTests/fast/do...
LayoutTests/fast/dom/shadow/shadow-root-attached.html:24:
document.body.offsetLeft; // cause a style recalc to trigger attachment.
Whoa, why do we need that now?

[esprehn, 2013-05-17 03:02:39]

On 2013/05/17 02:58:44, Dimitri Glazkov wrote:
> Adding morrita, who'd seen/wrote a lot of this code.
> 
>
https://chromiumcodereview.appspot.com/15159008/diff/1001/LayoutTests/fast/do...
> File LayoutTests/fast/dom/shadow/shadow-root-attached.html (right):
> 
>
https://chromiumcodereview.appspot.com/15159008/diff/1001/LayoutTests/fast/do...
> LayoutTests/fast/dom/shadow/shadow-root-attached.html:24:
> document.body.offsetLeft; // cause a style recalc to trigger attachment.
> Whoa, why do we need that now?

Previously we'd mark nodes as attached when they went through lazyAttach. This
test doesn't really test anything except that nodes inserted into Shadow DOM go
through lazyAttach (which is silly). Now that lazyAttach doesn't setAttached we
need to cause a style recalc to actually attach them.

Nothing should be marked as attached unless it really went through attach().

[esprehn, 2013-05-17 03:35:25]

All the selection/* failures are 0x0 RenderTexts that are no longer produced.
object-image-nested-fallback.html contains a race condition with plugin loading,
but object-image-error.html seems to be some strange race thing I don't
understand. When I run the test in ContentShell it prints "PASS" fine, but DRT
never does even when I trigger a style recalc manually or add setTimeouts. Needs
some more digging.

[Hajime Morrita, 2013-05-17 04:01:24]

This change is theoretically correct.
What we need is fix old pieces which depend on theoretically incorrect
behaviour. 
Lazily attached nodes is in inconsistent state and it would be great if we get
rid of that.

[eseidel, 2013-05-17 04:35:29]

I'm curious why you believe lazyAttached nodes to be in an inconsistent state? 
They're notes which need style recalc and are not currently attached?  That's
exactly the case as when a node was previously display: none, and is now being
made display: block, for instance?  Right?  Or would that node be marked
attached() yet have no renderer?

My vision for this is that Nodes should be marked as needing style resolve when
they are inserted into a tree (as they are today) and otherwise there should be
no special "attach" logic.  attach() is simply a helper method which creates the
renderer, but there is no concept of attached().  We likely will need a concept
of everCalculatedStyleInThisTree() for some callsites (to replace attached() in
some cases).

[Hajime Morrita, 2013-05-17 04:56:27]

On 2013/05/17 04:35:29, eseidel wrote:
> I'm curious why you believe lazyAttached nodes to be in an inconsistent state?

> They're notes which need style recalc and are not currently attached?  That's
> exactly the case as when a node was previously display: none, and is now being
> made display: block, for instance?  Right?  Or would that node be marked
> attached() yet have no renderer?

You're right. My intuition here was that the node should have renderer if it is
attached
and it has visible style. But yes, there is node which turns none to non-none.

> 
> My vision for this is that Nodes should be marked as needing style resolve
when
> they are inserted into a tree (as they are today) and otherwise there should
be
> no special "attach" logic.  attach() is simply a helper method which creates
the
> renderer, but there is no concept of attached().  We likely will need a
concept
> of everCalculatedStyleInThisTree() for some callsites (to replace attached()
in
> some cases).

Right. We might want to kill the "attached" flag eventually. 
Things depending on the flag should be migrated to other lifecycle events like
entering/leaving tree
or recalculating styles. I believe esprehn has his own thought here though.

[esprehn, 2013-05-17 21:08:58]

Getting really close on this, after this patch attached() means "ever had style
recalc" and we can rename in a follow up patch.

[esprehn, 2013-05-17 22:00:11]

Okay this is ready. It passes the tests on the try bots. :)

[ojan, 2013-05-18 00:28:36]

https://codereview.chromium.org/15159008/diff/41001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/15159008/diff/41001/Source/core/dom/Node.cpp#...
Source/core/dom/Node.cpp:874: if (document()->inStyleRecalc()) {
When do we call lazyAttach while in styleRecalc?

https://codereview.chromium.org/15159008/diff/41001/Source/core/dom/shadow/Sh...
File Source/core/dom/shadow/ShadowRoot.cpp (right):

https://codereview.chromium.org/15159008/diff/41001/Source/core/dom/shadow/Sh...
Source/core/dom/shadow/ShadowRoot.cpp:146: change = Force;
Still don't get this change. Probably deserves to be explained in the change
description or might deserve a comment depending on why this is needed.

[esprehn, 2013-05-18 01:06:30]

On 2013/05/18 00:28:36, ojan wrote:
> https://codereview.chromium.org/15159008/diff/41001/Source/core/dom/Node.cpp
> File Source/core/dom/Node.cpp (right):
> 
>
https://codereview.chromium.org/15159008/diff/41001/Source/core/dom/Node.cpp#...
> Source/core/dom/Node.cpp:874: if (document()->inStyleRecalc()) {
> When do we call lazyAttach while in styleRecalc?

Plugins sometimes do it, specifically HTMLObjectElement::renderFallbackContent
called reattach(), and sometimes, but not always, renderFallbackContent is
called inside recalcStyle since plugins are driven off the render tree (I know
eric wants to fix that). I changed this to lazyReattach but that was bad since
in some cases (see object-image-error.html) we'd end up doing
setNeedsRecalcStyle in renderFallbackContent by way of an attach() call which
then clears the recalc style bits preventing us from ever getting a recalc
style.

It's the same reason we can't do setNeedsLayout() inside layout since an
ancestor might clear their bits leaving you as needsLayout() but your parent
doesn't think any descendants need it.

> 
>
https://codereview.chromium.org/15159008/diff/41001/Source/core/dom/shadow/Sh...
> File Source/core/dom/shadow/ShadowRoot.cpp (right):
> 
>
https://codereview.chromium.org/15159008/diff/41001/Source/core/dom/shadow/Sh...
> Source/core/dom/shadow/ShadowRoot.cpp:146: change = Force;
> Still don't get this change. Probably deserves to be explained in the change
> description or might deserve a comment depending on why this is needed.

The change is for correctness. I don't think you can observe it right now
because we never do setNeedsStyleRecalc() on a ShadowRoot without doing it on
the host, but without this shadowRoot.lazyAttach() doesn't work because we don't
promote the change type to Force for all the descendants. This is the same logic
inside Element::recalcStyle:

        if (styleChangeType() == FullStyleChange)
            change = Force;
        else if (change != Force)
            change = localChange;

Except we don't have a localChange since ShadowRoot doesn't have it's own
RenderStyle.

I'll add some text to the patch description. I'm not sure we need a comment
since ShadowRoot::recalcStyle is just a duplicate of Element::recalcStyle (with
bugs from missing things).

[esprehn, 2013-05-18 01:27:35]

I thought about this more and decided comments were a good idea so I added them
to the places you didn't think were clear.

[eseidel, 2013-05-22 00:42:32]

lgtm again. :)  I don't want to block you.  It's just not clear to me the final
path forward, and you and I have failed to be in the same place at once for the
last 3 days.

[commit-bot: I haz the power, 2013-05-22 19:32:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/15159008/54001

[esprehn, 2013-05-22 20:59:16]

Committed patchset #10 manually as r150924 (presubmit successful).