Bug 89670

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.