Bug 92164 - [CSS Shapes] Shapes should not cause relayout when they are equivalent
Summary: [CSS Shapes] Shapes should not cause relayout when they are equivalent
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bear Travis
URL:
Keywords:
: 100811 (view as bug list)
Depends on:
Blocks: 89256
  Show dependency treegraph
 
Reported: 2012-07-24 14:36 PDT by Bear Travis
Modified: 2023-08-01 11:38 PDT (History)
9 users (show)

See Also:


Attachments
Initial patch (6.08 KB, patch)
2013-06-14 16:10 PDT, Bear Travis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bear Travis 2012-07-24 14:36:17 PDT
RenderStyle::diff should not cause relayout and RenderBlock::styleDidChange should not generate a new WrapShapeInfo for two different but equivalent WrapShapes
Currently, just pointers are used for comparison, but eventually they should use a deep compare
Comment 1 Bem Jones-Bey 2013-02-12 14:58:08 PST
This also affects shape-outside, not just shape-inside.
Comment 2 Nico Weber 2013-02-12 15:07:30 PST
*** Bug 100811 has been marked as a duplicate of this bug. ***
Comment 3 Bear Travis 2013-06-14 16:10:43 PDT
Created attachment 204744 [details]
Initial patch
Comment 4 Sam Weinig 2013-06-15 20:23:37 PDT
Comment on attachment 204744 [details]
Initial patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204744&action=review

> Source/WebCore/ChangeLog:11
> +        The two layouts are equivalent, so there are no new tests.

If this is being done for performance, do you have any perf results that this made better? Is there a benchmark you are using or a test case you are investigating?

> Source/WebCore/rendering/style/BasicShapes.h:98
> +    virtual bool operator==(const BasicShape& other)

You should add OVERRIDE here (and on all the others below).  Given that this is a virtual function, does this really need to be inline?

> Source/WebCore/rendering/style/BasicShapes.h:211
> +        for (size_t i = 0; i < values().size(); i++)
> +            if (getXAt(i) != otherPolygon->getXAt(i) || getYAt(i) != otherPolygon->getYAt(i))
> +                return false;

The for-loop needs braces.
Comment 5 Zoltan Horvath 2014-02-07 10:15:00 PST
Comment on attachment 204744 [details]
Initial patch

Pulling this out from the review queue, since it's need to be updated. (Changelog: [CSS Exclusions])
Comment 6 Zoltan Horvath 2014-02-07 10:16:04 PST
Closing bug until we have a perf test to verify the improvement.
Comment 7 Ahmad Saleem 2023-08-01 11:38:09 PDT
Just to add we do have FIXME still in our code referring to bug, which is marked duplicate of this:

https://searchfox.org/wubkat/source/Source/WebCore/rendering/RenderBox.cpp#447

// FIXME: A future optimization would do a deep comparison for equality. (bug 100811)
    if (shapeOutside == oldShapeOutside && shapeMargin == oldShapeMargin && shapeImageThreshold == oldShapeImageThreshold)
        return;