Bug 64689 - [CSSRegions]Add basic RenderRegion support
Summary: [CSSRegions]Add basic RenderRegion support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 57312
  Show dependency treegraph
 
Reported: 2011-07-18 05:40 PDT by Mihnea Ovidenie
Modified: 2011-07-28 16:33 PDT (History)
2 users (show)

See Also:


Attachments
Patch (18.19 KB, patch)
2011-07-19 08:03 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 2 (18.20 KB, patch)
2011-07-19 08:11 PDT, Mihnea Ovidenie
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Patch 3 (25.59 KB, patch)
2011-07-26 00:58 PDT, Mihnea Ovidenie
hyatt: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch 4 (25.83 KB, patch)
2011-07-27 23:50 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihnea Ovidenie 2011-07-18 05:40:59 PDT
RenderRegion is the rendering class for the region element and it will be used to render the content of RenderFlowThread (see bug 64516).
Comment 1 Mihnea Ovidenie 2011-07-19 08:03:15 PDT
Created attachment 101320 [details]
Patch
Comment 2 WebKit Review Bot 2011-07-19 08:06:33 PDT
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.
Comment 3 Mihnea Ovidenie 2011-07-19 08:11:27 PDT
Created attachment 101321 [details]
Patch 2

Fix style.
Comment 4 Dave Hyatt 2011-07-20 11:17:59 PDT
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?
Comment 5 Mihnea Ovidenie 2011-07-26 00:58:54 PDT
Created attachment 101977 [details]
Patch 3
Comment 6 Mihnea Ovidenie 2011-07-26 01:01:38 PDT
(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 7 Dave Hyatt 2011-07-27 09:19:15 PDT
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 8 WebKit Review Bot 2011-07-27 09:30:10 PDT
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
Comment 9 Mihnea Ovidenie 2011-07-27 23:50:16 PDT
Created attachment 102231 [details]
Patch 4

Rework the patch after RenderFlowThread patch got in.
Comment 10 Dave Hyatt 2011-07-28 11:41:53 PDT
Comment on attachment 102231 [details]
Patch 4

r=me
Comment 11 WebKit Review Bot 2011-07-28 16:33:52 PDT
Comment on attachment 102231 [details]
Patch 4

Clearing flags on attachment: 102231

Committed r91955: <http://trac.webkit.org/changeset/91955>
Comment 12 WebKit Review Bot 2011-07-28 16:33:57 PDT
All reviewed patches have been landed.  Closing bug.