Bug 89670 – Resolve CSS Exclusions shapeInside, shapeOutside properties to Length based WrapShape classes

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Hans Muller]

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.

[Bear Travis]

Created attachment 150042 [details]
Initial patch with length-based WrapShapes

[Simon Fraser (smfr)]

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.

[Bear Travis]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Bear Travis]

Created attachment 150360 [details]
Updated patch

Reuploading the patch and rerunning tests

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Bear Travis]

Created attachment 151290 [details]
Updated patch

Updating the patch and reuploading. The tests appear to be failing without the patch as well.

[Simon Fraser (smfr)]

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?

[Bear Travis]

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.

[Early Warning System Bot]

Comment on attachment 151555 [details]
Updated Patch

Attachment 151555 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13200111

[Early Warning System Bot]

Comment on attachment 151555 [details]
Updated Patch

Attachment 151555 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13198149

[Gyuyoung Kim]

Comment on attachment 151555 [details]
Updated Patch

Attachment 151555 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13207110

[WebKit Review Bot]

Comment on attachment 151555 [details]
Updated Patch

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

[Bear Travis]

Created attachment 151582 [details]
Build system updates

Build system updates for the added files

[Bear Travis]

Created attachment 151604 [details]
Updating changelogs

Updating comments in the changelogs to be more descriptive

[Antti Koivisto]

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.

[Bear Travis]

Created attachment 151742 [details]
Updating changelog

Making changelog updates, added bug 90998 to track virtualization cleanup

[Bear Travis]

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.

[Bear Travis]

Created attachment 152319 [details]
Updated patch

Adding WrapShapeFunctions.cpp to xcode project sources

[Dirk Schulze]

Comment on attachment 152319 [details]
Updated patch

Anttik already gave his r+. So rs=me.

[WebKit Review Bot]

Comment on attachment 152319 [details]
Updated patch

Clearing flags on attachment: 152319

Committed r122773: <http://trac.webkit.org/changeset/122773>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.