|
Description
Tony Chang
2013-02-12 17:29:38 PST
Created attachment 187976 [details]
Patch
fast/table/border-collapsing/cached-change-row-border-width.html failed on Chromium Linux, looking to see if it fails on Mac too. Comment on attachment 187976 [details] Patch Attachment 187976 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16517457 New failing tests: fast/table/border-collapsing/cached-change-row-border-width.html Comment on attachment 187976 [details] Patch Attachment 187976 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16454761 New failing tests: fast/table/border-collapsing/cached-change-row-border-width.html Comment on attachment 187976 [details] Patch Attachment 187976 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16546168 New failing tests: fast/table/border-collapsing/cached-change-row-border-width.html Created attachment 188189 [details]
Patch
This patch also seems to fix bug 104864. This makes me think that http://trac.webkit.org/changeset/129644 revealed this layout bug by doing fewer style recalcs. Comment on attachment 188189 [details]
Patch
Looks good
Comment on attachment 188189 [details] Patch Clearing flags on attachment: 188189 Committed r142889: <http://trac.webkit.org/changeset/142889> All reviewed patches have been landed. Closing bug. *** Bug 104864 has been marked as a duplicate of this bug. *** This patch appears to have caused an assertion failure on scrollbars/overflow-scrollbar-combinations.html: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=scrollbars%2Foverflow-scrollbar-combinations.html Blame list: http://trac.webkit.org/log/?verbose=on&rev=142890&stop_rev=142888 ASSERTION FAILED: needsLayout() /Volumes/Data/slave/mountainlion-debug/build/Source/WebCore/rendering/RenderBox.cpp(324) : virtual void WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, const WebCore::RenderStyle *) 1 0x10d654c4a WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) 2 0x10d5ca6d3 WebCore::RenderBlock::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) 3 0x10d7b94fd WebCore::RenderScrollbarPart::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) 4 0x10d78cf39 WebCore::RenderObject::setStyle(WTF::PassRefPtr<WebCore::RenderStyle>) 5 0x10d7b3d5e WebCore::RenderScrollbar::updateScrollbarPart(WebCore::ScrollbarPart, bool) 6 0x10d7b3ef1 WebCore::RenderScrollbar::updateScrollbarParts(bool) 7 0x10d7b43aa WebCore::RenderScrollbar::styleChanged() 8 0x10d70786c WebCore::RenderLayer::setHasVerticalScrollbar(bool) 9 0x10d714484 WebCore::RenderLayer::updateScrollbarsAfterStyleChange(WebCore::RenderStyle const*) 10 0x10d7149b1 WebCore::RenderLayer::styleChanged(WebCore::StyleDifference, WebCore::RenderStyle const*) 11 0x10d74e33e WebCore::RenderLayerModelObject::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) 12 0x10d654526 WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) 13 0x10d5ca6d3 WebCore::RenderBlock::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) 14 0x10d78cf39 WebCore::RenderObject::setStyle(WTF::PassRefPtr<WebCore::RenderStyle>) 15 0x10d78c47a WebCore::RenderObject::setAnimatableStyle(WTF::PassRefPtr<WebCore::RenderStyle>) 16 0x10d4fa0d6 WebCore::NodeRenderingContext::createRendererForElementIfNeeded() 17 0x10c89298d WebCore::Element::createRendererIfNeeded() 18 0x10c8929e5 WebCore::Element::attach() 19 0x10cb198bc WebCore::executeTask(WebCore::HTMLConstructionSiteTask&) 20 0x10cb19746 WebCore::HTMLConstructionSite::executeQueuedTasks() 21 0x10cc01075 WebCore::HTMLTreeBuilder::constructTree(WebCore::AtomicHTMLToken*) 22 0x10cb3a191 WebCore::HTMLDocumentParser::constructTreeFromHTMLToken(WebCore::HTMLToken&) 23 0x10cb39949 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) 24 0x10cb39110 WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) 25 0x10cb3a650 WebCore::HTMLDocumentParser::append(WebCore::SegmentedString const&) 26 0x10c644da7 WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, unsigned long) 27 0x10c716963 WebCore::DocumentWriter::addData(char const*, unsigned long) 28 0x10c6dcc87 WebCore::DocumentLoader::commitData(char const*, unsigned long) 29 0x10a012371 WebKit::WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) 30 0x10c6dcd60 WebCore::DocumentLoader::commitLoad(char const*, int) 31 0x10c6dd30b WebCore::DocumentLoader::receivedData(char const*, int) Re-opened since this is blocked by bug 109891 Created attachment 188602 [details]
Patch
Comment on attachment 188602 [details]
Patch
Fixed the ASSERT by moving the check into the if statement. See ChangeLog for more details.
Comment on attachment 188602 [details] Patch Clearing flags on attachment: 188602 Committed r143092: <http://trac.webkit.org/changeset/143092> All reviewed patches have been landed. Closing bug. I'm not happy with this patch, or with the original code that was added here. Can we talk in #webkit on Monday about a better approach? I'm guessing you ended up here because you weren't quite sure how to patch layoutBlock, but RenderBox is way too general a spot for this kind of code (and this code is going to trigger relayouts of children in situations where it wasn't necessary to do so). Reopening this to code a more specialized fix that doesn't do relayouts it shouldn't. Created attachment 188940 [details]
Patch
Created attachment 188941 [details]
Patch
Add a couple of null checks.
Comment on attachment 188941 [details] Patch Attachment 188941 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16610442 New failing tests: fast/regions/seamless-iframe-flowed-into-regions.html Comment on attachment 188941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188941&action=review > Source/WebCore/rendering/RenderBlock.cpp:420 > + // First walk our normal flow objects and see if there is any change. > + for (RenderBox* childBox = firstChildBox(); childBox; childBox = childBox->nextSiblingBox()) { > + if (childBox->isFloatingOrOutOfFlowPositioned()) > + continue; > + childBox->setChildNeedsLayout(true, MarkOnlyThis); > + } I would be good to mention why we don't have to set floating objects dirty per IRC discussion. > Source/WebCore/rendering/RenderBlock.cpp:423 > + // Now walk the positioned objects for which we are the containing block. > + TrackedRendererListHashSet* positionedDescendants = positionedObjects(); It would be great if you could come up with a test case for floating or position objects. > Source/WebCore/rendering/RenderTableRow.cpp:89 > + if (!childBox->isTableCell()) > + continue; On a completely unrelated note, it's weird that we can ever have a non-RenderTableCell RenderObject in RenderTableRow. Created attachment 188954 [details]
Patch with a positioning test case
I added a positioning test case that illustrates what was wrong with the RenderBox code and why you need RenderBlock-specific code to fix this bug the right way.
Comment on attachment 188954 [details] Patch with a positioning test case View in context: https://bugs.webkit.org/attachment.cgi?id=188954&action=review > Source/WebCore/ChangeLog:27 > + Reviewed by NOBODY (OOPS!). This line should appear before the long description but after the bug url. > Source/WebCore/ChangeLog:30 > + Added fast/block/positioning/border-change-relayout-test.html to illustrate why the > + old patch didn't work. Please use the de-factor standard format: Test: fast/block/positioning/border-change-relayout-test.html Fixed in r143284. (In reply to comment #18) > I'm not happy with this patch, or with the original code that was added here. Can we talk in #webkit on Monday about a better approach? I'm guessing you ended up here because you weren't quite sure how to patch layoutBlock, but RenderBox is way too general a spot for this kind of code (and this code is going to trigger relayouts of children in situations where it wasn't necessary to do so). Thanks for fixing! Sorry, I was OOO yesterday due to President's Day. |