Bug 64689 – [CSSRegions]Add basic RenderRegion support

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Mihnea Ovidenie]

RenderRegion is the rendering class for the region element and it will be used to render the content of RenderFlowThread (see bug 64516).

[Mihnea Ovidenie]

Created attachment 101320 [details]
Patch

[WebKit Review Bot]

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.

[Mihnea Ovidenie]

Created attachment 101321 [details]
Patch 2

Fix style.

[Dave Hyatt]

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?

[Mihnea Ovidenie]

Created attachment 101977 [details]
Patch 3

[Mihnea Ovidenie]

(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.

[Dave Hyatt]

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.

[WebKit Review Bot]

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

[Mihnea Ovidenie]

Created attachment 102231 [details]
Patch 4

Rework the patch after RenderFlowThread patch got in.

[Dave Hyatt]

Comment on attachment 102231 [details]
Patch 4

r=me

[WebKit Review Bot]

Comment on attachment 102231 [details]
Patch 4

Clearing flags on attachment: 102231

Committed r91955: <http://trac.webkit.org/changeset/91955>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.