Bug 86952 – Represent margins as box and simplify writing mode logic

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

[Emil A Eklund]

Change RenderBox to represent margins as a box instead of using a separate value for each edge.

[Emil A Eklund]

Created attachment 142886 [details]
Patch

[WebKit Review Bot]

Comment on attachment 142886 [details]
Patch

Attachment 142886 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12734260

New failing tests:
css3/flexbox/child-overflow.html

[WebKit Review Bot]

Created attachment 142889 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Seidel (no email)]

Comment on attachment 142886 [details]
Patch

This patch is amazing.  Do we have any clue about potential perf impact?  We're turning some direct-access into function calls, which could be fine, but might be hot.

[Emil A Eklund]

(In reply to comment #4)
> (From update of attachment 142886 [details])
> This patch is amazing.  Do we have any clue about potential perf impact?  We're turning some direct-access into function calls, which could be fine, but might be hot.

No, none of the webkit perf tests showed any difference at all. The only way to know for sure is to wait for the PLT data.

Most of the function calls should be inlined and optimized away. I suppose I could add the inline compiler hint.

[Emil A Eklund]

Created attachment 142913 [details]
Patch

[Emil A Eklund]

Created attachment 142916 [details]
Patch

[Emil A Eklund]

Marked top/right/bottom/left as inline and changed the writing mode aware methods to take a const style pointer.

[Julien Chaffraix]

Comment on attachment 142916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142916&action=review

This is a super cool change. This class should enable more simplifications as we switch also borders, paddings and maybe even model outline as a special overflow.

> Source/WebCore/platform/graphics/FractionalLayoutBox.h:40
> +class FractionalLayoutBox {

This naming is not right as this is not a box but an outset around a box. I can't recall the mathematical name for such a shape but here are some ideas for a better name: FractionalLayoutOutset, FractionalLayoutBoxExtend, FractionalLayoutAureole, FractionalLayoutCrown.

None of those are 100% satisfactory to me but at least, they don't pretend to be a box when they are not.

> Source/WebCore/rendering/RenderBox.h:239
> +    virtual LayoutUnit marginBefore() const { return m_marginBox.before(style()); }
> +    virtual LayoutUnit marginAfter() const { return m_marginBox.after(style()); }
> +    virtual LayoutUnit marginStart() const { return m_marginBox.start(style()); }
> +    virtual LayoutUnit marginEnd() const { return m_marginBox.end(style()); }

Please annotate any virtual method with OVERRIDE when not already done (not repeated).

[Emil A Eklund]

Comment on attachment 142916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142916&action=review

>> Source/WebCore/platform/graphics/FractionalLayoutBox.h:40
>> +class FractionalLayoutBox {
> 
> This naming is not right as this is not a box but an outset around a box. I can't recall the mathematical name for such a shape but here are some ideas for a better name: FractionalLayoutOutset, FractionalLayoutBoxExtend, FractionalLayoutAureole, FractionalLayoutCrown.
> 
> None of those are 100% satisfactory to me but at least, they don't pretend to be a box when they are not.

I'm just trying to follow our existing naming convention, we already have LengthBox that serves a similar role. Furthermore other toolkits tends to use Box to describe this kind of class.

> Source/WebCore/rendering/RenderBox.h:240
> +

Will do.

[Eric Seidel (no email)]

Comment on attachment 142916 [details]
Patch

Let's see what happens!  IT's a huge style win, hopefully not a perf hit.  If it is, we'll make it faste.r :)

[Julien Chaffraix]

Comment on attachment 142916 [details]
Patch

> I'm just trying to follow our existing naming convention, we already have LengthBox that serves a similar role. Furthermore other toolkits tends to use Box to describe this kind of class.

That argument doesn't cut: the class you describe seems to be used for both a box and an outline around a box. As far as the other toolkit do, I don't think it's a good idea to pick an imprecise name to match them. I would rather see the name improved than land as-is but I won't block the patch on that.

However I think the override changes *do* warrant an improved patch.

[Emil A Eklund]

(In reply to comment #12)
> (From update of attachment 142916 [details])
> > I'm just trying to follow our existing naming convention, we already have LengthBox that serves a similar role. Furthermore other toolkits tends to use Box to describe this kind of class.
> 
> That argument doesn't cut: the class you describe seems to be used for both a box and an outline around a box. As far as the other toolkit do, I don't think it's a good idea to pick an imprecise name to match them. I would rather see the name improved than land as-is but I won't block the patch on that.

I'll gladly rename it if we can come up with a better name. How about FractionalLayoutBoxExtend?

> However I think the override changes *do* warrant an improved patch.

Of course, I'll fix that before landing.

[Julien Chaffraix]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 142916 [details] [details])
> > > I'm just trying to follow our existing naming convention, we already have LengthBox that serves a similar role. Furthermore other toolkits tends to use Box to describe this kind of class.
> > 
> > That argument doesn't cut: the class you describe seems to be used for both a box and an outline around a box. As far as the other toolkit do, I don't think it's a good idea to pick an imprecise name to match them. I would rather see the name improved than land as-is but I won't block the patch on that.
> 
> I'll gladly rename it if we can come up with a better name. How about FractionalLayoutBoxExtend?

Should it be extend or extent? (writing mode uses extent when referring to the dimension so it would seem I misspelled it originally).

Regardless of the exact spelling, I am totally fine with that name but note that I can't assess if this name makes total sense for a native speaker.

[Emil A Eklund]

Created attachment 143364 [details]
Patch for landing

[Emil A Eklund]

Renamed the new class to FractionalLayoutBoxExtent as suggested by Julien and added the OVERRIDE keyword as needed.

[WebKit Review Bot]

Comment on attachment 143364 [details]
Patch for landing

Clearing flags on attachment: 143364

Committed r118076: <http://trac.webkit.org/changeset/118076>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.