a quad of IntSize is often used to represent the radii of a rect. Extracting it to the class will simplify the code.
Created attachment 77532 [details] Patch
Attachment 77532 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7211223
Created attachment 77534 [details] try to fix chromium buid
Created attachment 77962 [details] updates to ToT
Created attachment 78511 [details] Patch
Hi Simon, could you take a look? I'd like to land this before other box-shadow fixes.
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.
Created attachment 78918 [details] Patch
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.
Attachment 78918 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7593027
Attachment 78918 [details] did not build on win: Build output: http://queues.webkit.org/results/7523040
Created attachment 79127 [details] trying to fix the build break
Simon, if you have time, would you be able to take a look again?
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.
Committed r76004: <http://trac.webkit.org/changeset/76004>
(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
Committed r76016: <http://trac.webkit.org/changeset/76016>
(In reply to comment #17) > Committed r76016: <http://trac.webkit.org/changeset/76016> This commit rolled the change.
Rolled at http://trac.webkit.org/changeset/76016 due to pixel test failure
Created attachment 79270 [details] Patch
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 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&
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.
Committed r76083: <http://trac.webkit.org/changeset/76083>