Re: WIP CL (issue 2398293003 by nainar@chromium.org)

[nai...@chromium.org]

esprehn,

Uploading this patch for a design review, PTAL? Thanks! :)


https://codereview.chromium.org/2398293003/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (left):

https://codereview.chromium.org/2398293003/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp#oldcode1898
third_party/WebKit/Source/core/dom/Element.cpp:1898:
Moved this to rebuildLayoutTree()

https://codereview.chromium.org/2398293003/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2398293003/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp#newcode2010
third_party/WebKit/Source/core/dom/Element.cpp:2010: // This is
information passed on to recalcStyle from recalcDescendantStyles
Need to decide if this method is preferable or whether we want to store
the Text node from recalcStyle and use that here in rebuildLayoutTree

https://codereview.chromium.org/2398293003/diff/40001/third_party/WebKit/Source/core/dom/Element.h
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/2398293003/diff/40001/third_party/WebKit/Source/core/dom/Element.h#newcode403
third_party/WebKit/Source/core/dom/Element.h:403: StyleRecalcChange
rebuildLayoutTree();
Moved this here because it will need to be accessed in Document in a
future CL

https://codereview.chromium.org/2398293003/

[nai...@chromium.org]

Moving bugsnash from cc to a reviewer.

https://codereview.chromium.org/2398293003/

[esp...@chromium.org]

https://codereview.chromium.org/2398293003/diff/100001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2398293003/diff/100001/third_party/WebKit/Source/core/dom/Element.cpp#newcode2001
third_party/WebKit/Source/core/dom/Element.cpp:2001: for (Node* child =
firstChild(); child && child->isElementNode();
this breaks at the first non element, ex. text or comment.

you need:

if (node->isElementNode())

inside the loop.

Also you don't handle Shadow DOM here, you need to iterate the
ShadowRoots first, see how ::recalcStyle does this:

for (ShadowRoot* root = youngestShadowRoot(); root;
root = root->olderShadowRoot()) {

Actually I think this entire side of the branch is unreachable because
you only call this inside recalcOwnStyle right now right after calling
setNeedsReattachLayoutTree() we always take the
needsReattachLayoutTree().

Maybe we remove this side of the if statement and make it NOTREACHED()
for this patch. The next patch that move rebuildLayoutTree to be it's
own step in updateStyle() will fill it in.

https://codereview.chromium.org/2398293003/diff/100001/third_party/WebKit/Source/core/dom/Element.cpp#newcode2015
third_party/WebKit/Source/core/dom/Element.cpp:2015: for (Node* child =
parentNode()->lastChild(); child;
This is n^2, if you appendChild 500 elements, this always iterates from
the end to yourself.

I think we want to just start at |this| and iterate until we find the
next whitespace sibling.

https://codereview.chromium.org/2398293003/

[nai...@chromium.org]

Elliott,

PTAL?

Thanks!

https://codereview.chromium.org/2398293003/diff/100001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2398293003/diff/100001/third_party/WebKit/Source/core/dom/Element.cpp#newcode2001
third_party/WebKit/Source/core/dom/Element.cpp:2001: for (Node* child =
firstChild(); child && child->isElementNode();

Added NOTREACHED() here and noted the changes needed for a future patch.

https://codereview.chromium.org/2398293003/diff/100001/third_party/WebKit/Source/core/dom/Element.cpp#newcode2015
third_party/WebKit/Source/core/dom/Element.cpp:2015: for (Node* child =
parentNode()->lastChild(); child;

Done. Starting iteration from this->nextSibling.

Can change this to checking in the if condition that the node in
consideration is not this itself.

https://codereview.chromium.org/2398293003/

[bugs...@chromium.org]

https://codereview.chromium.org/2398293003/diff/140001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2398293003/diff/140001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1998
third_party/WebKit/Source/core/dom/Element.cpp:1998: else if
(childNeedsReattachLayoutTree())
I think this would be better as a DCHECK

https://codereview.chromium.org/2398293003/diff/140001/third_party/WebKit/Source/core/dom/Element.cpp#newcode2004
third_party/WebKit/Source/core/dom/Element.cpp:2004: // (like WebKit
does).
I wouldn't include what webkit does in a comment - it might change

https://codereview.chromium.org/2398293003/diff/140001/third_party/WebKit/Source/core/dom/Element.cpp#newcode2009
third_party/WebKit/Source/core/dom/Element.cpp:2009: if
(sibling->isElementNode() && toElement(sibling)->layoutObject()) {
could avoid this branch by adding it to the for loop conditional

https://codereview.chromium.org/2398293003/

[nai...@chromium.org]

@bugsnash,

Made the changes you asked for. PTAL? Thanks!

https://codereview.chromium.org/2398293003/diff/140001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2398293003/diff/140001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1998
third_party/WebKit/Source/core/dom/Element.cpp:1998: else if
(childNeedsReattachLayoutTree())

Done.

https://codereview.chromium.org/2398293003/diff/140001/third_party/WebKit/Source/core/dom/Element.cpp#newcode2004
third_party/WebKit/Source/core/dom/Element.cpp:2004: // (like WebKit
does).

Done.

https://codereview.chromium.org/2398293003/diff/140001/third_party/WebKit/Source/core/dom/Element.cpp#newcode2009
third_party/WebKit/Source/core/dom/Element.cpp:2009: if
(sibling->isElementNode() && toElement(sibling)->layoutObject()) {

Done.

https://codereview.chromium.org/2398293003/

[bugs...@chromium.org]

https://codereview.chromium.org/2398293003/diff/160001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2398293003/diff/160001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1996
third_party/WebKit/Source/core/dom/Element.cpp:1996: if
(needsReattachLayoutTree())
what is the purpose of this check? it doesn't seem necessary at this
point

https://codereview.chromium.org/2398293003/diff/160001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1999
third_party/WebKit/Source/core/dom/Element.cpp:1999:
DCHECK(!childNeedsReattachLayoutTree());
add a comment explaining why we expect this DCHECK to be true (i.e.
reattachLayoutTree handles the children)

https://codereview.chromium.org/2398293003/diff/160001/third_party/WebKit/Source/core/dom/Element.cpp#newcode2002
third_party/WebKit/Source/core/dom/Element.cpp:2002: Text*
nextTextSibling = nullptr;
the comment about the trade off between storage and calculation was
good, I just meant to remove the part about webkit.

I think this would be better separated into its own method
nextTextSibling(), possibly on Node, which can return null if the next
sibling is not an Element or doesn't have a layout object. In future
once Text mirrors Element this could even be called from within
reattachWhitespaceSiblings itself instead of being passed here.

https://codereview.chromium.org/2398293003/

[nai...@chromium.org]

@bugsnash,

Made the changes requested. PTAL? Thanks!

https://codereview.chromium.org/2398293003/diff/160001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2398293003/diff/160001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1996
third_party/WebKit/Source/core/dom/Element.cpp:1996: if
(needsReattachLayoutTree())

Moved to a DCHECK and added an explainer comment here as well.

https://codereview.chromium.org/2398293003/diff/160001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1999
third_party/WebKit/Source/core/dom/Element.cpp:1999:
DCHECK(!childNeedsReattachLayoutTree());

Done.

https://codereview.chromium.org/2398293003/diff/160001/third_party/WebKit/Source/core/dom/Element.cpp#newcode2002
third_party/WebKit/Source/core/dom/Element.cpp:2002: Text*
nextTextSibling = nullptr;

Edited the comment.

Moved code to function.

https://codereview.chromium.org/2398293003/

[esp...@chromium.org]

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1893
third_party/WebKit/Source/core/dom/Element.cpp:1893:
clearChildNeedsReattachLayoutTree();
Note that once you start walking the tree you're not going to want to
clear the clearChildNeedsReattachLayoutTree bit here. These bits should
only be touched inside reattachLayoutTree.

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1990
third_party/WebKit/Source/core/dom/Element.cpp:1990: AttachContext
reattachContext;
DCHECK(inActiveDocument()) in this function

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/Element.cpp#newcode2002
third_party/WebKit/Source/core/dom/Element.cpp:2002:
DCHECK(!childNeedsReattachLayoutTree());
DCHECK(parentNode());
DCHECK(parentNode()->childNeedsReattachLayoutTree());

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/Node.cpp
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/Node.cpp#newcode396
third_party/WebKit/Source/core/dom/Node.cpp:396:
!(sibling->isElementNode() && toElement(sibling)->layoutObject());
Run demorgans :)

(!sibling->isElementNode() || !toElement(sibling)->layoutObject())

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp
File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right):

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp#newcode136
third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:136:
clearChildNeedsReattachLayoutTree();
ditto, you won't want to touch the reattach bits in recalcStyle once you
start tree walking inside reattachLayoutTree.

https://codereview.chromium.org/2398293003/

[nai...@chromium.org]

@esprehn,

Made the changes you asked for, PTAL? Thanks!

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1893
third_party/WebKit/Source/core/dom/Element.cpp:1893:
clearChildNeedsReattachLayoutTree();

Done.

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1990
third_party/WebKit/Source/core/dom/Element.cpp:1990: AttachContext
reattachContext;

Done.

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/Element.cpp#newcode2002
third_party/WebKit/Source/core/dom/Element.cpp:2002:
DCHECK(!childNeedsReattachLayoutTree());

Done.

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/Node.cpp
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/Node.cpp#newcode396
third_party/WebKit/Source/core/dom/Node.cpp:396:
!(sibling->isElementNode() && toElement(sibling)->layoutObject());

Done.

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp
File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right):

https://codereview.chromium.org/2398293003/diff/180001/third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp#newcode136
third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:136:
clearChildNeedsReattachLayoutTree();

Done.

https://codereview.chromium.org/2398293003/

[esp...@chromium.org]

This introduced an n^2 looking for the next text sibling which makes me a little
nervous but I say let's go for it and figure out it later. Speaking of there's
n^2 protection in the form of needsAttach() getter that's used during layout
tree construction. We need to figure out how to make that work in the next patch
by looking at the bits you added here.


https://codereview.chromium.org/2398293003/diff/200001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (left):

https://codereview.chromium.org/2398293003/diff/200001/third_party/WebKit/Source/core/dom/Element.cpp#oldcode1898
third_party/WebKit/Source/core/dom/Element.cpp:1898:
reattachWhitespaceSiblingsIfNeeded(nextTextSibling);
We don't need the nextTextSibling here anymore so we could remove the
argument in a follow up patch?

https://codereview.chromium.org/2398293003/

[esp...@chromium.org]

lgtm



https://codereview.chromium.org/2398293003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2398293003/240001


https://chromiumcodereview.appspot.com/2398293003/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #12 (id:240001)

https://chromiumcodereview.appspot.com/2398293003/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

Patchset 12 (id:??) landed as
https://crrev.com/09b7a8cf0647abca732b3718987a629d12a67fee
Cr-Commit-Position: refs/heads/master@{#426353}

https://codereview.chromium.org/2398293003/

[nai...@chromium.org]

Going to revert this patch as it is suspected to cause crbug.com/658322.

Since I am OOO - I can't actually investigate till I get into Sydney. So going
to revert to be safe.

https://codereview.chromium.org/2398293003/

[nai...@chromium.org]

A revert of this CL (patchset #12 id:240001) has been created in
https://codereview.chromium.org/2439973005/ by nai...@chromium.org.

The reason for reverting is: Suspected to cause crbug.com/658322..

https://codereview.chromium.org/2398293003/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2398293003/240001

https://codereview.chromium.org/2398293003/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

Try jobs failed on following builders:
chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-generic_chromium_compile_only_ng/builds/223580)

https://codereview.chromium.org/2398293003/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

Committed patchset #12 (id:240001)

https://codereview.chromium.org/2398293003/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

Patchset 12 (id:??) landed as

https://crrev.com/651c13992922ce5c2b0dd0072c5c3e4a59a03b87
Cr-Commit-Position: refs/heads/master@{#427612}

https://codereview.chromium.org/2398293003/