The values of the shapeInside and shapeOutside CSS properties should be defined in terms of Length. Currently they're defined with CSSWrapShape objects, which just contain the specified CSS values. Define new Length based analogs to CSSWrapShape and have StyleBuilder resolve shapeInside and shapeOutside to the new classes.
Created attachment 150042 [details] Initial patch with length-based WrapShapes
Comment on attachment 150042 [details] Initial patch with length-based WrapShapes View in context: https://bugs.webkit.org/attachment.cgi?id=150042&action=review > Source/WebCore/ChangeLog:12 > + Covered by existing tests in LayoutTests/fast/exclusions It looks like you're adding a lot of new functionality here. I don't see how it can be covered by existing tests. For example, you need tests with various length units, including viewport-relative lengths. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:759 > + case WrapShape::WRAP_SHAPE_RECTANGLE: > + { Brace should be on the previous line. Same with the case statements below. > Source/WebCore/css/CSSWrapShapes.h:154 > PassRefPtr<CSSPrimitiveValue> getXAt(unsigned i) { return m_values.at(i * 2); } > PassRefPtr<CSSPrimitiveValue> getYAt(unsigned i) { return m_values.at(i * 2 + 1); } > + const Vector<RefPtr<CSSPrimitiveValue> >& values() { return m_values; } > These should all be |const|. > Source/WebCore/css/StyleBuilder.cpp:1742 > + RefPtr<WrapShape> wrapShape = 0; No need to initialize to 0. > Source/WebCore/css/StyleBuilder.cpp:1759 > + wrapShape = rect; You have an extra ref/deref here.You can say rect.release() to transfer ownership. > Source/WebCore/rendering/style/WrapShapes.h:77 > + : m_radiusX(Undefined) > + , m_radiusY(Undefined) These should be indented. > Source/WebCore/rendering/style/WrapShapes.h:93 > + Length left() const { return m_left; } > + Length top() const { return m_top; } Seems odd to denote a circle by its top/left position, rather than the center.
Created attachment 150284 [details] Updated patch Made the suggested changes, with the exception of changing circle's coordinates (bug 89669). Added a test to cover the various length units. The shape variations are tested in parsing-wrap-shape-inside/outside.
Comment on attachment 150284 [details] Updated patch Attachment 150284 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13108609 New failing tests: svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html
Created attachment 150299 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 150360 [details] Updated patch Reuploading the patch and rerunning tests
Comment on attachment 150360 [details] Updated patch Attachment 150360 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13124114 New failing tests: fast/loader/loadInProgress.html
Created attachment 150380 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 151290 [details] Updated patch Updating the patch and reuploading. The tests appear to be failing without the patch as well.
Comment on attachment 151290 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=151290&action=review > Source/WebCore/css/StyleBuilder.cpp:1761 > + CSSWrapShapeRectangle* rectValue = static_cast<CSSWrapShapeRectangle *>(wrapShapeValue); > + RefPtr<WrapShapeRectangle> rect = WrapShapeRectangle::create(); > + > + rect->setLeft(convertToLength(styleResolver, rectValue->left())); > + rect->setTop(convertToLength(styleResolver, rectValue->top())); > + rect->setWidth(convertToLength(styleResolver, rectValue->width())); > + rect->setHeight(convertToLength(styleResolver, rectValue->height())); It's odd to have these WrapShape <-> CSSWrapShape* conversions scattered between two different files. Maybe add a CSSWrapShapeValues file and put all the code in there. > Source/WebCore/rendering/style/WrapShapes.h:63 > + Length radiusX() const { return m_radiusX; } > + Length radiusY() const { return m_radiusY; } Should be cornerRadius I think. > Source/WebCore/rendering/style/WrapShapes.h:85 > + Length m_radiusX; > + Length m_radiusY; I think m_cornerRadiusX/Y would be better. > LayoutTests/fast/exclusions/parsing-wrap-shape-lengths.html:42 > +testInner("-webkit-shape-inside", "circle(-1.5px, +1.5px, 1.5px)", "circle(-1.5px, 1.5px, 1.5px)"); > +testInner("-webkit-shape-inside", "circle(-.5px, +.5px, .5px)", "circle(-0.5px, 0.5px, 0.5px)"); Do you have tests that test bad values too?
Created attachment 151555 [details] Updated Patch Moved WrapShape <-> CSSWrapShape conversions to WrapShapeFunctions .h/.cpp files. Changed radiusX/Y to cornerRadiusX/Y. Added some non-valid length tests. The specification does not yet restrict the lengths (eg, negative corner radii), so not yet testing those.
Comment on attachment 151555 [details] Updated Patch Attachment 151555 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13200111
Comment on attachment 151555 [details] Updated Patch Attachment 151555 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13198149
Comment on attachment 151555 [details] Updated Patch Attachment 151555 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13207110
Comment on attachment 151555 [details] Updated Patch Attachment 151555 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13205149
Created attachment 151582 [details] Build system updates Build system updates for the added files
Created attachment 151604 [details] Updating changelogs Updating comments in the changelogs to be more descriptive
Comment on attachment 151604 [details] Updating changelogs View in context: https://bugs.webkit.org/attachment.cgi?id=151604&action=review > Source/WebCore/ChangeLog:14 > + - Modifying RenderStyle to use length based WrapShape classes, > + rather than the CSSValue CSSWrapShape classes. > + - The translation between WrapShape and CSSWrapShape classes > + is handled by helper functions in the new WrapShapeFunctions files. > + - StyleBuilder resolves CSSWrapShapes to WrapShapes for layout use. > + - CSSComputedStyleDeclaration translates WrapShapes to CSSWrapShapes > + for style use. You should explain why. > Source/WebCore/rendering/style/WrapShapes.h:52 > +class WrapShape : public RefCounted<WrapShape> { > +public: > + enum Type { > + WRAP_SHAPE_RECTANGLE = 1, > + WRAP_SHAPE_CIRCLE = 2, > + WRAP_SHAPE_ELLIPSE = 3, > + WRAP_SHAPE_POLYGON = 4 > + }; > + > + virtual Type type() const = 0; > + virtual ~WrapShape() { } > + > +protected: > + WrapShape() { } > +}; This doesn't really need the vptr. You could make the type a field in the base and cast to subclass in destructor. This would save some memory (on 64bit) and avoid the virtual calls. Not a big deal though.
Created attachment 151742 [details] Updating changelog Making changelog updates, added bug 90998 to track virtualization cleanup
anttik / smfr, could one of you review this when you get the chance? I think it's getting close and there are a couple other patches in the works depending on it.
Created attachment 152319 [details] Updated patch Adding WrapShapeFunctions.cpp to xcode project sources
Comment on attachment 152319 [details] Updated patch Anttik already gave his r+. So rs=me.
Comment on attachment 152319 [details] Updated patch Clearing flags on attachment: 152319 Committed r122773: <http://trac.webkit.org/changeset/122773>
All reviewed patches have been landed. Closing bug.