Bug 51664 – Refactoring: Extract RoundedIntRect class

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Hajime Morrita]

a quad of IntSize is often used to represent the radii of a rect. 
Extracting it to the class will simplify the code.

[Hajime Morrita]

Created attachment 77532 [details]
Patch

[WebKit Review Bot]

Attachment 77532 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7211223

[Hajime Morrita]

Created attachment 77534 [details]
try to fix chromium buid

[Hajime Morrita]

Created attachment 77962 [details]
updates to ToT

[Hajime Morrita]

Created attachment 78511 [details]
Patch

[Hajime Morrita]

Hi Simon, could you take a look?
I'd like to land this before other box-shadow fixes.

[Simon Fraser (smfr)]

Comment on attachment 78511 [details]
Patch

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

> Source/WebCore/platform/graphics/IntRectRadii.cpp:43
> +void IntRectRadii::constrainFor(const IntRect& rect)
> +{
> +    // Constrain corner radii using CSS3 rules:
> +    // http://www.w3.org/TR/css3-background/#the-border-radius

I'm not sure that CSS-specific behavior belongs in a utility class like this. Is this just one possible way of constraining the radii?

> Source/WebCore/platform/graphics/IntRectRadii.h:38
> +class IntRectRadii {
> +public:
> +    IntRectRadii() {}

I wonder if it might not be better to have a RoundedIntRect that includes the rect and the radii. Maybe it could inherit from IntRect?

> Source/WebCore/rendering/RenderObject.cpp:1030
> +        graphicsContext->addRoundedRectClip(halfBorderRect, 
> +                                            style->getInnerBorderRadiiForRectWithBorderWidths(halfBorderRect, 
> +                                                                                              style->borderLeftWidth() / 2, 
> +                                                                                              style->borderBottomWidth() / 2, 
> +                                                                                              style->borderLeftWidth() / 2, 
> +                                                                                              style->borderRightWidth() / 2));

We don't normally indent like this.

[Hajime Morrita]

Created attachment 78918 [details]
Patch

[Hajime Morrita]

Hi Simon, thank you for reviewing and I'm sorry for my slow response.
I updated the patch to address the feedback. Could you take another look?

(In reply to comment #7)
> (From update of attachment 78511 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78511&action=review
> 
> > Source/WebCore/platform/graphics/IntRectRadii.cpp:43
> > +void IntRectRadii::constrainFor(const IntRect& rect)
> > +{
> > +    // Constrain corner radii using CSS3 rules:
> > +    // http://www.w3.org/TR/css3-background/#the-border-radius
> 
> I'm not sure that CSS-specific behavior belongs in a utility class like this. Is this just one possible way of constraining the radii?
Sure. Moved to RenderStyle.cpp

> 
> > Source/WebCore/platform/graphics/IntRectRadii.h:38
> > +class IntRectRadii {
> > +public:
> > +    IntRectRadii() {}
> 
> I wonder if it might not be better to have a RoundedIntRect that includes the rect and the radii. Maybe it could inherit from IntRect?
It makes sense. I extracted RoundedIntRect. The radii are also grouped int RoundedIntRect::Radii
class because there are some cases that handle radii individually.

> 
> > Source/WebCore/rendering/RenderObject.cpp:1030
> > +        graphicsContext->addRoundedRectClip(halfBorderRect, 
> > +                                            style->getInnerBorderRadiiForRectWithBorderWidths(halfBorderRect, 
> > +                                                                                              style->borderLeftWidth() / 2, 
> > +                                                                                              style->borderBottomWidth() / 2, 
> > +                                                                                              style->borderLeftWidth() / 2, 
> > +                                                                                              style->borderRightWidth() / 2));
> 
> We don't normally indent like this.
Fixed.

[WebKit Review Bot]

Attachment 78918 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7593027

[Build Bot]

Attachment 78918 [details] did not build on win:
Build output: http://queues.webkit.org/results/7523040

[Hajime Morrita]

Created attachment 79127 [details]
trying to fix the build break

[Hajime Morrita]

Simon, if you have time, would you be able to take a look again?

[Simon Fraser (smfr)]

Comment on attachment 79127 [details]
trying to fix the build break

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

> Source/WebCore/platform/graphics/RoundedIntRect.h:48
> +        Radii(const IntSize& topLeft, const IntSize& topRight, const IntSize& bottomLeft, const IntSize& bottomRight)
> +            : m_topLeft(topLeft)
> +            , m_topRight(topRight)
> +            , m_bottomLeft(bottomLeft)
> +            , m_bottomRight(bottomRight)
> +        {
> +        }
> +
> +        void setTopLeft(const IntSize& size) { m_topLeft = size; }

I think you should include x(), y(), width(), height(), right() and bottom() here just as IntRect has.

[Hajime Morrita]

Committed r76004: <http://trac.webkit.org/changeset/76004>

[Csaba Osztrogonác]

(In reply to comment #15)
> Committed r76004: <http://trac.webkit.org/changeset/76004>

It broke the build on all GTK bots. It seems you missed
adding the following files to the GTK buildsystem:

platform/graphics/RoundedIntRect.cpp
platform/graphics/RoundedIntRect.h

[Hajime Morrita]

Committed r76016: <http://trac.webkit.org/changeset/76016>

[Hajime Morrita]

(In reply to comment #17)
> Committed r76016: <http://trac.webkit.org/changeset/76016>
This commit rolled the change.

[Hajime Morrita]

Rolled at http://trac.webkit.org/changeset/76016 due to pixel test failure

[Hajime Morrita]

Created attachment 79270 [details]
Patch

[Hajime Morrita]

Hi Simon, I found pixel test failures and other redness on the buildbot so rolled the last patch out.
Now I uploaded revised one. Would you able to take it again?

This addresses following isssues:

- Added missing file for WebCore.vcproj and GNUMakefile.am
- Added missing null check (which caused a crash on some release builds) in paintBoxShadow().
   Extracting includeLogicalEdges() causes null-access.
- Shadow radii computation was broken, again in paintBoxSadow(), which causes
   pixel test failures with 0 tolerance. So I fixed it and now they passes.

Thanks in advance.

[Antonio Gomes]

Comment on attachment 79270 [details]
Patch

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

Nice work!

> Source/WebCore/platform/graphics/GraphicsContext.cpp:546
> +    path.addRoundedRect(rect.rect(), rect.radii().topLeft(), rect.radii().topRight(), rect.radii().bottomLeft(), rect.radii().bottomRight());

It would nice if Path::addRoundedRect get the "const RoundedIntRect&" as parameter, and extract the values from there.

Caller sites would look better, maybe.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:556
> +    path.addRoundedRect(rect.rect(), rect.radii().topLeft(), rect.radii().topRight(), rect.radii().bottomLeft(), rect.radii().bottomRight());

Would affect here too.

> Source/WebCore/platform/graphics/RoundedIntRect.h:87
> +    void move(const IntSize size) { m_rect.move(size); }

const IntSize&

[Hajime Morrita]

Simon, Antonio, thank you for taking the review!

> > Source/WebCore/platform/graphics/GraphicsContext.cpp:546
> > +    path.addRoundedRect(rect.rect(), rect.radii().topLeft(), rect.radii().topRight(), rect.radii().bottomLeft(), rect.radii().bottomRight());
> 
> It would nice if Path::addRoundedRect get the "const RoundedIntRect&" as parameter, and extract the values from there.
> 
> Caller sites would look better, maybe.
I noticed that Path has only float-based APIs. So I think we should have RoundedFloatRect
for this purpose. I filed Bug 52685 and will take this.

> > Source/WebCore/platform/graphics/RoundedIntRect.h:87
> > +    void move(const IntSize size) { m_rect.move(size); }
> 
> const IntSize&
Will fix before land.

[Hajime Morrita]

Committed r76083: <http://trac.webkit.org/changeset/76083>