Bug 64689

Created attachment 101320 [details] Patch

Attachment 101320 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/RenderObject.cpp:59: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 101321 [details] Patch 2 Fix style.

Comment on attachment 101321 [details] Patch 2 It's pretty hard for me to evaluate this patch without understanding a bit more about what RenderRegion is going to be doing. At first glance it looks pretty odd to me that RenderRegion is a replaced element. I don't quite understand that. I was also a bit surprised to see a RenderRegion created if regionThread is set in the style. How does this interact with different display types? It seems like the display type is being ignored?

Created attachment 101977 [details] Patch 3

(In reply to comment #4) > (From update of attachment 101321 [details]) > It's pretty hard for me to evaluate this patch without understanding a bit more about what RenderRegion is going to be doing. At first glance it looks pretty odd to me that RenderRegion is a replaced element. I don't quite understand that. > > I was also a bit surprised to see a RenderRegion created if regionThread is set in the style. How does this interact with different display types? It seems like the display type is being ignored? I have added basic layout and painting code. The RenderRegion object does not take its content from RenderFlowThread yet, it will do that in the next forthcoming patches. We use now RenderBox as a base class for RenderRegion. When creating a RenderRegion, a filtering is made so that only certain display type elements can become regions.

Comment on attachment 101977 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=101977&action=review r=me > Source/WebCore/rendering/RenderRegion.cpp:59 > +void RenderRegion::layout() > +{ > + ASSERT(needsLayout()); > + > + computeLogicalWidth(); > + computeLogicalHeight(); > + > + setNeedsLayout(false); > +} This is interesting. I wonder if there should be an intrinsic size for regions or not. I know this is being debated in the CSS WG. I suggested they have no intrinsic size (and that is what you've done here), but I know others would like them to maybe be more like iframes or objects (300x150 intrinsic size). I guess for now we can just stick with 0x0.

Comment on attachment 101977 [details] Patch 3 Rejecting attachment 101977 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2 Last 500 characters of output: ebCore/rendering/RenderObject.h Hunk #1 succeeded at 431 (offset 4 lines). patching file Source/WebCore/rendering/RenderRegion.cpp patching file Source/WebCore/rendering/RenderRegion.h patching file Source/WebCore/rendering/style/RenderStyle.cpp Hunk #1 FAILED at 350. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/rendering/style/RenderStyle.cpp.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'David Hyatt', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/9250800

Created attachment 102231 [details] Patch 4 Rework the patch after RenderFlowThread patch got in.

Comment on attachment 102231 [details] Patch 4 r=me

Comment on attachment 102231 [details] Patch 4 Clearing flags on attachment: 102231 Committed r91955: <http://trac.webkit.org/changeset/91955>

All reviewed patches have been landed. Closing bug.