Bug 86952 - Represent margins as box and simplify writing mode logic
Summary: Represent margins as box and simplify writing mode logic
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: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-19 17:50 PDT by Emil A Eklund
Modified: 2012-05-22 16:05 PDT (History)
8 users (show)

See Also:


Attachments
Patch (39.66 KB, patch)
2012-05-19 17:55 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (672.72 KB, application/zip)
2012-05-19 19:03 PDT, WebKit Review Bot
no flags Details
Patch (34.97 KB, patch)
2012-05-20 12:41 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (35.18 KB, patch)
2012-05-20 14:01 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch for landing (37.11 KB, patch)
2012-05-22 14:32 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2012-05-19 17:50:22 PDT
Change RenderBox to represent margins as a box instead of using a separate value for each edge.
Comment 1 Emil A Eklund 2012-05-19 17:55:51 PDT
Created attachment 142886 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-19 19:03:20 PDT
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
Comment 3 WebKit Review Bot 2012-05-19 19:03:25 PDT
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
Comment 4 Eric Seidel (no email) 2012-05-19 19:48:00 PDT
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.
Comment 5 Emil A Eklund 2012-05-19 21:46:24 PDT
(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.
Comment 6 Emil A Eklund 2012-05-20 12:41:29 PDT
Created attachment 142913 [details]
Patch
Comment 7 Emil A Eklund 2012-05-20 14:01:01 PDT
Created attachment 142916 [details]
Patch
Comment 8 Emil A Eklund 2012-05-20 14:01:39 PDT
Marked top/right/bottom/left as inline and changed the writing mode aware methods to take a const style pointer.
Comment 9 Julien Chaffraix 2012-05-21 13:43:24 PDT
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).
Comment 10 Emil A Eklund 2012-05-21 13:48:19 PDT
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.
Comment 11 Eric Seidel (no email) 2012-05-21 16:04:43 PDT
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 :)
Comment 12 Julien Chaffraix 2012-05-21 16:24:12 PDT
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.
Comment 13 Emil A Eklund 2012-05-21 16:28:56 PDT
(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.
Comment 14 Julien Chaffraix 2012-05-21 16:37:47 PDT
(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.
Comment 15 Emil A Eklund 2012-05-22 14:32:05 PDT
Created attachment 143364 [details]
Patch for landing
Comment 16 Emil A Eklund 2012-05-22 14:34:36 PDT
Renamed the new class to FractionalLayoutBoxExtent as suggested by Julien and added the OVERRIDE keyword as needed.
Comment 17 WebKit Review Bot 2012-05-22 16:04:49 PDT
Comment on attachment 143364 [details]
Patch for landing

Clearing flags on attachment: 143364

Committed r118076: <http://trac.webkit.org/changeset/118076>
Comment 18 WebKit Review Bot 2012-05-22 16:05:07 PDT
All reviewed patches have been landed.  Closing bug.