Bug 37688

Created attachment 53498 [details] Patch

+cc people who have added mucked with this code or expressed interest. I am planning to try to make lazy attachment the only form of attachment, with special treatment for forms/plugins/frames. Even if that doesn't pan out this is still a good step forward - less parameters + less branching = less complexity + more gooder.

Comment on attachment 53498 [details] Patch > diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog > index c5e227e..24e1ee6 100644 > --- a/LayoutTests/ChangeLog > +++ b/LayoutTests/ChangeLog > @@ -1,3 +1,24 @@ > +2010-04-15 James Robinson <jamesr@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Update baselines affected by lazier attachment > + https://bugs.webkit.org/show_bug.cgi?id=37688 > + > + There are no user-visible changes, just a slightly different set of > + 0-sized render objects in the render tree due to style recalcs/layouts > + happening at slightly different times. > + > + * platform/mac/editing/inserting/break-blockquote-after-delete-expected.txt: > + * platform/mac/fast/forms/basic-buttons-expected.txt: > + * platform/mac/fast/forms/formmove3-expected.txt: > + * platform/mac/fast/forms/preserveFormDuringResidualStyle-expected.txt: > + * platform/mac/fast/invalid/003-expected.txt: > + * platform/mac/fast/invalid/table-residual-style-crash-expected.txt: > + * platform/mac/fast/table/014-expected.txt: > + * platform/mac/tables/mozilla/bugs/bug647-expected.txt: > + * platform/mac/tables/mozilla/other/wa_table_tr_align-expected.txt: You should go ahead and add results for Chromium as well -- or at least update test_expectations.txt. > - > - // Should assert isBlockFlow || isInlineFlow when deletion improves. See 4244964. > - ASSERT(container->renderer()); Can you just remove these asserts? What were they for in the first place?

Created attachment 55166 [details] Patch

I've marked the affected Chromium tests as TEXT PASS in chromium's text_expectations.txt. I will rebase as needed after landing. It is safe to remove those ASSERT()s. They date from before there was a Node::lazyAttach() and when all nodes in the DOM would get a renderer on creation. The issue cited in the comment doesn't seem to be an issue any more.

I don't understand the layout test results updates. Seems this would be easier to understand if we had the minimal code change to cause said updates. Like just flipping the default from false to true? Or by changing one or two callers? Do we really understand the test output changes and are they expected?

The layout test results changes are all noise due to leading/trailing whitespace in blocks. I'll see if I can somehow isolate those effects from the meat of this patch.

Comment on attachment 55166 [details] Patch Clearing r?, needs a bit of work.

These two patches changed and did similar changes: https://github.com/WebKit/WebKit/commit/795565b984974c6bd771a7fc35099c45e3b6e37e https://github.com/WebKit/WebKit/commit/19da37c26771f4be4bae77683f40fb2b8396d64c -> rniwa@webkit.org - Is this patch needed? Thanks!

We wrote this code so this bug is no longer relevant.