Bug 170988 - Do not paint the border of the box if the dirty region does not intersect with border area
Summary: Do not paint the border of the box if the dirty region does not intersect wit...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gwang Yoon Hwang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-19 05:36 PDT by Gwang Yoon Hwang
Modified: 2017-09-08 10:31 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.12 KB, patch)
2017-04-19 05:37 PDT, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff
Patch (7.59 KB, patch)
2017-04-20 05:57 PDT, Gwang Yoon Hwang
simon.fraser: review+
Details | Formatted Diff | Diff
Patch (7.60 KB, patch)
2017-04-21 09:16 PDT, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gwang Yoon Hwang 2017-04-19 05:36:38 PDT
Do not paint the border of the box if the dirty region does not intersect with border area
Comment 1 Gwang Yoon Hwang 2017-04-19 05:37:32 PDT
Created attachment 307477 [details]
Patch
Comment 2 Carlos Garcia Campos 2017-04-19 05:46:55 PDT
Sorry, I thought this was cairo specific.
Comment 3 Simon Fraser (smfr) 2017-04-19 15:37:43 PDT
Comment on attachment 307477 [details]
Patch

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

Looks OK but I didn't yet check all the math.

> Source/WebCore/platform/graphics/RoundedRect.cpp:251
> +static bool isCircleContainsPoint(const FloatPoint& point, const FloatPoint& center, const FloatSize& radii)
> +{
> +    FloatPoint transformedPoint(point);
> +    transformedPoint.move(-center.x(), -center.y());
> +    transformedPoint.scale(radii.height(), radii.width());
> +    float radius = radii.width() * radii.height();
> +
> +    if (transformedPoint.x() > radius || transformedPoint.y() > radius)
> +        return false;
> +    if (transformedPoint.x() + transformedPoint.y() <= radius)
> +        return true;
> +    return (transformedPoint.lengthSquared() <= radius * radius);
> +}

Please put this in GeometryUtilities.cpp (and fix the header to use #pragma once at the same time).

> Source/WebCore/platform/graphics/RoundedRect.cpp:260
> +        FloatPoint center(m_rect.x() + topLeft.width(), m_rect.y() + topLeft.height());

We usually do this now like: 

FloatPoint center = { m_rect.x() + topLeft.width(), m_rect.y() + topLeft.height() };
Comment 4 Gwang Yoon Hwang 2017-04-20 05:57:37 PDT
Created attachment 307590 [details]
Patch
Comment 5 Gwang Yoon Hwang 2017-04-20 06:02:03 PDT
(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 307477 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307477&action=review
> 
> Looks OK but I didn't yet check all the math.
> 
The math stuff :)

> > Source/WebCore/platform/graphics/RoundedRect.cpp:251
> > +static bool isCircleContainsPoint(const FloatPoint& point, const FloatPoint& 
> 
> Please put this in GeometryUtilities.cpp (and fix the header to use #pragma
> once at the same time).

I moved it to the GeometryUtilities and renamed it to ellipseContainsPoint
Also I modernized the header.
 
> 
> > Source/WebCore/platform/graphics/RoundedRect.cpp:260
> > +        FloatPoint center(m_rect.x() + topLeft.width(), m_rect.y() + topLeft.height());
> 
> We usually do this now like: 
> 
> FloatPoint center = { m_rect.x() + topLeft.width(), m_rect.y() +
> topLeft.height() };

Thanks, fixed. I removed unneeded FloatSize declarations from here, too.
Comment 6 Simon Fraser (smfr) 2017-04-21 03:50:33 PDT
Comment on attachment 307590 [details]
Patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1737
> +    // If no borders intersects with the dirty area, we can skip to paint borders.

"skip the border painting"
Comment 7 Gwang Yoon Hwang 2017-04-21 09:16:02 PDT
Created attachment 307736 [details]
Patch
Comment 8 Gwang Yoon Hwang 2017-04-21 09:27:14 PDT
(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 307590 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307590&action=review
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1737
> > +    // If no borders intersects with the dirty area, we can skip to paint borders.
> 
> "skip the border painting"

Thanks for the correction :)
Comment 9 WebKit Commit Bot 2017-04-21 09:58:53 PDT
Comment on attachment 307736 [details]
Patch

Clearing flags on attachment: 307736

Committed r215613: <http://trac.webkit.org/changeset/215613>
Comment 10 Gwang Yoon Hwang 2017-05-02 07:26:32 PDT
This patch has been landed. Closing
Comment 11 Simon Fraser (smfr) 2017-09-08 08:06:18 PDT
This caused bug 176498.
Comment 12 Michael Catanzaro 2017-09-08 10:31:54 PDT
Simon, I think this commit was just an optimization that could be rolled out, but Yoon doesn't work for us anymore so I can't ask him. It did function as a workaround for bug #126124, though, so please reopen that bug if you roll this out.