Change RenderBox to represent margins as a box instead of using a separate value for each edge.
Created attachment 142886 [details] Patch
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
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 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.
(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.
Created attachment 142913 [details] Patch
Created attachment 142916 [details] Patch
Marked top/right/bottom/left as inline and changed the writing mode aware methods to take a const style pointer.
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 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 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 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.
(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.
(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.
Created attachment 143364 [details] Patch for landing
Renamed the new class to FractionalLayoutBoxExtent as suggested by Julien and added the OVERRIDE keyword as needed.
Comment on attachment 143364 [details] Patch for landing Clearing flags on attachment: 143364 Committed r118076: <http://trac.webkit.org/changeset/118076>
All reviewed patches have been landed. Closing bug.