Bug 51664 - Refactoring: Extract RoundedIntRect class
Summary: Refactoring: Extract RoundedIntRect class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-27 18:20 PST by Hajime Morrita
Modified: 2011-01-18 17:11 PST (History)
8 users (show)

See Also:


Attachments
Patch (94.04 KB, patch)
2010-12-27 18:46 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
try to fix chromium buid (95.41 KB, patch)
2010-12-27 20:01 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
updates to ToT (95.55 KB, patch)
2011-01-04 19:30 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (96.23 KB, patch)
2011-01-11 03:32 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (108.30 KB, patch)
2011-01-14 02:27 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
trying to fix the build break (108.36 KB, patch)
2011-01-16 22:46 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (114.02 KB, patch)
2011-01-18 05:48 PST, Hajime Morrita
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2010-12-27 18:20:33 PST
a quad of IntSize is often used to represent the radii of a rect. 
Extracting it to the class will simplify the code.
Comment 1 Hajime Morrita 2010-12-27 18:46:59 PST
Created attachment 77532 [details]
Patch
Comment 2 WebKit Review Bot 2010-12-27 19:04:14 PST
Attachment 77532 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7211223
Comment 3 Hajime Morrita 2010-12-27 20:01:29 PST
Created attachment 77534 [details]
try to fix chromium buid
Comment 4 Hajime Morrita 2011-01-04 19:30:21 PST
Created attachment 77962 [details]
updates to ToT
Comment 5 Hajime Morrita 2011-01-11 03:32:01 PST
Created attachment 78511 [details]
Patch
Comment 6 Hajime Morrita 2011-01-11 03:32:55 PST
Hi Simon, could you take a look?
I'd like to land this before other box-shadow fixes.
Comment 7 Simon Fraser (smfr) 2011-01-11 09:21:31 PST
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.
Comment 8 Hajime Morrita 2011-01-14 02:27:18 PST
Created attachment 78918 [details]
Patch
Comment 9 Hajime Morrita 2011-01-14 02:32:16 PST
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.
Comment 10 WebKit Review Bot 2011-01-14 02:44:57 PST
Attachment 78918 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7593027
Comment 11 Build Bot 2011-01-14 03:01:32 PST
Attachment 78918 [details] did not build on win:
Build output: http://queues.webkit.org/results/7523040
Comment 12 Hajime Morrita 2011-01-16 22:46:36 PST
Created attachment 79127 [details]
trying to fix the build break
Comment 13 Hajime Morrita 2011-01-17 03:30:51 PST
Simon, if you have time, would you be able to take a look again?
Comment 14 Simon Fraser (smfr) 2011-01-17 10:19:43 PST
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.
Comment 15 Hajime Morrita 2011-01-18 00:59:25 PST
Committed r76004: <http://trac.webkit.org/changeset/76004>
Comment 16 Csaba Osztrogonác 2011-01-18 01:25:20 PST
(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
Comment 17 Hajime Morrita 2011-01-18 04:27:07 PST
Committed r76016: <http://trac.webkit.org/changeset/76016>
Comment 18 Hajime Morrita 2011-01-18 04:28:44 PST
(In reply to comment #17)
> Committed r76016: <http://trac.webkit.org/changeset/76016>
This commit rolled the change.
Comment 19 Hajime Morrita 2011-01-18 04:47:40 PST
Rolled at http://trac.webkit.org/changeset/76016 due to pixel test failure
Comment 20 Hajime Morrita 2011-01-18 05:48:57 PST
Created attachment 79270 [details]
Patch
Comment 21 Hajime Morrita 2011-01-18 05:56:47 PST
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.
Comment 22 Antonio Gomes 2011-01-18 08:39:41 PST
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&
Comment 23 Hajime Morrita 2011-01-18 16:57:55 PST
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.
Comment 24 Hajime Morrita 2011-01-18 17:11:19 PST
Committed r76083: <http://trac.webkit.org/changeset/76083>