Created attachment 418307 [details] Testcase The attached testcase shows a table layout bug where resizing the window from wide to narrow (triggering the media query) causes bad table layout; fixed on reload.
Originally seen on https://test262.report/?engines=chakra%2Cjavascriptcore%2Cspidermonkey%2Cv8%2Cxs%2Cqjs%2Cengine262
<rdar://problem/73578735>
test262.report is not loading for me, but my forthcoming patch does fix the attached testcase.
Created attachment 423691 [details] Patch
Created attachment 423693 [details] patch with dependencies for EWS
Created attachment 423779 [details] Patch
Created attachment 423780 [details] patch with dependencies for EWS
Comment on attachment 423779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423779&action=review > Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:842 > + }; > + collapseAndDestroyAnonymousSiblings(); I will let someone capable do the real review, but some style nits: you can merge this into one line as just: }(); I'm not sure why the lambda above, clearFloatsAndOutOfFlowPositionedObjects, doesn't do that, but if you could fix it as well that would be good. > Source/WebCore/rendering/updating/RenderTreeBuilderTable.cpp:250 > +template <typename Parent, typename Child> No space after 'template' here. > Source/WebCore/rendering/updating/RenderTreeBuilderTable.h:59 > + template <typename Parent, typename Child> No space after 'template' here.
(In reply to Sam Weinig from comment #8) > > Source/WebCore/rendering/updating/RenderTreeBuilderTable.cpp:250 > > +template <typename Parent, typename Child> > > No space after 'template' here. No space wins (only just!) in the codebase: $ git grep 'template <' | wc -l ; git grep 'template<' | wc -l 13013 13057 but it's not what .clang-format says: $ grep SpaceAfterTemplate .clang-format SpaceAfterTemplateKeyword: true
(In reply to Sam Weinig from comment #8) > > Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:842 > > + }; > > + collapseAndDestroyAnonymousSiblings(); > > I will let someone capable do the real review, but some style nits: > > you can merge this into one line as just: > > }(); > > I'm not sure why the lambda above, clearFloatsAndOutOfFlowPositionedObjects, > doesn't do that, but if you could fix it as well that would be good. I think there's some advantage to naming the lambdas here, to make it clear what they're doing, although perhaps it's obvious from their contents. I only found two other instances of immediately executed lambda expressions where the return type is void, in CSSPropertyParseHelpers, and for those it's pretty clear what's going on. If we're getting rid of their names, then clearFloatsAndOutOfFlowPositionedObjects could simply become an if statement. collapseAndDestroyAnonymousSiblings could too, I guess, but I slightly like the more spaced out: if (of this type) { do this; return; } if (of that type) { do that; return; } than: if (of this type) { do this; } else if (of that type) { do that; }
I tend to use this pattern. I don't have a strong preference over "auto foo = [&] { }();" I find it more readable but :shrugs: >If we're getting rid of their names, then clearFloatsAndOutOfFlowPositionedObjects I would not do that as their names act as "sticky" comments and they help code readability.
(In reply to zalan from comment #11) > I tend to use this pattern. I don't have a strong preference over "auto foo > = [&] { }();" I find it more readable but :shrugs: > > >If we're getting rid of their names, then clearFloatsAndOutOfFlowPositionedObjects > I would not do that as their names act as "sticky" comments and they help > code readability. My bad, I didn't realize this was idiomatic. Please, ignore.
Created attachment 423936 [details] Patch
Committed r275059: <https://commits.webkit.org/r275059> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423936 [details].