Summary: | Resolve CSS Exclusions shapeInside, shapeOutside properties to Length based WrapShape classes | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hans Muller <giles_joplin> | ||||||||||||||||||||||||
Component: | CSS | Assignee: | Bear Travis <betravis> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | betravis, cmarcelo, dglazkov, eric, koivisto, macpherson, menard, rakuco, simon.fraser, webkit.review.bot | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||
Bug Blocks: | 89259, 89671, 90998 | ||||||||||||||||||||||||||
Attachments: |
|
Description
Hans Muller
2012-06-21 09:32:04 PDT
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. |