Summary: | Refactoring: Extract RoundedIntRect class | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | buildbot, dglazkov, hyatt, mitz, ossy, rhodovan.u-szeged, simon.fraser, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||
Attachments: |
|
Description
Hajime Morrita
2010-12-27 18:20:33 PST
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> |