Created attachment 138408 [details] Minimal example See attached example. In the current nightly build: The img from the first cell overlap into the second cell.
Created attachment 154859 [details] Patch
Comment on attachment 154859 [details] Patch As non-reviewer review, this looks good to me, and I can confirm this matches the result in other browsers. Julien, what do you think?
Comment on attachment 154859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154859&action=review > Julien, what do you think? This patch will impact any RenderBlocks and not just RenderTableCells. The ChangeLog seems to suggest that this is table cell specific so it should be clarified. > Source/WebCore/rendering/RenderBlock.cpp:5872 > + isPrevChildInlineFlow = false; Interestingly, we don't set isPrevChildInlineFlow in the else branch below which looks suspicious. Ideally you want to update |isPrevChildInlineFlow| for every iteration so it should probably be pushed to the end of the while loop (outside of any branch to garantee that it is updated)
Leviw knows this code better and may have some comments.
Created attachment 165738 [details] Patch
(In reply to comment #3) > (From update of attachment 154859 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154859&action=review > Thanks Julien for reviewing the patch. Have made changes as per your comments. I agree, it would be safer to update |isPrevChildInlineFlow| at the end of the while loop. Have modified the changelog as well. Shall ping Leviw for further reviews. > > Julien, what do you think? > > This patch will impact any RenderBlocks and not just RenderTableCells. The ChangeLog seems to suggest that this is table cell specific so it should be clarified. > > > Source/WebCore/rendering/RenderBlock.cpp:5872 > > + isPrevChildInlineFlow = false; > > Interestingly, we don't set isPrevChildInlineFlow in the else branch below which looks suspicious. Ideally you want to update |isPrevChildInlineFlow| for every iteration so it should probably be pushed to the end of the while loop (outside of any branch to garantee that it is updated)
Updating the title since this isn't table-specific. The fix LGTM. The test case really belongs in fast/block (and not using tables).
Created attachment 166200 [details] Patch
(In reply to comment #7) > Updating the title since this isn't table-specific. The fix LGTM. The test case really belongs in fast/block (and not using tables). Thanks for the review Levi. Have moved the testcase to fast/block and removed the dependency on tables as well.
Comment on attachment 166200 [details] Patch Thank you for the fix! LGTM.
Comment on attachment 166200 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166200&action=review > LayoutTests/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). Whoops... missed this :(
Created attachment 166358 [details] Patch
Comment on attachment 166358 [details] Patch Attachment 166358 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14080214 New failing tests: http/tests/workers/terminate-during-sync-operation.html
(In reply to comment #11) > (From update of attachment 166200 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166200&action=review > > > LayoutTests/ChangeLog:8 > > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > > Whoops... missed this :( Thanks for the review Levi and apologize for missing out on removing that line from the changelog. Have uploaded another patch now, albeit it fails for mac port this time. :( The failing test case though seems to be unrelated to my changes; it verifies web workers.
Comment on attachment 166358 [details] Patch Clearing flags on attachment: 166358 Committed r130057: <http://trac.webkit.org/changeset/130057>
All reviewed patches have been landed. Closing bug.