|
|
Created:
4 years, 1 month ago by nainar Modified:
3 years, 9 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, meade_UTC10, Mike Lawther (Google), rwlbuis, shans, sof, webcomponents-bugzilla_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCall Element::rebuildLayoutTree from Document::updateStyle directly
This patch changes the callstack in Document::updateStyle to call
Element::rebuildLayoutTree after Element::recalcStyle instead of
from it.
It also adds helper functions needed to perform Layout Tree Construction:
1. ContainerNode::rebuildChildrenLayoutTrees()
2. Element::rebuildShadowRootLayoutTree()
3. Element::reattachPseudoElementLayoutTree(PseudoId pseudoId)
4. ShadowRoot::rebuildLayoutTree()
It also changes the setting/getting of the (Child)NeedsStyleRecalc
and (Child)NeedsReattachLayoutTree bits to match this pattern.
The patch also adds a LayoutTest to test for the crash caught
in crbug.com/695950.
BUG=595137
Review-Url: https://codereview.chromium.org/2473743003
Cr-Original-Commit-Position: refs/heads/master@{#451987}
Committed: https://chromium.googlesource.com/chromium/src/+/8b6c7b7ce5a36d8eda49dd6e60c5e7c8f5c7277f
Review-Url: https://codereview.chromium.org/2473743003
Cr-Commit-Position: refs/heads/master@{#454824}
Committed: https://chromium.googlesource.com/chromium/src/+/f4c976fc1f4578b0c75ba304553ef2ff6134f551
Patch Set 1 #
Total comments: 3
Patch Set 2 : Done 1 #Patch Set 3 : Push to Reitveld #Patch Set 4 : For comments #Patch Set 5 : Commit the renames #
Total comments: 19
Patch Set 6 : Stage 1 #Patch Set 7 : FirstLetterPseudoElements are not being created #
Total comments: 15
Patch Set 8 : Addressing bugsnash's comments #Patch Set 9 : Addressing bugsnash's comments #Patch Set 10 : For review #
Total comments: 1
Patch Set 11 : Address esprehn's comments #
Total comments: 12
Patch Set 12 : Comments #Patch Set 13 : Handover #
Total comments: 1
Patch Set 14 : Import bugsnash@ code and add comment #Patch Set 15 : Everything works so far - failing test still #Patch Set 16 : Everything works so far - failing test still #Patch Set 17 : Split up didRecalcStyle into didRecalcStyle and didRebuildLayoutTree #
Total comments: 1
Patch Set 18 : Final patch - make sure to retain information needed to determine whether to call detachLayoutTree() #
Total comments: 6
Patch Set 19 : Remove redundant clear for root's NeedReattachLayoutTree flag #
Total comments: 1
Patch Set 20 : Clean up test failures #Patch Set 21 : setNeedsReattachLayoutTree flag in ElementShadow::addShadowRoot #Patch Set 22 : Fix failures due to information not being retained correctly for detachLayoutTree() in Text::reatta… #Patch Set 23 : Fix failures due to information not being retained correctly for detachLayoutTree() in Text::reatta… #
Total comments: 14
Patch Set 24 : Fix unconditional ShadowRoot::rebuildLayoutTree() and retain needsStyleRecalc information for cases… #Patch Set 25 : Fix unconditional ShadowRoot::rebuildLayoutTree() and retain needsStyleRecalc information for cases… #
Total comments: 2
Patch Set 26 : Mirror setChildNeedsStyleRecalc and markAncestorsWithNeedsReattachLayoutTree with setChildNeedsReat… #
Total comments: 22
Patch Set 27 : Clean up all the asserts #Patch Set 28 : Clean up all the asserts #Patch Set 29 : Clean up all the asserts #
Total comments: 5
Patch Set 30 : Clean up all the asserts #Patch Set 31 : "StyleSharing should use info from map if available. Early store information on the HashMap if we h… #
Total comments: 4
Patch Set 32 : Fixing edge cases in code #
Total comments: 10
Patch Set 33 : Move reattachLayoutTree related code into the relevant block #Patch Set 34 : Make needsAttach() only check if getStyleChangeType flag is NeedsReattachStyleChange and add a Layo… #
Total comments: 1
Patch Set 35 : Make needsAttach() only check if getStyleChangeType flag is NeedsReattachStyleChange and add a Layo… #Patch Set 36 : Make needsAttach() only check if getStyleChangeType flag is NeedsReattachStyleChange and add a Layo… #Messages
Total messages: 186 (103 generated)
nainar@chromium.org changed reviewers: + bugsnash@chromium.org, esprehn@chromium.org
Hi! Need some help figuring an issue out. Given this test case: <li> <p>Text</p> </li> The expected result is: <bullet point> Text However with my patch you get: <bullet point> Text My understanding is that the <p> element should have been laid out inline. (Gets a LayoutInline object) but is instead getting laid out as a block (Gets a LayoutBlock object.) I have been digging and can't understand why the change in behaviour. Any ideas? Also this patch currently crashes on most of the tests (All that use either the testharness or the js-test framework.) https://codereview.chromium.org/2473743003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Text.cpp (left): https://codereview.chromium.org/2473743003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Text.cpp:404: void Text::rebuildTextLayoutTree(Text* nextTextSibling) { If bugs finishes his patch before this needs to land - I will rebase to not call nextTextSibling() here and instead use the value from the HashMap
On 2016/11/03 00:17:10, nainar wrote: > Hi! > > Need some help figuring an issue out. > > Given this test case: > > <li> > <p>Text</p> > </li> > > The expected result is: > <bullet point> Text > > However with my patch you get: > <bullet point> > Text > > My understanding is that the <p> element should have been laid out inline. (Gets > a LayoutInline object) but is instead getting laid out as a block (Gets a > LayoutBlock object.) I have been digging and can't understand why the change in > behaviour. Any ideas? It could be that you have whitespace reattachment changes which triggers a variant of crbug.com/364817 One out of a few whitespace reattachment bugs.
On 2016/11/03 at 10:31:28, rune wrote: > On 2016/11/03 00:17:10, nainar wrote: > > Hi! > > > > Need some help figuring an issue out. > > > > Given this test case: > > > > <li> > > <p>Text</p> > > </li> > > > > The expected result is: > > <bullet point> Text > > > > However with my patch you get: > > <bullet point> > > Text > > > > My understanding is that the <p> element should have been laid out inline. (Gets > > a LayoutInline object) but is instead getting laid out as a block (Gets a > > LayoutBlock object.) I have been digging and can't understand why the change in > > behaviour. Any ideas? > > It could be that you have whitespace reattachment changes which triggers a variant of crbug.com/364817 One out of a few whitespace reattachment bugs. Thanks for sending the link over rune. I am investigating this. I made an incorrect assumption though. The failing test I have linked above isn't because of this patch. But is already happening in Canary. I have file crbug.com/662762. It was most definitely caused by my previous patch: crrev.com/2476163002. However, that patch doesn't change anything other than calling Node::reattachWhitespaceSiblingsIfNeeded from Element::rebuildLayoutTree instead of Element::recalcStyle. The order of code execution hasn't changed at all. Investigating.
https://codereview.chromium.org/2473743003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2473743003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:2014: return ReattachNoLayoutObject; Is ReattachNoLayoutObject dead code now? https://codereview.chromium.org/2473743003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:1998: } else if (childNeedsReattachLayoutTree()) { You're missing: SelectorFilterParentScope filterScope(*this); StyleSharingDepthScope sharingScope(*this); which is why you're failing asserts. Also the PseudoIdBefore. :)
nainar@chromium.org changed reviewers: + rune@opera.com
Hi rune@, With regards to this patch, the meat of the code seems to be correct. The code in Element::rebuildLayoutTree() mirrors the code in Element::recalcStyle(). However, I am currently having trouble with failing asserts due to the (child)NeedsStyleRecalc and (child)NeedsReattachLayoutTree bits being set incorrectly. I have tried my best to go through the places that the bits are set and make sure that we are setting (child)NeedsStyleRecalc correctly in recalc style code and clearing them as needed. Could you please take a look at this WIP patch and let me know where I am wrong? Any help would be greatly appreciated. Thank you! Also both bugsnash@ and esprehn@ are on leave.
On 2016/11/23 12:15:09, nainar wrote: > Hi rune@, > > With regards to this patch, the meat of the code seems to be correct. The code > in Element::rebuildLayoutTree() mirrors the code in Element::recalcStyle(). > > However, I am currently having trouble with failing asserts due to the > (child)NeedsStyleRecalc and (child)NeedsReattachLayoutTree bits being set > incorrectly. I have tried my best to go through the places that the bits are set > and make sure that we are setting (child)NeedsStyleRecalc correctly in recalc > style code and clearing them as needed. Could you please take a look at this WIP > patch and let me know where I am wrong? Any help would be greatly appreciated. > > Thank you! Do you have a simple case which triggers these assert?
On 2016/11/23 at 12:44:34, rune wrote: > On 2016/11/23 12:15:09, nainar wrote: > > Hi rune@, > > > > With regards to this patch, the meat of the code seems to be correct. The code > > in Element::rebuildLayoutTree() mirrors the code in Element::recalcStyle(). > > > > However, I am currently having trouble with failing asserts due to the > > (child)NeedsStyleRecalc and (child)NeedsReattachLayoutTree bits being set > > incorrectly. I have tried my best to go through the places that the bits are set > > and make sure that we are setting (child)NeedsStyleRecalc correctly in recalc > > style code and clearing them as needed. Could you please take a look at this WIP > > patch and let me know where I am wrong? Any help would be greatly appreciated. > > > > Thank you! > > Do you have a simple case which triggers these assert? One case out of many that triggers it is this: <style> .test:first-letter { color: green;} </style> <div class="test">Block to Inline</div> A minimized test case taken from fast/css/first-letter-removed-added.html I think we need to set the needsReattachLayoutTree bit on the PseudoElement here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme... before calling attachLayoutTree on it?
On 2016/11/23 13:19:12, nainar wrote: > On 2016/11/23 at 12:44:34, rune wrote: > > On 2016/11/23 12:15:09, nainar wrote: > > > Hi rune@, > > > > > > With regards to this patch, the meat of the code seems to be correct. The > code > > > in Element::rebuildLayoutTree() mirrors the code in Element::recalcStyle(). > > > > > > However, I am currently having trouble with failing asserts due to the > > > (child)NeedsStyleRecalc and (child)NeedsReattachLayoutTree bits being set > > > incorrectly. I have tried my best to go through the places that the bits are > set > > > and make sure that we are setting (child)NeedsStyleRecalc correctly in > recalc > > > style code and clearing them as needed. Could you please take a look at this > WIP > > > patch and let me know where I am wrong? Any help would be greatly > appreciated. > > > > > > Thank you! > > > > Do you have a simple case which triggers these assert? > > One case out of many that triggers it is this: > > <style> > .test:first-letter { color: green;} > </style> > <div class="test">Block to Inline</div> > > A minimized test case taken from fast/css/first-letter-removed-added.html > > I think we need to set the needsReattachLayoutTree bit on the PseudoElement > here: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme... > before calling attachLayoutTree on it? I didn't have the time to get far on this yesterday, looking now. Apart from the clean tree assertions there were a lot of failing tests without DCHECKs triggering. I'll try to debug some simple none->block change now to see what's happening.
On 2016/11/24 at 08:30:51, rune wrote: > On 2016/11/23 13:19:12, nainar wrote: > > On 2016/11/23 at 12:44:34, rune wrote: > > > On 2016/11/23 12:15:09, nainar wrote: > > > > Hi rune@, > > > > > > > > With regards to this patch, the meat of the code seems to be correct. The > > code > > > > in Element::rebuildLayoutTree() mirrors the code in Element::recalcStyle(). > > > > > > > > However, I am currently having trouble with failing asserts due to the > > > > (child)NeedsStyleRecalc and (child)NeedsReattachLayoutTree bits being set > > > > incorrectly. I have tried my best to go through the places that the bits are > > set > > > > and make sure that we are setting (child)NeedsStyleRecalc correctly in > > recalc > > > > style code and clearing them as needed. Could you please take a look at this > > WIP > > > > patch and let me know where I am wrong? Any help would be greatly > > appreciated. > > > > > > > > Thank you! > > > > > > Do you have a simple case which triggers these assert? > > > > One case out of many that triggers it is this: > > > > <style> > > .test:first-letter { color: green;} > > </style> > > <div class="test">Block to Inline</div> > > > > A minimized test case taken from fast/css/first-letter-removed-added.html > > > > I think we need to set the needsReattachLayoutTree bit on the PseudoElement > > here: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme... > > before calling attachLayoutTree on it? > > I didn't have the time to get far on this yesterday, looking now. > > Apart from the clean tree assertions there were a lot of failing tests without DCHECKs triggering. I'll try to debug some simple none->block change now to see what's happening. Hi, I took a stab at the theory I had above. And that seems to work. I have set the needsReattachLayoutTree in Element::createPseudoElementIfNeeded before calling PseudoElement::attachLayoutTree(). After fixing some up other asserts, (uploaded the new patch here as well) I have a new problem. If you take fast/css/first-letter-removed-added.html for example. While everything is laid out correctly, the testharness results aren't visible. None of the LayoutTests results are showing up. All the tests pass, It's just the testHarness results aren't visible.
On 2016/11/24 08:59:11, nainar wrote: > On 2016/11/24 at 08:30:51, rune wrote: > > On 2016/11/23 13:19:12, nainar wrote: > > > On 2016/11/23 at 12:44:34, rune wrote: > > > > On 2016/11/23 12:15:09, nainar wrote: > > > > > Hi rune@, > > > > > > > > > > With regards to this patch, the meat of the code seems to be correct. > The > > > code > > > > > in Element::rebuildLayoutTree() mirrors the code in > Element::recalcStyle(). > > > > > > > > > > However, I am currently having trouble with failing asserts due to the > > > > > (child)NeedsStyleRecalc and (child)NeedsReattachLayoutTree bits being > set > > > > > incorrectly. I have tried my best to go through the places that the bits > are > > > set > > > > > and make sure that we are setting (child)NeedsStyleRecalc correctly in > > > recalc > > > > > style code and clearing them as needed. Could you please take a look at > this > > > WIP > > > > > patch and let me know where I am wrong? Any help would be greatly > > > appreciated. > > > > > > > > > > Thank you! > > > > > > > > Do you have a simple case which triggers these assert? > > > > > > One case out of many that triggers it is this: > > > > > > <style> > > > .test:first-letter { color: green;} > > > </style> > > > <div class="test">Block to Inline</div> > > > > > > A minimized test case taken from fast/css/first-letter-removed-added.html > > > > > > I think we need to set the needsReattachLayoutTree bit on the PseudoElement > > > here: > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme... > > > before calling attachLayoutTree on it? > > > > I didn't have the time to get far on this yesterday, looking now. > > > > Apart from the clean tree assertions there were a lot of failing tests without > DCHECKs triggering. I'll try to debug some simple none->block change now to see > what's happening. > > Hi, > > I took a stab at the theory I had above. And that seems to work. I have set the > needsReattachLayoutTree in Element::createPseudoElementIfNeeded before calling > PseudoElement::attachLayoutTree(). After fixing some up other asserts, (uploaded > the new patch here as well) I have a new problem. > > If you take fast/css/first-letter-removed-added.html for example. While > everything is laid out correctly, the testharness results aren't visible. > None of the LayoutTests results are showing up. All the tests pass, It's just > the testHarness results aren't visible.
On 2016/11/24 08:59:11, nainar wrote: > If you take fast/css/first-letter-removed-added.html for example. While > everything is laid out correctly, the testharness results aren't visible. > None of the LayoutTests results are showing up. All the tests pass, It's just > the testHarness results aren't visible. Based on a failing test when I applied the patch, I don't get this to render anything in content_shell: <!DOCTYPE html> <style> .display-when-first-child { display: none; } .display-when-first-child:first-child { display: block; } </style> <div id="test"></div> <div class="display-when-first-child">PASS</div> <script> document.body.removeChild(test); </script> The removeChild triggers a style recalc to happen before a layout tree is attached the first time. We walk the tree on style recalc but the computed style parent is always null as we don't have any layoutObject parents. We end up not generating any ComputedStyle objects at all, and the layout tree attachment is just skipped afterwards.
On 2016/11/24 09:07:22, rune wrote: > On 2016/11/24 08:59:11, nainar wrote: > > > If you take fast/css/first-letter-removed-added.html for example. While > > everything is laid out correctly, the testharness results aren't visible. > > None of the LayoutTests results are showing up. All the tests pass, It's just > > the testHarness results aren't visible. > > Based on a failing test when I applied the patch, I don't get this to render > anything in content_shell: > > <!DOCTYPE html> > <style> > .display-when-first-child { display: none; } > .display-when-first-child:first-child { display: block; } > </style> > <div id="test"></div> > <div class="display-when-first-child">PASS</div> > <script> > document.body.removeChild(test); > </script> > > The removeChild triggers a style recalc to happen before a layout tree is > attached the first time. We walk the tree on style recalc but the computed style > parent is always null as we don't have any layoutObject parents. We end up not > generating any ComputedStyle objects at all, and the layout tree attachment is > just skipped afterwards. I'm not able to generate layout trees at all here ...
On 2016/11/24 at 09:22:38, rune wrote: > On 2016/11/24 09:07:22, rune wrote: > > On 2016/11/24 08:59:11, nainar wrote: > > > > > If you take fast/css/first-letter-removed-added.html for example. While > > > everything is laid out correctly, the testharness results aren't visible. > > > None of the LayoutTests results are showing up. All the tests pass, It's just > > > the testHarness results aren't visible. > > > > Based on a failing test when I applied the patch, I don't get this to render > > anything in content_shell: > > > > <!DOCTYPE html> > > <style> > > .display-when-first-child { display: none; } > > .display-when-first-child:first-child { display: block; } > > </style> > > <div id="test"></div> > > <div class="display-when-first-child">PASS</div> > > <script> > > document.body.removeChild(test); > > </script> > > > > The removeChild triggers a style recalc to happen before a layout tree is > > attached the first time. We walk the tree on style recalc but the computed style > > parent is always null as we don't have any layoutObject parents. We end up not > > generating any ComputedStyle objects at all, and the layout tree attachment is > > just skipped afterwards. > > I'm not able to generate layout trees at all here ... I am assuming you mean with patch 4 in this comment. The output I get for fast/css/first-letter-removed-added.html is as follows: https://drive.google.com/a/chromium.org/file/d/0ByKKRKBF20RYdGhHcGhCUVlFcWs/v.... For the test case you have given above, running the code in Patch 4 - I get PASS.
On 2016/11/24 10:37:46, nainar wrote: > On 2016/11/24 at 09:22:38, rune wrote: > > On 2016/11/24 09:07:22, rune wrote: > > > On 2016/11/24 08:59:11, nainar wrote: > > > > > > > If you take fast/css/first-letter-removed-added.html for example. While > > > > everything is laid out correctly, the testharness results aren't visible. > > > > None of the LayoutTests results are showing up. All the tests pass, It's > just > > > > the testHarness results aren't visible. > > > > > > Based on a failing test when I applied the patch, I don't get this to render > > > anything in content_shell: > > > > > > <!DOCTYPE html> > > > <style> > > > .display-when-first-child { display: none; } > > > .display-when-first-child:first-child { display: block; } > > > </style> > > > <div id="test"></div> > > > <div class="display-when-first-child">PASS</div> > > > <script> > > > document.body.removeChild(test); > > > </script> > > > > > > The removeChild triggers a style recalc to happen before a layout tree is > > > attached the first time. We walk the tree on style recalc but the computed > style > > > parent is always null as we don't have any layoutObject parents. We end up > not > > > generating any ComputedStyle objects at all, and the layout tree attachment > is > > > just skipped afterwards. > > > > I'm not able to generate layout trees at all here ... > > I am assuming you mean with patch 4 in this comment. The output I get for > fast/css/first-letter-removed-added.html is as follows: > https://drive.google.com/a/chromium.org/file/d/0ByKKRKBF20RYdGhHcGhCUVlFcWs/v.... > > > For the test case you have given above, running the code in Patch 4 - I get > PASS. Yes. PS4.
On 2016/11/24 10:40:38, rune wrote: > On 2016/11/24 10:37:46, nainar wrote: > > On 2016/11/24 at 09:22:38, rune wrote: > > > On 2016/11/24 09:07:22, rune wrote: > > > > On 2016/11/24 08:59:11, nainar wrote: > > > > > > > > > If you take fast/css/first-letter-removed-added.html for example. While > > > > > everything is laid out correctly, the testharness results aren't > visible. > > > > > None of the LayoutTests results are showing up. All the tests pass, It's > > just > > > > > the testHarness results aren't visible. > > > > > > > > Based on a failing test when I applied the patch, I don't get this to > render > > > > anything in content_shell: > > > > > > > > <!DOCTYPE html> > > > > <style> > > > > .display-when-first-child { display: none; } > > > > .display-when-first-child:first-child { display: block; } > > > > </style> > > > > <div id="test"></div> > > > > <div class="display-when-first-child">PASS</div> > > > > <script> > > > > document.body.removeChild(test); > > > > </script> > > > > > > > > The removeChild triggers a style recalc to happen before a layout tree is > > > > attached the first time. We walk the tree on style recalc but the computed > > style > > > > parent is always null as we don't have any layoutObject parents. We end up > > not > > > > generating any ComputedStyle objects at all, and the layout tree > attachment > > is > > > > just skipped afterwards. > > > > > > I'm not able to generate layout trees at all here ... > > > > I am assuming you mean with patch 4 in this comment. The output I get for > > fast/css/first-letter-removed-added.html is as follows: > > > https://drive.google.com/a/chromium.org/file/d/0ByKKRKBF20RYdGhHcGhCUVlFcWs/v.... > > > > > > For the test case you have given above, running the code in Patch 4 - I get > > PASS. > > Yes. PS4. Wait a sec, I actually applied this on top of all my patches for crbug.com/567021 which might explain the difference. Even though my changes passes all tests, updating active style happens differently.
On 2016/11/25 at 08:01:50, rune wrote: > On 2016/11/24 10:40:38, rune wrote: > > On 2016/11/24 10:37:46, nainar wrote: > > > On 2016/11/24 at 09:22:38, rune wrote: > > > > On 2016/11/24 09:07:22, rune wrote: > > > > > On 2016/11/24 08:59:11, nainar wrote: > > > > > > > > > > > If you take fast/css/first-letter-removed-added.html for example. While > > > > > > everything is laid out correctly, the testharness results aren't > > visible. > > > > > > None of the LayoutTests results are showing up. All the tests pass, It's > > > just > > > > > > the testHarness results aren't visible. > > > > > > > > > > Based on a failing test when I applied the patch, I don't get this to > > render > > > > > anything in content_shell: > > > > > > > > > > <!DOCTYPE html> > > > > > <style> > > > > > .display-when-first-child { display: none; } > > > > > .display-when-first-child:first-child { display: block; } > > > > > </style> > > > > > <div id="test"></div> > > > > > <div class="display-when-first-child">PASS</div> > > > > > <script> > > > > > document.body.removeChild(test); > > > > > </script> > > > > > > > > > > The removeChild triggers a style recalc to happen before a layout tree is > > > > > attached the first time. We walk the tree on style recalc but the computed > > > style > > > > > parent is always null as we don't have any layoutObject parents. We end up > > > not > > > > > generating any ComputedStyle objects at all, and the layout tree > > attachment > > > is > > > > > just skipped afterwards. > > > > > > > > I'm not able to generate layout trees at all here ... > > > > > > I am assuming you mean with patch 4 in this comment. The output I get for > > > fast/css/first-letter-removed-added.html is as follows: > > > > > https://drive.google.com/a/chromium.org/file/d/0ByKKRKBF20RYdGhHcGhCUVlFcWs/v.... > > > > > > > > > For the test case you have given above, running the code in Patch 4 - I get > > > PASS. > > > > Yes. PS4. > > Wait a sec, I actually applied this on top of all my patches for crbug.com/567021 which might explain the difference. Even though my changes passes all tests, updating active style happens differently. I'm sorry I am a bit lost are you saying that the patch set here when added over your patches (are these local patches? or submitted ones on ToT) works correctly and layout tests pass?
On 2016/11/25 08:46:36, nainar wrote: > On 2016/11/25 at 08:01:50, rune wrote: > > On 2016/11/24 10:40:38, rune wrote: > > > On 2016/11/24 10:37:46, nainar wrote: > > > > On 2016/11/24 at 09:22:38, rune wrote: > > > > > On 2016/11/24 09:07:22, rune wrote: > > > > > > On 2016/11/24 08:59:11, nainar wrote: > > > > > > > > > > > > > If you take fast/css/first-letter-removed-added.html for example. > While > > > > > > > everything is laid out correctly, the testharness results aren't > > > visible. > > > > > > > None of the LayoutTests results are showing up. All the tests pass, > It's > > > > just > > > > > > > the testHarness results aren't visible. > > > > > > > > > > > > Based on a failing test when I applied the patch, I don't get this to > > > render > > > > > > anything in content_shell: > > > > > > > > > > > > <!DOCTYPE html> > > > > > > <style> > > > > > > .display-when-first-child { display: none; } > > > > > > .display-when-first-child:first-child { display: block; } > > > > > > </style> > > > > > > <div id="test"></div> > > > > > > <div class="display-when-first-child">PASS</div> > > > > > > <script> > > > > > > document.body.removeChild(test); > > > > > > </script> > > > > > > > > > > > > The removeChild triggers a style recalc to happen before a layout tree > is > > > > > > attached the first time. We walk the tree on style recalc but the > computed > > > > style > > > > > > parent is always null as we don't have any layoutObject parents. We > end up > > > > not > > > > > > generating any ComputedStyle objects at all, and the layout tree > > > attachment > > > > is > > > > > > just skipped afterwards. > > > > > > > > > > I'm not able to generate layout trees at all here ... > > > > > > > > I am assuming you mean with patch 4 in this comment. The output I get for > > > > fast/css/first-letter-removed-added.html is as follows: > > > > > > > > https://drive.google.com/a/chromium.org/file/d/0ByKKRKBF20RYdGhHcGhCUVlFcWs/v.... > > > > > > > > > > > > For the test case you have given above, running the code in Patch 4 - I > get > > > > PASS. > > > > > > Yes. PS4. > > > > Wait a sec, I actually applied this on top of all my patches for > crbug.com/567021 which might explain the difference. Even though my changes > passes all tests, updating active style happens differently. > > I'm sorry I am a bit lost are you saying that the patch set here when added over > your patches (are these local patches? or submitted ones on ToT) works correctly > and layout tests pass? Applied the last patch here: https://codereview.chromium.org/1913833002/
On 2016/11/25 at 08:51:38, rune wrote: > On 2016/11/25 08:46:36, nainar wrote: > > On 2016/11/25 at 08:01:50, rune wrote: > > > On 2016/11/24 10:40:38, rune wrote: > > > > On 2016/11/24 10:37:46, nainar wrote: > > > > > On 2016/11/24 at 09:22:38, rune wrote: > > > > > > On 2016/11/24 09:07:22, rune wrote: > > > > > > > On 2016/11/24 08:59:11, nainar wrote: > > > > > > > > > > > > > > > If you take fast/css/first-letter-removed-added.html for example. > > While > > > > > > > > everything is laid out correctly, the testharness results aren't > > > > visible. > > > > > > > > None of the LayoutTests results are showing up. All the tests pass, > > It's > > > > > just > > > > > > > > the testHarness results aren't visible. > > > > > > > > > > > > > > Based on a failing test when I applied the patch, I don't get this to > > > > render > > > > > > > anything in content_shell: > > > > > > > > > > > > > > <!DOCTYPE html> > > > > > > > <style> > > > > > > > .display-when-first-child { display: none; } > > > > > > > .display-when-first-child:first-child { display: block; } > > > > > > > </style> > > > > > > > <div id="test"></div> > > > > > > > <div class="display-when-first-child">PASS</div> > > > > > > > <script> > > > > > > > document.body.removeChild(test); > > > > > > > </script> > > > > > > > > > > > > > > The removeChild triggers a style recalc to happen before a layout tree > > is > > > > > > > attached the first time. We walk the tree on style recalc but the > > computed > > > > > style > > > > > > > parent is always null as we don't have any layoutObject parents. We > > end up > > > > > not > > > > > > > generating any ComputedStyle objects at all, and the layout tree > > > > attachment > > > > > is > > > > > > > just skipped afterwards. > > > > > > > > > > > > I'm not able to generate layout trees at all here ... > > > > > > > > > > I am assuming you mean with patch 4 in this comment. The output I get for > > > > > fast/css/first-letter-removed-added.html is as follows: > > > > > > > > > > > https://drive.google.com/a/chromium.org/file/d/0ByKKRKBF20RYdGhHcGhCUVlFcWs/v.... > > > > > > > > > > > > > > > For the test case you have given above, running the code in Patch 4 - I > > get > > > > > PASS. > > > > > > > > Yes. PS4. > > > > > > Wait a sec, I actually applied this on top of all my patches for > > crbug.com/567021 which might explain the difference. Even though my changes > > passes all tests, updating active style happens differently. > > > > I'm sorry I am a bit lost are you saying that the patch set here when added over > > your patches (are these local patches? or submitted ones on ToT) works correctly > > and layout tests pass? > > Applied the last patch here: > > https://codereview.chromium.org/1913833002/ Yup, I just did the same and I get no visible output to the screen. With just my patch I do get output. The only suspect I can come up with is the change to Document::updateStyleAndLayoutTreeIgnorePendingStylesheets in your patch since that is the only thing that could be related amongst our work.
On 2016/11/25 10:01:08, nainar wrote: > On 2016/11/25 at 08:51:38, rune wrote: > > On 2016/11/25 08:46:36, nainar wrote: > > > On 2016/11/25 at 08:01:50, rune wrote: > > > > On 2016/11/24 10:40:38, rune wrote: > > > > > On 2016/11/24 10:37:46, nainar wrote: > > > > > > On 2016/11/24 at 09:22:38, rune wrote: > > > > > > > On 2016/11/24 09:07:22, rune wrote: > > > > > > > > On 2016/11/24 08:59:11, nainar wrote: > > > > > > > > > > > > > > > > > If you take fast/css/first-letter-removed-added.html for > example. > > > While > > > > > > > > > everything is laid out correctly, the testharness results aren't > > > > > visible. > > > > > > > > > None of the LayoutTests results are showing up. All the tests > pass, > > > It's > > > > > > just > > > > > > > > > the testHarness results aren't visible. > > > > > > > > > > > > > > > > Based on a failing test when I applied the patch, I don't get this > to > > > > > render > > > > > > > > anything in content_shell: > > > > > > > > > > > > > > > > <!DOCTYPE html> > > > > > > > > <style> > > > > > > > > .display-when-first-child { display: none; } > > > > > > > > .display-when-first-child:first-child { display: block; } > > > > > > > > </style> > > > > > > > > <div id="test"></div> > > > > > > > > <div class="display-when-first-child">PASS</div> > > > > > > > > <script> > > > > > > > > document.body.removeChild(test); > > > > > > > > </script> > > > > > > > > > > > > > > > > The removeChild triggers a style recalc to happen before a layout > tree > > > is > > > > > > > > attached the first time. We walk the tree on style recalc but the > > > computed > > > > > > style > > > > > > > > parent is always null as we don't have any layoutObject parents. > We > > > end up > > > > > > not > > > > > > > > generating any ComputedStyle objects at all, and the layout tree > > > > > attachment > > > > > > is > > > > > > > > just skipped afterwards. > > > > > > > > > > > > > > I'm not able to generate layout trees at all here ... > > > > > > > > > > > > I am assuming you mean with patch 4 in this comment. The output I get > for > > > > > > fast/css/first-letter-removed-added.html is as follows: > > > > > > > > > > > > > > > https://drive.google.com/a/chromium.org/file/d/0ByKKRKBF20RYdGhHcGhCUVlFcWs/v.... > > > > > > > > > > > > > > > > > > For the test case you have given above, running the code in Patch 4 - > I > > > get > > > > > > PASS. > > > > > > > > > > Yes. PS4. > > > > > > > > Wait a sec, I actually applied this on top of all my patches for > > > crbug.com/567021 which might explain the difference. Even though my changes > > > passes all tests, updating active style happens differently. > > > > > > I'm sorry I am a bit lost are you saying that the patch set here when added > over > > > your patches (are these local patches? or submitted ones on ToT) works > correctly > > > and layout tests pass? > > > > Applied the last patch here: > > > > https://codereview.chromium.org/1913833002/ > > Yup, I just did the same and I get no visible output to the screen. With just my > patch I do get output. The only suspect I can come up with is the change to > Document::updateStyleAndLayoutTreeIgnorePendingStylesheets in your patch since > that is the only thing that could be related amongst our work. Yes. I removed the resolverChanged(FullStyleUpdate) which, in addition to re-collecting active stylesheets for all scopes, would force a style recalc of the whole tree. In the 567021 world, recollecting stylesheets should be enough as the diff with the previous active stylesheets will mark relevant elements to be recalculated. The only exception is the case where we have used placeholder styles, in which case the elements have style which did not reflect the active stylesheets at the time. That is handled in the "else" with a SubtreeStyleChange on Document.
Hi rune@, I have made some changes and commented inline to add some context to why some of those changes (in this and earlier patches have been made.) Hope this helps you investigate. Thanks for the help again! https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (left): https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:94: DCHECK(needsAttach()); This is unnecessary - maybe replace this with a check that ensures that needsReattachLayoutTree() is false? https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:552: if (!oldChild.needsAttach()) This statemement is no longer true since we are not setting the NeedsReattachStyleChange flag anymore in Node::detachLayoutTree() however we are setting the NeedsReattachLayotuTree flag which we should check here. https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:2376: setNeedsReattachLayoutTree(); This is as ContainerNode::attachLayoutTree() asserts that this flag is set. https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.h (left): https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.h:1451: HeapHashMap<Member<Element>, StyleReattachData> m_styleReattachDataMap; Changed this and all relevant functions since we need to store information about both Element and Text nodes. https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:186: DCHECK(needsAttach()); See comment in ContainerNode destructor. https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1665: if (hasRareData() && getStyleChangeType() == NeedsReattachStyleChange) { We are setting the corresponging NeedsReattachLayoutTree - check that here instead. https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:3269: element->setLocalNeedsReattachLayoutTree(); Only the Pseudo Element needs to be reattached. https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:890: if (getStyleChangeType() <= SubtreeStyleChange) Question: This will always be true since SubtreeStyleChange is the max value you can set StyleChangeType to be now. Should this be ommitted now? Or replaced with !needsReattachLayoutTree() https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:922: setNeedsReattachLayoutTree(); Set the relevant NeedsReattachLayoutTree flag. https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.h (left): https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:399: bool needsAttach() const { Redundant information this is encapsulated in needsReattachLayoutTree() check now. https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:87: SubtreeStyleChange = 2 << nodeStyleChangeShift Redundant information this is encapsulated in NeedsReattachLayoutTree flag. https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:977: markAncestorsWithChildNeedsReattachLayoutTree(); Changed to the equivalent reattachLayoutTree function. https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Text.cpp (right): https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Text.cpp:384: if (getStyleChangeType() <= SubtreeStyleChange) Again this will always be true. Make it a no op? Or replace with !needsReattachLayoutTree()
Description was changed from ========== Call Element::rebuildLayoutTree from Document::updateStyle directly This patch changes the callstack in Document::updateStyle to call Element::rebuildLayoutTree after Element::recalcStyle instead of from it. It also adds helper functions needed to perform Layout Tree Construction: 1. ContainerNode::rebuildDescendantLayoutTree() 2. Element::rebuildShadowRootLayoutTree() 3. Element::reattachPseudoElementLayoutTree(PseudoId pseudoId) 4. ShadowRoot::rebuildLayoutTree() It also changes the setting/getting of the (Child)NeedsStyleRecalc and (Child)NeedsReattachLayoutTree bits to match this pattern. It takes a performance hit by calling nextTextSibling() - this is in the interim while bugsnash works on a solution (see design doc - linked in the bug) BUG=595137 ========== to ========== Call Element::rebuildLayoutTree from Document::updateStyle directly This patch changes the callstack in Document::updateStyle to call Element::rebuildLayoutTree after Element::recalcStyle instead of from it. It also adds helper functions needed to perform Layout Tree Construction: 1. ContainerNode::rebuildDescendantLayoutTree() 2. Element::rebuildShadowRootLayoutTree() 3. Element::reattachPseudoElementLayoutTree(PseudoId pseudoId) 4. ShadowRoot::rebuildLayoutTree() It also changes the setting/getting of the (Child)NeedsStyleRecalc and (Child)NeedsReattachLayoutTree bits to match this pattern. BUG=595137 ==========
Description was changed from ========== Call Element::rebuildLayoutTree from Document::updateStyle directly This patch changes the callstack in Document::updateStyle to call Element::rebuildLayoutTree after Element::recalcStyle instead of from it. It also adds helper functions needed to perform Layout Tree Construction: 1. ContainerNode::rebuildDescendantLayoutTree() 2. Element::rebuildShadowRootLayoutTree() 3. Element::reattachPseudoElementLayoutTree(PseudoId pseudoId) 4. ShadowRoot::rebuildLayoutTree() It also changes the setting/getting of the (Child)NeedsStyleRecalc and (Child)NeedsReattachLayoutTree bits to match this pattern. BUG=595137 ========== to ========== Call Element::rebuildLayoutTree from Document::updateStyle directly This patch changes the callstack in Document::updateStyle to call Element::rebuildLayoutTree after Element::recalcStyle instead of from it. It also adds helper functions needed to perform Layout Tree Construction: 1. ContainerNode::rebuildDescendantLayoutTree() 2. Element::rebuildShadowRootLayoutTree() 3. Element::reattachPseudoElementLayoutTree(PseudoId pseudoId) 4. ShadowRoot::rebuildLayoutTree() It also changes the setting/getting of the (Child)NeedsStyleRecalc and (Child)NeedsReattachLayoutTree bits to match this pattern. BUG=595137, 658092 ==========
I've applied the latest patch set on master from Saturday (no local patches this time). This simple case shows up blank: <!DOCTYPE html> <div>PASS</div> The setNeedsReattachLayoutTree flag is only set on Document, is cleared on attaching Document::initialize before any elements are added and never set again. https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (left): https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:94: DCHECK(needsAttach()); On 2016/11/29 06:13:39, nainar wrote: > This is unnecessary - maybe replace this with a check that ensures that > needsReattachLayoutTree() is false? Wouldn't this correspond with needsReattachLayoutTree() being true? That is, detachLayoutTree() has been called before the node is deleted? https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:552: if (!oldChild.needsAttach()) On 2016/11/29 06:13:39, nainar wrote: > This statemement is no longer true since we are not setting the > NeedsReattachStyleChange flag anymore in Node::detachLayoutTree() however we are > setting the NeedsReattachLayotuTree flag which we should check here. Shouldn't this be !needsReattachLayoutTree()? https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:890: if (getStyleChangeType() <= SubtreeStyleChange) On 2016/11/29 06:13:39, nainar wrote: > Question: This will always be true since SubtreeStyleChange is the max value you > can set StyleChangeType to be now. Should this be ommitted now? Or replaced with > !needsReattachLayoutTree() Yes, this test doesn't make sense now. If you setNeedsReattachLayoutTree() in detachLayoutTree(), the check here should probably be !needsReattachLayoutTree(), yes. https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Text.cpp (right): https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Text.cpp:384: if (getStyleChangeType() <= SubtreeStyleChange) On 2016/11/29 06:13:39, nainar wrote: > Again this will always be true. Make it a no op? Or replace with > !needsReattachLayoutTree() Replace with !needsReattachLayoutTree(), I think.
Per meeting discussion I think you want to leave a bunch of the needsAttach() machinery alone. https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (left): https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:94: DCHECK(needsAttach()); Leave this ASSERT alone, it's making sure by the time the node is destroyed that it has already been detached. https://codereview.chromium.org/2473743003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:552: if (!oldChild.needsAttach()) Leave this alone.
@esprehn, Made the changes you asked for in patch 7. Changes explained inline as comments. With this patch if we use the following test case: <style> .test:first-letter { font-size:64px; } </style> <div class="test"><span id="test6" style="float:left">To</span></div> <script> document.body.offsetTop; // Force layout test6.style.float = "none"; </script> The expected result is a T with font size 64 px followed by the letter o with a font size of 12 px. The actual result with this patch is a To with font size 12 px. My investigation shows that we aren't creating the FirstLetterPseudoElement for the div element. We are setting the childNeedsReattachLayoutTree bit on div (and the needsReattachLayoutTree bit on span) in this test case when we set float to none. This skips the reattachLayoutTree recursion. We don't go into Element::attachLayoutTree and don't create the FirstLetterPseudoElement. In this case should we be calling createPseudoElementIfNeeded(FirstLetter) in Element::rebuildLayoutTree? Only running into this issue with FirstLetter as I ran all of fast test suite and have 4 first-letter tests failing only. https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1861: clearNeedsReattachLayoutTree(); In case we don't perform recalcOwnStyle we will never clear the NeedsReattachLayoutTree flag which is set on the creation of each Node. Clear that here. https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:916: setLocalNeedsReattachLayoutTree(); make sure that we are setting the NeedsReattachLayoutTree flag on each node that we set NeedsReattachStyleChange information on. https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Text.cpp (right): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Text.cpp:398: clearNeedsReattachLayoutTree(); In case we don't perform rebuildTextLayoutTree we will never clear the NeedsReattachLayoutTree flag which is set on the creation of each Node. Clear that here.
https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ContainerNode.h (right): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ContainerNode.h:243: void rebuildDescendantLayoutTrees(); This name is strange (as I commented in the design doc). I think you mean reattach descendent nodes or rebuild children trees. I recommend rebuildChildrenLayoutTrees. https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:3249: was this intentional? https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1861: clearNeedsReattachLayoutTree(); On 2016/12/07 at 14:48:21, nainar wrote: > In case we don't perform recalcOwnStyle we will never clear the NeedsReattachLayoutTree flag which is set on the creation of each Node. Clear that here. good explanation, would be useful in a code comment https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:916: setLocalNeedsReattachLayoutTree(); On 2016/12/07 at 14:48:21, nainar wrote: > make sure that we are setting the NeedsReattachLayoutTree flag on each node that we set NeedsReattachStyleChange information on. this function is difficult to understand at first glance. shouldn't this be calling setNeedsReattachLayoutTree() so that the ancestors are marked too? alternatively if you're not setting it cause you can guarantee it will be immediately attached after this then setting the NeedsReattachLayoutTree flag shouldn't be necessary https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Text.cpp (right): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Text.cpp:398: clearNeedsReattachLayoutTree(); On 2016/12/07 at 14:48:21, nainar wrote: > In case we don't perform rebuildTextLayoutTree we will never clear the NeedsReattachLayoutTree flag which is set on the creation of each Node. Clear that here. as above, if there is a need to explain the code in the review, the explanation would be useful in the code itself
https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ContainerNode.h (right): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ContainerNode.h:243: void rebuildDescendantLayoutTrees(); On 2016/12/07 at 23:21:44, Bugs Nash wrote: > This name is strange (as I commented in the design doc). I think you mean reattach descendent nodes or rebuild children trees. I recommend rebuildChildrenLayoutTrees. I'm good with renaming it to rebuildChildrenLayoutTrees() -> done so locally. https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:3249: Nope - haven't sanitized the code for erroneous formating issues. Fixed this issue locally. https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1861: clearNeedsReattachLayoutTree(); Done locally. https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:916: setLocalNeedsReattachLayoutTree(); On 2016/12/07 at 23:21:44, Bugs Nash wrote: > On 2016/12/07 at 14:48:21, nainar wrote: > > make sure that we are setting the NeedsReattachLayoutTree flag on each node that we set NeedsReattachStyleChange information on. > > this function is difficult to understand at first glance. shouldn't this be calling setNeedsReattachLayoutTree() so that the ancestors are marked too? alternatively if you're not setting it cause you can guarantee it will be immediately attached after this then setting the NeedsReattachLayoutTree flag shouldn't be necessary As per discussion with esprehn@, we want to set the NeedsReattachLayoutTree flag only on the node itself and check that wherever we set the NeedsReattachStyleChange flag to make sure that we attach all nodes that need it and then clear the bits correctly. The line directly above also doesn't call Node::setNeedsStyleRecalc() with the NeedsReattachStyleChange flag (which would amongst other things set the childNeedsStyleRecalc flag on the ancestors) but instead only sets the flag on the node itself. https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Text.cpp (right): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Text.cpp:398: clearNeedsReattachLayoutTree(); Done locally.
https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:916: setLocalNeedsReattachLayoutTree(); On 2016/12/07 at 23:49:10, nainar wrote: > On 2016/12/07 at 23:21:44, Bugs Nash wrote: > > On 2016/12/07 at 14:48:21, nainar wrote: > > > make sure that we are setting the NeedsReattachLayoutTree flag on each node that we set NeedsReattachStyleChange information on. > > > > this function is difficult to understand at first glance. shouldn't this be calling setNeedsReattachLayoutTree() so that the ancestors are marked too? alternatively if you're not setting it cause you can guarantee it will be immediately attached after this then setting the NeedsReattachLayoutTree flag shouldn't be necessary > > As per discussion with esprehn@, we want to set the NeedsReattachLayoutTree flag only on the node itself and check that wherever we set the NeedsReattachStyleChange flag to make sure that we attach all nodes that need it and then clear the bits correctly. > > The line directly above also doesn't call Node::setNeedsStyleRecalc() with the NeedsReattachStyleChange flag (which would amongst other things set the childNeedsStyleRecalc flag on the ancestors) but instead only sets the flag on the node itself. If this is similar to the above call then I would recommend naming it similarly. It's not clear what 'local' means here.
Made all the changes and uploaded to patch 9. Thanks for the review! https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2473743003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:916: setLocalNeedsReattachLayoutTree(); On 2016/12/08 at 00:27:59, Bugs Nash wrote: > On 2016/12/07 at 23:49:10, nainar wrote: > > On 2016/12/07 at 23:21:44, Bugs Nash wrote: > > > On 2016/12/07 at 14:48:21, nainar wrote: > > > > make sure that we are setting the NeedsReattachLayoutTree flag on each node that we set NeedsReattachStyleChange information on. > > > > > > this function is difficult to understand at first glance. shouldn't this be calling setNeedsReattachLayoutTree() so that the ancestors are marked too? alternatively if you're not setting it cause you can guarantee it will be immediately attached after this then setting the NeedsReattachLayoutTree flag shouldn't be necessary > > > > As per discussion with esprehn@, we want to set the NeedsReattachLayoutTree flag only on the node itself and check that wherever we set the NeedsReattachStyleChange flag to make sure that we attach all nodes that need it and then clear the bits correctly. > > > > The line directly above also doesn't call Node::setNeedsStyleRecalc() with the NeedsReattachStyleChange flag (which would amongst other things set the childNeedsStyleRecalc flag on the ancestors) but instead only sets the flag on the node itself. > > If this is similar to the above call then I would recommend naming it similarly. It's not clear what 'local' means here. Yup that makes sense calling setFlag(NeedsReattachLayoutTree) directly here instead of that. Made the change locally.
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Lgtm, amazing patch! This is how I feel about this patch: http://i.imgur.com/jrb2WgX.gif https://codereview.chromium.org/2473743003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Text.cpp (right): https://codereview.chromium.org/2473743003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Text.cpp:404: styleReattachData.computedStyle = nullptr; You can add = nullptr in the struct itself so this is the default.
The CQ bit was checked by nainar@chromium.org
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/2473743003/#ps200001 (title: "Address esprehn's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by nainar@chromium.org
https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ContainerNode.cpp:1313: clearChildNeedsStyleRecalc(); I don't think you need to clear this bit, it should be cleared already by the reattach logic above if everything is working correctly. https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2001: DCHECK(!childNeedsStyleRecalc()); Yeah so you assert !childNeedsStyleRecalc here so there's no reason to clear it inside rebuildChildrenLayoutTrees() https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/StyleReattachData.h (right): https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/StyleReattachData.h:13: StyleReattachData() : computedStyle(nullptr) {} RefPtr actually defaults to nullptr, no reason to assign this. https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Text.cpp (right): https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Text.cpp:404: styleReattachData.computedStyle = nullptr; remove this line https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right): https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:135: setNeedsReattachLayoutTree(); Hmm wait this doesn't seem right, why are you forcing a reattach of the ShadowRoot on every recalcStyle? https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:145: clearChildNeedsReattachLayoutTree(); This bit is cleared inside rebuildChildrenLayoutTrees? I don't think you need to clear it here.
Replied inline to your comments. https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ContainerNode.cpp:1313: clearChildNeedsStyleRecalc(); Done. https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2001: DCHECK(!childNeedsStyleRecalc()); ACK. https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/StyleReattachData.h (right): https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/StyleReattachData.h:13: StyleReattachData() : computedStyle(nullptr) {} Done. https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Text.cpp (right): https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Text.cpp:404: styleReattachData.computedStyle = nullptr; Done. https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right): https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:135: setNeedsReattachLayoutTree(); So in the following test case: <script> onload = function() { var sr = document.getElementById('host').createShadowRoot(); } </script> </div> <div id="host"> When we create the ShadowRoot we set NeedsReattachLayoutTree flag on it. It's parent div host never has its childNeedsReattachLayoutTree flag set. And hence it doesnt enter the childNeedsReattachLayoutTree part of Element::rebuildLayoutTree and never clears the flag on the ShadowRoot. https://codereview.chromium.org/2473743003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:145: clearChildNeedsReattachLayoutTree(); ShadowRoot::rebuildLayoutTree() also gets called by Element::rebuildShadowRootLayoutTree() which doesn't clear the bit.
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
For the crashing test: svg/as-image/svg-nested.html. Attaching a gdb instance to content shell shows the following: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fffbd988700 (LWP 28120)] 0x00007fffe8b45aec in blink::FlatTreeTraversal::assertPrecondition (node=...) at ../../third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.h:127 127 DCHECK(!node.needsDistributionRecalc()); Stack trace: #0 0x00007fffe8b45aec in blink::FlatTreeTraversal::assertPrecondition (node=...) at ../../third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.h:127 #1 0x00007fffe8b45a89 in blink::FlatTreeTraversal::parent (node=..., details=0x0) at ../../third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.h:173 #2 0x00007fffe8d7fe66 in blink::LayoutTreeBuilderTraversal::parent (node=..., details=0x0) at ../../third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.cpp:68 #3 0x00007fffe8d31fc0 in blink::Node::parentComputedStyle (this=0x307b6d95a010) at ../../third_party/WebKit/Source/core/dom/NodeComputedStyle.h:55 #4 0x00007fffe8d22e5c in blink::Element::recalcStyle (this=0x307b6d95a010, change=blink::Reattach, nextTextSibling=0x0) at ../../third_party/WebKit/Source/core/dom/Element.cpp:1858 #5 0x00007fffe8c6240e in blink::ContainerNode::recalcDescendantStyles (this=0x307b6d959f70, change=blink::Reattach) at ../../third_party/WebKit/Source/core/dom/ContainerNode.cpp:1291 #6 0x00007fffe8d22fdf in blink::Element::recalcStyle (this=0x307b6d959f70, change=blink::Reattach, nextTextSibling=0x0) at ../../third_party/WebKit/Source/core/dom/Element.cpp:1886 The node being passed here is an "a". For the test timing out: fast/dom/SelectorAPI/resig-SelectorsAPI-test.xhtml It seems like needsAttach() is not being set correctly which is causing us to not early exit and avoid a n^2 loop. I have investigated some and wasn't able to pin down the issue. Unfortunately I am going on vacation. Bugs will be coordinating on this. https://codereview.chromium.org/2473743003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1883: else In case we don't perform ShadowRoot::recalcStyle we will never clear the NeedsReattachLayoutTree flag which is set on the creation of each Node. Clear that here. Edit
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Elliott, I have rewritten the patch and fast/forms/textfield-to-password-on-focus.html is failing consistently across all platforms. This is a new failure which makes me think someone has landed a change in this field causing a difference? fast/css/css3-ch-unit.html is failing on Mac and Windows. I've stepped through a log of functions executed and looked at the code path in gdb and can't find the issue. Can you please take a look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/01/25 09:09:13, nainar wrote: > Elliott, > > I have rewritten the patch and fast/forms/textfield-to-password-on-focus.html is > failing consistently across all platforms. This is a new failure which makes me > think someone has landed a change in this field causing a difference? > fast/css/css3-ch-unit.html is failing on Mac and Windows. > > I've stepped through a log of functions executed and looked at the code path in > gdb and can't find the issue. > > Can you please take a look? I've reduced textfield-to-password-on-focus.html to: <!DOCTYPE html> <p>The input should be focused and you should be able to type into it</p> <input type="text" id="field" onfocus="this.setAttribute('type', 'password')"> <script> onload = () => field.focus(); </script> and debugged to find that some custom style callbacks expect the layout tree to be (re-)attached when called. In particular, HTMLFormControlElement::didRecalcStyle() calls updateFromElement on the layout object, but that needs to happen after the (re-)attach. You need to go through the custom style callbacks to see if they need be be called on recalc, on attach, or both, and move the attach-dependent callback code to happen after the attachment. PS. I wonder if this was a problem without this CL if the onfocus handler triggered a re-attachment further up the tree? On another thought, this is probably OK if the re-attachment of the input element happens. The re-attachment happens on one of the UA shadow elements in this case.
On 2017/01/25 12:13:22, rune wrote: > PS. I wonder if this was a problem without this CL if the onfocus handler > triggered a re-attachment further up the tree? On another thought, this is > probably OK if the re-attachment of the input element happens. The re-attachment > happens on one of the UA shadow elements in this case. Note, I haven't confirmed all the details here, but I've debugged to the point where the input is recalc'ed with NoInherit, and its childNeedsReattachLayoutTree() returns true.
Thank you for the catch! Looking at the implementations for didStyleRecalc I think the following is the case Element::didStyleRecalc -> just a DCHECK FirstLetterPseudoElement::didStyleRecalc -> should be called after style recalc PseudoElement::didStyleRecalc -> should be called after style recalc HTMLSelectElement::didStyleRecalc -> doesn't need a LayoutObject HTMLMediaElement::didStyleRecalc -> assumes attach happened HTMLFormControlElement::didStyleRecalc -> assumes attach happened By that logic the code should change to: Element::recalcStyle() ... if (hasCustomStyleCallbacks() && (isPseudoElement() || is a HTMLSelectElement)) didRecalcStyle(); -> deal with PseudoElements Element::rebuildLayoutTree() ... if (hasCustomStyleCallbacks() && !isPseudoElement()) didRecalcStyle(); -> deal with the others // reattachWhitespaceLayoutObjects However given that the form uses neither of these kinds of elements, I don't see how that could be related? Thanks for the help!
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/01/27 at 07:08:16, nainar wrote: > Thank you for the catch! > > Looking at the implementations for didStyleRecalc I think the following is the case > > Element::didStyleRecalc -> just a DCHECK > FirstLetterPseudoElement::didStyleRecalc -> should be called after style recalc > PseudoElement::didStyleRecalc -> should be called after style recalc > HTMLSelectElement::didStyleRecalc -> doesn't need a LayoutObject > > HTMLMediaElement::didStyleRecalc -> assumes attach happened > HTMLFormControlElement::didStyleRecalc -> assumes attach happened > > By that logic the code should change to: > > Element::recalcStyle() > ... > if (hasCustomStyleCallbacks() && (isPseudoElement() || is a HTMLSelectElement)) > didRecalcStyle(); -> deal with PseudoElements > > Element::rebuildLayoutTree() > ... > if (hasCustomStyleCallbacks() && !isPseudoElement()) > didRecalcStyle(); -> deal with the others > // reattachWhitespaceLayoutObjects > > However given that the form uses neither of these kinds of elements, I don't see how that could be related? > > Thanks for the help! Have uploaded the patch having split the call to Element::didRecalcStyle into Element::didRecalcStyle() (for PseudoElement, FirstLetterPseudoElement, HTMLSelectElement) and Element::didRebuildLayoutTree() (for HTMLFormControlElement and HTMLMediaElement) and calling them from Element::recalcStyle() and Element::rebuildLayoutTree() respectively. Also checked calls to Element::willRecalcStyle to be safe. Nothing that needs porting there.
Hmm, didRebuildLayoutTree is the same as ::attachLayoutTree() if you call super first?
Hi all, FYI https://codereview.chromium.org/2473743003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2473743003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:906: if (getStyleChangeType() < NeedsReattachStyleChange) Figured out why tests were failing! This check here will always be true if I call clearNeedsStyleRecalc() in Element::recalcStyle() (which clears the styleChangeType making it always less than NeedsReattachStyleChange) due to which we are performing detachLayoutTree() in more cases than is necessary. So looks like I can't clear that flag there. Working on this now. Will have a patch up soon once I make sure all the asserts are in order. Just posting an update here.
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi all, Patch with fix is uploaded now. PTAL?
Looking very cool, the unconditional reattach on shadow root means every style recalc through shadow Dom ends up walking down through there doing nothing. Something isn't right there, you shouldn't need to force the bit on. https://codereview.chromium.org/2473743003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1930: root->clearNeedsReattachLayoutTree(); I wonder how this can happen, how do we get here with the reattach flag set but without the recalc style flag set? https://codereview.chromium.org/2473743003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Text.cpp (right): https://codereview.chromium.org/2473743003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Text.cpp:400: clearNeedsReattachLayoutTree(); How was this bit set without ended up inside rebuild layout tree? https://codereview.chromium.org/2473743003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right): https://codereview.chromium.org/2473743003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:156: setNeedsReattachLayoutTree(); This is setting reattach unconditionally on every shadow root which isn't right. You should never need to force this bit on here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Elliott, Answered your questions. https://codereview.chromium.org/2473743003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1930: root->clearNeedsReattachLayoutTree(); Yup this is redundant code and never executed. Removed. https://codereview.chromium.org/2473743003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Text.cpp (right): https://codereview.chromium.org/2473743003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Text.cpp:400: clearNeedsReattachLayoutTree(); It was set on the text node's creation because NeedsReattachLayoutTree flag was added to the DefaultNodeFlags which every Text node is initialized with. https://codereview.chromium.org/2473743003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right): https://codereview.chromium.org/2473743003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:156: setNeedsReattachLayoutTree(); When we call ShadowRoot::recalcStyle() we have no way of knowing when we need to call ShadowRoot::rebuildLayoutTree() currently only the NeedsReattachLayoutTree flag is set on the ShadowRoot itself. However it's ancestors dont have this information. I understand that we are indiscriminately calling ShadowRoot::rebuildLayoutTree() as a result of this. The only other way I see is to check if the NeedsReattachLayoutTree flag is set and then mark the ancestors accordingly. Which is the same as calling setNeedsReattachLayoutTree()
https://codereview.chromium.org/2473743003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right): https://codereview.chromium.org/2473743003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:155: // otherwise we will never rebuild the LayoutTree. I see what you're missing is a call to host->setChildNeedsReattachLayoutTree() inside the shadow root creation path. Look at the code inside ElementShadow addShadowRoot. Youll see it already sets bits like this. You just need to do the same. Note you probably need code for both v1 and v0 shadow roots.
On 2017/02/02 at 23:31:03, esprehn wrote: > https://codereview.chromium.org/2473743003/diff/360001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right): > > https://codereview.chromium.org/2473743003/diff/360001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:155: // otherwise we will never rebuild the LayoutTree. > I see what you're missing is a call to host->setChildNeedsReattachLayoutTree() inside the shadow root creation path. Look at the code inside ElementShadow addShadowRoot. Youll see it already sets bits like this. You just need to do the same. Note you probably need code for both v1 and v0 shadow roots. Looks like the v0 and v1 cases are handled the same. You just need to add a line calling shadowHost.setChildNeeds...() Here: https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...
On 2017/02/03 at 00:11:35, esprehn wrote: > On 2017/02/02 at 23:31:03, esprehn wrote: > > https://codereview.chromium.org/2473743003/diff/360001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right): > > > > https://codereview.chromium.org/2473743003/diff/360001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:155: // otherwise we will never rebuild the LayoutTree. > > I see what you're missing is a call to host->setChildNeedsReattachLayoutTree() inside the shadow root creation path. Look at the code inside ElementShadow addShadowRoot. Youll see it already sets bits like this. You just need to do the same. Note you probably need code for both v1 and v0 shadow roots. > > Looks like the v0 and v1 cases are handled the same. You just need to add a line calling shadowHost.setChildNeeds...() Here: > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... Yup added that - fixing a couple of asserts that are failing. Will get a CL uploaded soon hopefully
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (left): https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ContainerNode.cpp:771: childAttachedAllowedWhenAttachingChildren(this)); Lets add this back? https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ContainerNode.cpp:1283: DCHECK(!needsStyleRecalc()); ditto https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2050: DCHECK(parentNode()); Can we add these DCHECKs back? https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1944: if (!(needsReattachLayoutTree() || childNeedsReattachLayoutTree())) Run demorgans here https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp (right): https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp:85: shadowHost.setChildNeedsStyleRecalc(); remove this line it doesn't do anything since we do setNeedsStyleRecalc below. shadowRoot->setNeedsReattachLayoutTree(); https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp:86: shadowHost.setChildNeedsReattachLayoutTree(); you can remove this one. https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right): https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:155: host().markAncestorsWithChildNeedsReattachLayoutTree(); remove all this.
Elliott, Made all the changes as per the meeting. Still working on a few asserts crashing in: fast/forms/datalist/update-range-with-datalist.html fast/forms/text/text-update-datalist-while-focused.html fast/forms/text/text-appearance-datalist-dynamic.html fast/forms/number/number-appearance-datalist-dynamic.html fast/dom/HTMLImageElement/image-sizes-viewport-with-template-parent-and-empty-sizes.html Let me know what you think https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (left): https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ContainerNode.cpp:771: childAttachedAllowedWhenAttachingChildren(this)); Done. https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ContainerNode.cpp:1283: DCHECK(!needsStyleRecalc()); This information isn't true anymore. We are retaining the needsStyleRecalc information https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2050: DCHECK(parentNode()); They are back. They moved above. https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1944: if (!(needsReattachLayoutTree() || childNeedsReattachLayoutTree())) Done. https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp (right): https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp:85: shadowHost.setChildNeedsStyleRecalc(); Done. https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp:86: shadowHost.setChildNeedsReattachLayoutTree(); Done. https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right): https://codereview.chromium.org/2473743003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:155: host().markAncestorsWithChildNeedsReattachLayoutTree(); Done.
What asserts do you see? https://codereview.chromium.org/2473743003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right): https://codereview.chromium.org/2473743003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:155: clearNeedsReattachLayoutTree(); this shouldn't be needed, how do we get here with the bits up the tree invalid? if yourself has needsReattachLayoutTree here then you can CHECK(host().childNeedsReattachLayoutTree()) or something is bad
On 2017/02/15 at 02:01:00, esprehn wrote: > What asserts do you see? The asserts I am getting are that there are nodes whose childNeedsReattachLayoutTree bits are set but there parents don't have the bit set. Specifically in the case of ShadowRoots. "Minimized" case here: <script> function insert() { var img = new Image(); var template = document.getElementById('template'); img.sizes = ''; template.appendChild(img); } </script> <body onload="insert()"> <template id ="template"></template> </body> The IMG element in this case has the following bits: needsStyleRecalc 1 childNeedsStyleRecalc 0 needsReattachLayoutTree 0 childNeedsReattachLayoutTree 1 The needsStyleRecalc bit is retained due to childNeedsReattachLayoutTree bit. My working theory is that when we set the needsReattachLayoutTree bit on a ShadowRoot we are not setting the complete information and should be setting some bits on the host too. I might be wrong - actively invetigating https://codereview.chromium.org/2473743003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right): https://codereview.chromium.org/2473743003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:155: clearNeedsReattachLayoutTree(); On 2017/02/15 at 02:01:00, esprehn wrote: > this shouldn't be needed, how do we get here with the bits up the tree invalid? > > if yourself has needsReattachLayoutTree here then you can CHECK(host().childNeedsReattachLayoutTree()) or something is bad Given the following test case: <script> onload = function() { thirdProgress.appendChild(firstProgress); } </script> <progress id="firstProgress"> </progress> <progress id="thirdProgress"></progress> If you DCHECK for parentOrShadowHostNode()->childNeedsReattachLayoutTree() in the case where needsReattachLayoutTree() is true. The assert fails. Print debugging shows the following: When we call thirdProgress.appendChild(firstProgress) we call ContainerNode::removeBetween() which detaches. Which sets the NeedsReattachLayoutTree which the parent nodes aren't aware of. Which needs to be corrected. Which is why I was trying to do here.
On 2017/02/15 at 06:04:46, nainar wrote: > On 2017/02/15 at 02:01:00, esprehn wrote: > > What asserts do you see? > > The asserts I am getting are that there are nodes whose childNeedsReattachLayoutTree bits are set but there parents don't have the bit set. Specifically in the case of ShadowRoots. > > "Minimized" case here: > <script> > function insert() { > var img = new Image(); > var template = document.getElementById('template'); > img.sizes = ''; > template.appendChild(img); > } > </script> > <body onload="insert()"> > <template id ="template"></template> > </body> > > The IMG element in this case has the following bits: > needsStyleRecalc 1 childNeedsStyleRecalc 0 needsReattachLayoutTree 0 childNeedsReattachLayoutTree 1 > > The needsStyleRecalc bit is retained due to childNeedsReattachLayoutTree bit. My working theory is that when we set the needsReattachLayoutTree bit on a ShadowRoot we are not setting the complete information and should be setting some bits on the host too. I might be wrong - actively investigating Intended to send this out yesterday and didnt. In the test case given above, the ShadowHost is an IMG element. Which doesn't have a parent node when it enters ElementShadow::addShadowRoot(). Hence the ShadowRoot has needsReattachLayoutTree and the ShadowHost has childNeedsReattachLayoutTree bit set on it. When in Element::recalcStyle the IMG element has a parent node which doesn' think its descendants needReattachLayoutTree in the above test case. Leaving the bits on IMG and its ShadowRoot set and never cleared. Given the two individual situations the best thing seems to be to do the following: In ElementShadow::addShadowRoot setChildNeedsReattachLayoutTree on the ShadowHost (the ShadowRoot already has the needsReattachLayoutTree bit set) In ShadowRoot::recalcStyle 1. If the ShadowHost has its childNeedsReattachLayoutTree bit set propagate that to the parents of the host (deals with this situation) 2. If not clear the needsReattachLayoutTree bit on the root. (deals with the situation we discussed in the comments) This has the disadvantage of setting unnecessary bits.
Elliott, As per conversation on Hangouts. PTAL?
On 2017/02/16 at 05:30:03, nainar wrote: > Elliott, > > As per conversation on Hangouts. PTAL? Did it pass all the tests?
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/16 at 05:37:02, esprehn wrote: > On 2017/02/16 at 05:30:03, nainar wrote: > > Elliott, > > > > As per conversation on Hangouts. PTAL? > > Did it pass all the tests? It passed the ones on fast and inspector. I am running a dry run here so lets see.
https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (left): https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ContainerNode.cpp:1283: DCHECK(!needsStyleRecalc()); add this back? Recomputing the descendant styles while yourself is still dirty is a bug. https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ContainerNode.cpp:802: setChildNeedsReattachLayoutTree(); You need a separate check for !childNeedsReattach... like the !childNeedsStyleRecalc(), otherwise if this element already had kids that needed a style recalc you'd fail to do this work. I think you want: if (change.isChildInsertion()) { if (!childNeedsStyleRecalc()) { // existing two lines. } if (!childNeedsReattachLayoutTree()) { // new two lines. } } Should be easy to write a test for that too, ex. element.firstElementChild.style.color = "red"; // mark a child for style recalc. element.appendChild(anotherElement); // we wouldn't do any of the code you have here now since it's marked. https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1887: DCHECK(!parentOrShadowHostNode()->needsStyleRecalc()); ditto, entering into here with a dirty style is a serious bug, it means you're inherting from a dirty parent. https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1971: DCHECK(!parentOrShadowHostNode()->needsStyleRecalc()); Add this back, entering into recalcOwnStyle with a dirty style on the parent is a bug. https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1909: clearNeedsReattachLayoutTree(); why would this be true? We should still follow the childNeeds bits in the next phase and eventually get here and clear it? The entry won't be in the map though. How do we get here with the NeedsReattachLayoutTree bit set, but not the ancestor bits set? I don't think you need this code. https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2065: DCHECK(!needsStyleRecalc()); remove, you check it below https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2079: clearNeedsStyleRecalc(); You DCHECK(!needsStyleRecalc()); right above this, so this code can be removed https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Text.cpp (right): https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Text.cpp:398: clearNeedsReattachLayoutTree(); I'm not sure how this is possible, I don't think it is. We should definitely perform a rebuildTextLayoutTree() walking down to here if the bits were set. https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Text.cpp:417: clearNeedsStyleRecalc(); how can this be true? reattachLayoutTree should have cleared this bit above. Remove this line.
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (left): https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ContainerNode.cpp:1283: DCHECK(!needsStyleRecalc()); Done. Needed to move the if (!needsReattachLayoutTree()) clearNeedsStyleRecalc(); up into the scope in which we are calling recalcOwnStyle https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ContainerNode.cpp:802: setChildNeedsReattachLayoutTree(); On 2017/02/16 at 05:49:25, esprehn wrote: > You need a separate check for !childNeedsReattach... like the !childNeedsStyleRecalc(), otherwise if this element already had kids that needed a style recalc you'd fail to do this work. > > I think you want: > > if (change.isChildInsertion()) { > if (!childNeedsStyleRecalc()) { > // existing two lines. > } > if (!childNeedsReattachLayoutTree()) { > // new two lines. > } > } > > Should be easy to write a test for that too, ex. > > element.firstElementChild.style.color = "red"; // mark a child for style recalc. > > element.appendChild(anotherElement); // we wouldn't do any of the code you have here now since it's marked. Done. Add test in this patch or in another one? https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1887: DCHECK(!parentOrShadowHostNode()->needsStyleRecalc()); This isn't true anymore in case we need this information in reattachLayoutTree(). Can bring it back in the case needsReattachLayoutTree is false. https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1971: DCHECK(!parentOrShadowHostNode()->needsStyleRecalc()); This isn't true anymore in case we need this information in reattachLayoutTree(). Can bring it back in the case needsReattachLayoutTree is false. https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1909: clearNeedsReattachLayoutTree(); Done. https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2065: DCHECK(!needsStyleRecalc()); Done. https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2079: clearNeedsStyleRecalc(); Done. https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Text.cpp (right): https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Text.cpp:398: clearNeedsReattachLayoutTree(); Done. https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Text.cpp:417: clearNeedsStyleRecalc(); Done.
https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1887: DCHECK(!parentOrShadowHostNode()->needsStyleRecalc()); On 2017/02/16 at 06:47:54, nainar wrote: > This isn't true anymore in case we need this information in reattachLayoutTree(). Can bring it back in the case needsReattachLayoutTree is false. Hmm? We should have cleared all the bits by calling attach() which then computes the style in order. Can you give a short example? https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1971: DCHECK(!parentOrShadowHostNode()->needsStyleRecalc()); On 2017/02/16 at 06:47:54, nainar wrote: > This isn't true anymore in case we need this information in reattachLayoutTree(). Can bring it back in the case needsReattachLayoutTree is false. This should be true, we're only leaving the style bits behind on the attach roots, and we should never traverse down into recalcOwnStyle on anything under an attach root. Can you give a short markup example?
https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1887: DCHECK(!parentOrShadowHostNode()->needsStyleRecalc()); Test case: <div style="display:none"> <input type="image" id="image" list="dl1"> </div> https://codereview.chromium.org/2473743003/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1971: DCHECK(!parentOrShadowHostNode()->needsStyleRecalc()); Same test case as above. I see what you mean here though. Debugging this.
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Post investigation. https://codereview.chromium.org/2473743003/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1906: if (change != Reattach) This was initially if (!needsReattachLayoutTree) - which is incorrect. We need to retain this information in case we are about to reattach. Which is change == Reattach
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
This looks good great! ...but your bots are red. Looks like maybe you broke style sharing somewhere? https://codereview.chromium.org/2473743003/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1906: if (change != Reattach) On 2017/02/16 at 08:05:57, nainar wrote: > This was initially if (!needsReattachLayoutTree) - which is incorrect. > > We need to retain this information in case we are about to reattach. Which is change == Reattach If we're going to reattach, then we did setNeedsReattachLayoutTree(), so change == Reattach should imply needsReattachLayoutTree() ? Curious where we need this bit still, needsAttach() already checks both. https://codereview.chromium.org/2473743003/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp (left): https://codereview.chromium.org/2473743003/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp:85: shadowHost.setChildNeedsStyleRecalc(); I think we probably want to leave this, while technically it's not needed, it preserves the invariant that if the shaodwRoot needs a recalc (which it does since it's brand new), that the host has the child bit set. (I know this was my suggestion.)
On 2017/02/16 at 21:24:33, esprehn wrote: > This looks good great! ...but your bots are red. Looks like maybe you broke style sharing somewhere? Yup rebased and got these failures consistently locally as well. :( Doesn't look like just style sharing some diplay:none tests are failing as well. We make the styleSharingList in ContainerNode::recalcDescendantStyles(). I think that is the only code that overlaps with my codepath. The list is then accessed in Element::styleForElement I believe. Looking into this. And the others. https://codereview.chromium.org/2473743003/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1906: if (change != Reattach) On 2017/02/16 at 21:24:33, esprehn wrote: > On 2017/02/16 at 08:05:57, nainar wrote: > > This was initially if (!needsReattachLayoutTree) - which is incorrect. > > > > We need to retain this information in case we are about to reattach. Which is change == Reattach > > If we're going to reattach, then we did setNeedsReattachLayoutTree(), so change == Reattach should imply needsReattachLayoutTree() ? Curious where we need this bit still, needsAttach() already checks both. change == Reattach does imply needsReattachLayoutTree() but not the other way round. When we enter reattachLayoutTree() there is a check to prevent us doing useless detach. If we clear the needsStyleRecalc bit that condition will always be true. And we will be unable to skip detachLayoutTree() in cases like we have already been through attachLayoutTree() https://codereview.chromium.org/2473743003/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp (left): https://codereview.chromium.org/2473743003/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp:85: shadowHost.setChildNeedsStyleRecalc(); Sure :)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On a completely unrelated note. Future things to consider: 1. Move StyleChange information on to the HashMap as opposed to retaining on needsStyleRecalc 2. Look into seeing if AttachContext.performingReattach can be replaced with calls to needsReattachLayoutTree instead?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Issue 658092 has been closed by clusterfuzz, perhaps it should it be removed from this patch?
Description was changed from ========== Call Element::rebuildLayoutTree from Document::updateStyle directly This patch changes the callstack in Document::updateStyle to call Element::rebuildLayoutTree after Element::recalcStyle instead of from it. It also adds helper functions needed to perform Layout Tree Construction: 1. ContainerNode::rebuildDescendantLayoutTree() 2. Element::rebuildShadowRootLayoutTree() 3. Element::reattachPseudoElementLayoutTree(PseudoId pseudoId) 4. ShadowRoot::rebuildLayoutTree() It also changes the setting/getting of the (Child)NeedsStyleRecalc and (Child)NeedsReattachLayoutTree bits to match this pattern. BUG=595137, 658092 ========== to ========== Call Element::rebuildLayoutTree from Document::updateStyle directly This patch changes the callstack in Document::updateStyle to call Element::rebuildLayoutTree after Element::recalcStyle instead of from it. It also adds helper functions needed to perform Layout Tree Construction: 1. ContainerNode::rebuildDescendantLayoutTree() 2. Element::rebuildShadowRootLayoutTree() 3. Element::reattachPseudoElementLayoutTree(PseudoId pseudoId) 4. ShadowRoot::rebuildLayoutTree() It also changes the setting/getting of the (Child)NeedsStyleRecalc and (Child)NeedsReattachLayoutTree bits to match this pattern. BUG=595137 ==========
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
With the current patch I have the following failing tests: fast/css/first-letter-rtc-crash.html fast/layout/display-none-no-relayout.html - relaying out more elements than needed. fast/selectors/style-sharing-children-prevent-sharing.html - Elliott could you take a look - this seems like a WAI. We are now allowing stylesharing for x and y - which we didnt have back then which is why the expectation was false. These aren't layout differences but asserts. https://codereview.chromium.org/2473743003/diff/600001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/SharedStyleFinder.cpp (right): https://codereview.chromium.org/2473743003/diff/600001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/SharedStyleFinder.cpp:62: inline ComputedStyle* getElementStyle(Element& element) { As per Hangout conversation, to fix style sharing instead of getting the computedStyle information from the Node in cases where needsReattachLayoutTree() is true we should read it from the HashMap. https://codereview.chromium.org/2473743003/diff/600001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/600001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1906: if (change != Reattach) This is needed because: Node::reattachLayoutTree reads if (getStyleChangeType() < NeedsReattachStyleChange) detachLayoutTree(reattachContext); attachLayoutTree(reattachContext); This worked before we didnt clear the styleChangeType information until after reattach had taken place. Since style resolution and layout tree construction were interlinked. Now that we clear the styleChangeType information way before we do reattachLayoutTree this doesn't work anymore. To fix this I need to retain this information. This could be stashed on the HashMap but that's to be seen later since we would have to access the HashMap repeatedly. This is a hacky fix but the best I can do at the moment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Also I have run the memory.top_10_mobile benchmark on all android bots and am now running the blink_style.top_25 benchmark here: https://codereview.chromium.org/2704133004
On 2017/02/21 at 03:13:29, nainar wrote: > Also I have run the memory.top_10_mobile benchmark on all android bots and am now running the blink_style.top_25 benchmark here: https://codereview.chromium.org/2704133004 Perf test results are on the patch linked above. Please also note that fast/css/first-letter-rtc-crash.html doesn't fail anymore with my patch. Seems erroneous.
On 2017/02/21 at 06:23:33, nainar wrote: > On 2017/02/21 at 03:13:29, nainar wrote: > > Also I have run the memory.top_10_mobile benchmark on all android bots and am now running the blink_style.top_25 benchmark here: https://codereview.chromium.org/2704133004 > > Perf test results are on the patch linked above. > > Please also note that fast/css/first-letter-rtc-crash.html doesn't fail anymore with my patch. Seems erroneous. Yeah I noticed that test was no longer crashing on my patch too. Weird.
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Uploaded in the latest patch with the following changes: bool layoutObjectWillChange = getStyleChangeType() == NeedsReattachStyleChange || layoutObject(); removing needsAttach here as it was forcing reattach on whitespace siblings always. Secondly, if (candidate.needsStyleRecalc() && !candidate.needsReattachLayoutTree()) return false; As we are retaining needsStyleRecalc information in some cases. https://codereview.chromium.org/2473743003/diff/620001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/SharedStyleFinder.cpp (right): https://codereview.chromium.org/2473743003/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/SharedStyleFinder.cpp:255: if (candidate.needsStyleRecalc() && !candidate.needsReattachLayoutTree()) Changed from last patch https://codereview.chromium.org/2473743003/diff/620001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2046: bool layoutObjectWillChange = Changed from last patch
https://codereview.chromium.org/2473743003/diff/600001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/600001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1906: if (change != Reattach) Ah that makes sense. https://codereview.chromium.org/2473743003/diff/600001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1980: } I think you can remove this now that we changed SharedStyleFinder? https://codereview.chromium.org/2473743003/diff/620001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1906: if (change != Reattach) You could add a comment above this that says this is needed because the rebuildLayoutTree code needs to see what the styleChangeType() was on reattach roots. https://codereview.chromium.org/2473743003/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2043: StyleReattachData styleReattachData = document().getStyleReattachData(*this); Move this whole block into the needsReattachLayoutTree() case, you don't need to do the hash map lookups for every element as you walk down the tree. https://codereview.chromium.org/2473743003/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2046: bool layoutObjectWillChange = Move this into the needsReattachLayoutTree() case. https://codereview.chromium.org/2473743003/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2067: if (layoutObjectWillChange || layoutObject()) { You want to move this whole block into the needsReattachLayoutTree() case, otherwise you're reattaching all of the whitespace as you walk down the tree
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Elliott, made the changes you asked for. https://codereview.chromium.org/2473743003/diff/620001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2473743003/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1906: if (change != Reattach) Done. https://codereview.chromium.org/2473743003/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2043: StyleReattachData styleReattachData = document().getStyleReattachData(*this); Done. https://codereview.chromium.org/2473743003/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2046: bool layoutObjectWillChange = Done. https://codereview.chromium.org/2473743003/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2067: if (layoutObjectWillChange || layoutObject()) { Done.
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
The CQ bit was checked by nainar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 640001, "attempt_start_ts": 1487755410735390, "parent_rev": "e359f31a6ca4013fcf2668ad3e5a1c53ae217983", "commit_rev": "8b6c7b7ce5a36d8eda49dd6e60c5e7c8f5c7277f"}
Message was sent while issue was closed.
Description was changed from ========== Call Element::rebuildLayoutTree from Document::updateStyle directly This patch changes the callstack in Document::updateStyle to call Element::rebuildLayoutTree after Element::recalcStyle instead of from it. It also adds helper functions needed to perform Layout Tree Construction: 1. ContainerNode::rebuildDescendantLayoutTree() 2. Element::rebuildShadowRootLayoutTree() 3. Element::reattachPseudoElementLayoutTree(PseudoId pseudoId) 4. ShadowRoot::rebuildLayoutTree() It also changes the setting/getting of the (Child)NeedsStyleRecalc and (Child)NeedsReattachLayoutTree bits to match this pattern. BUG=595137 ========== to ========== Call Element::rebuildLayoutTree from Document::updateStyle directly This patch changes the callstack in Document::updateStyle to call Element::rebuildLayoutTree after Element::recalcStyle instead of from it. It also adds helper functions needed to perform Layout Tree Construction: 1. ContainerNode::rebuildDescendantLayoutTree() 2. Element::rebuildShadowRootLayoutTree() 3. Element::reattachPseudoElementLayoutTree(PseudoId pseudoId) 4. ShadowRoot::rebuildLayoutTree() It also changes the setting/getting of the (Child)NeedsStyleRecalc and (Child)NeedsReattachLayoutTree bits to match this pattern. BUG=595137 Review-Url: https://codereview.chromium.org/2473743003 Cr-Commit-Position: refs/heads/master@{#451987} Committed: https://chromium.googlesource.com/chromium/src/+/8b6c7b7ce5a36d8eda49dd6e60c5... ==========
Message was sent while issue was closed.
Committed patchset #33 (id:640001) as https://chromium.googlesource.com/chromium/src/+/8b6c7b7ce5a36d8eda49dd6e60c5...
Message was sent while issue was closed.
On 2017/02/22 at 10:53:30, commit-bot wrote: > Committed patchset #33 (id:640001) as https://chromium.googlesource.com/chromium/src/+/8b6c7b7ce5a36d8eda49dd6e60c5... YEAH! \o/ \o/ \o/ \o/ \o/ \o/ \o/ \o/
Message was sent while issue was closed.
nainar@chromium.org changed reviewers: - bugsnash@chromium.org
Message was sent while issue was closed.
A revert of this CL (patchset #33 id:640001) has been created in https://codereview.chromium.org/2729453002/ by nainar@chromium.org. The reason for reverting is: Reason for revert is: https://bugs.chromium.org/p/chromium/issues/detail?id=695950 .
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Call Element::rebuildLayoutTree from Document::updateStyle directly This patch changes the callstack in Document::updateStyle to call Element::rebuildLayoutTree after Element::recalcStyle instead of from it. It also adds helper functions needed to perform Layout Tree Construction: 1. ContainerNode::rebuildDescendantLayoutTree() 2. Element::rebuildShadowRootLayoutTree() 3. Element::reattachPseudoElementLayoutTree(PseudoId pseudoId) 4. ShadowRoot::rebuildLayoutTree() It also changes the setting/getting of the (Child)NeedsStyleRecalc and (Child)NeedsReattachLayoutTree bits to match this pattern. BUG=595137 Review-Url: https://codereview.chromium.org/2473743003 Cr-Commit-Position: refs/heads/master@{#451987} Committed: https://chromium.googlesource.com/chromium/src/+/8b6c7b7ce5a36d8eda49dd6e60c5... ========== to ========== Call Element::rebuildLayoutTree from Document::updateStyle directly This patch changes the callstack in Document::updateStyle to call Element::rebuildLayoutTree after Element::recalcStyle instead of from it. It also adds helper functions needed to perform Layout Tree Construction: 1. ContainerNode::rebuildChildrenLayoutTrees() 2. Element::rebuildShadowRootLayoutTree() 3. Element::reattachPseudoElementLayoutTree(PseudoId pseudoId) 4. ShadowRoot::rebuildLayoutTree() It also changes the setting/getting of the (Child)NeedsStyleRecalc and (Child)NeedsReattachLayoutTree bits to match this pattern. The patch also adds a LayoutTest to test for the crash caught in crbug.com/695950. BUG=595137 Review-Url: https://codereview.chromium.org/2473743003 Cr-Commit-Position: refs/heads/master@{#451987} Committed: https://chromium.googlesource.com/chromium/src/+/8b6c7b7ce5a36d8eda49dd6e60c5... ==========
Rune, Changed needsAttach() to only check if getStyleChangeType() == NeedsReattachStyleChange as all callsites to this function were only trying to determine if we are attaching children and not reattach. So reverted the change I made there where I was checking getStyleChangeType() == NeedsReattachStyleChange || getFlag(NeedsReattachLayoutTree). Since you were on crbug.com/695950 and found the solution to the issue at hand, could you PTAL? Also esprehn@ who was active here is on leave. :) I have added the LayoutTest as well as expectations generated by --new-test-results. Have no intention of landing before branch point though so will attempt to land on Monday (Sydney time) if I get an LGTM before that. https://codereview.chromium.org/2473743003/diff/660001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2473743003/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:397: return getStyleChangeType() == NeedsReattachStyleChange; Retaining this as the function should only test for NeedsReattachStyleChange flag whether or not attachLayoutTree() is needed and not reattachLayoutTree()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by nainar@chromium.org
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/2473743003/#ps680001 (title: "Make needsAttach() only check if getStyleChangeType flag is NeedsReattachStyleChange and add a Layo…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp: While running git apply --index -p1; error: patch failed: third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp:86 error: third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp: patch does not apply Patch: third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp Index: third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp diff --git a/third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp b/third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp index e29035dc3be460bdb1f41826b77722664ae310a0..22b61ff7293dbc9fb01b524338768d5cc6d170e6 100644 --- a/third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp +++ b/third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp @@ -86,6 +86,7 @@ ShadowRoot& ElementShadow::addShadowRoot(Element& shadowHost, shadowHost.setNeedsStyleRecalc( SubtreeStyleChange, StyleChangeReasonForTracing::create(StyleChangeReason::Shadow)); + shadowRoot->setNeedsReattachLayoutTree(); InspectorInstrumentation::didPushShadowRoot(&shadowHost, shadowRoot);
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Message was sent while issue was closed.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by nainar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org, rune@opera.com Link to the patchset: https://codereview.chromium.org/2473743003/#ps700001 (title: "Make needsAttach() only check if getStyleChangeType flag is NeedsReattachStyleChange and add a Layo…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nainar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 700001, "attempt_start_ts": 1488772648227200, "parent_rev": "cc8ff78f49b8fe8265a15ed227ec273c08dd695a", "commit_rev": "f4c976fc1f4578b0c75ba304553ef2ff6134f551"}
Message was sent while issue was closed.
Description was changed from ========== Call Element::rebuildLayoutTree from Document::updateStyle directly This patch changes the callstack in Document::updateStyle to call Element::rebuildLayoutTree after Element::recalcStyle instead of from it. It also adds helper functions needed to perform Layout Tree Construction: 1. ContainerNode::rebuildChildrenLayoutTrees() 2. Element::rebuildShadowRootLayoutTree() 3. Element::reattachPseudoElementLayoutTree(PseudoId pseudoId) 4. ShadowRoot::rebuildLayoutTree() It also changes the setting/getting of the (Child)NeedsStyleRecalc and (Child)NeedsReattachLayoutTree bits to match this pattern. The patch also adds a LayoutTest to test for the crash caught in crbug.com/695950. BUG=595137 Review-Url: https://codereview.chromium.org/2473743003 Cr-Commit-Position: refs/heads/master@{#451987} Committed: https://chromium.googlesource.com/chromium/src/+/8b6c7b7ce5a36d8eda49dd6e60c5... ========== to ========== Call Element::rebuildLayoutTree from Document::updateStyle directly This patch changes the callstack in Document::updateStyle to call Element::rebuildLayoutTree after Element::recalcStyle instead of from it. It also adds helper functions needed to perform Layout Tree Construction: 1. ContainerNode::rebuildChildrenLayoutTrees() 2. Element::rebuildShadowRootLayoutTree() 3. Element::reattachPseudoElementLayoutTree(PseudoId pseudoId) 4. ShadowRoot::rebuildLayoutTree() It also changes the setting/getting of the (Child)NeedsStyleRecalc and (Child)NeedsReattachLayoutTree bits to match this pattern. The patch also adds a LayoutTest to test for the crash caught in crbug.com/695950. BUG=595137 Review-Url: https://codereview.chromium.org/2473743003 Cr-Original-Commit-Position: refs/heads/master@{#451987} Committed: https://chromium.googlesource.com/chromium/src/+/8b6c7b7ce5a36d8eda49dd6e60c5... Review-Url: https://codereview.chromium.org/2473743003 Cr-Commit-Position: refs/heads/master@{#454824} Committed: https://chromium.googlesource.com/chromium/src/+/f4c976fc1f4578b0c75ba304553e... ==========
Message was sent while issue was closed.
Committed patchset #36 (id:700001) as https://chromium.googlesource.com/chromium/src/+/f4c976fc1f4578b0c75ba304553e... |