Bug 98660 – RenderBoxModelObject::paintTranslucentBorderSides() creates needless transparency layers

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

[Tom Hudson]

The code that computes useTransparencyLayer doesn't pay attention to edges[i].shouldRender(), but paintBorderSides() does.
So we can call beginTransparencyLayer() and endTransparencyLayer() - which can cost tens of ms on some platforms if the layers are large - but render nothing to the layer we so carefully create and painstakingly blend.

[Tom Hudson]

Created attachment 167731 [details]
Simplified version of patch for testing

[Tom Hudson]

Created attachment 167734 [details]
Fix pathnames.

[Tom Hudson]

Created attachment 167739 [details]
Add Changelog.

[WebKit Review Bot]

Attachment 167739 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tom Hudson]

Created attachment 167741 [details]
Fix ChangeLog formatting.

[Tom Hudson]

Created attachment 167761 [details]
Alternate implementation in paintBorder() instead of paintTranslucentBorderSides()

Early-out higher up the callstack, so we do even less unnecessary work.

[Tom Hudson]

Created attachment 167776 [details]
After out-of-band review, moving the implementation back to paintTranslucentBorderSides(), but reversing the flag-application logic for reasability

[Simon Fraser (smfr)]

Comment on attachment 167776 [details]
After out-of-band review, moving the implementation back to paintTranslucentBorderSides(), but reversing the flag-application logic for reasability

I think it would be slightly better for RenderBoxModelObject::paintBorder() to compute edgesToDraw in its existing loop through the edges. edgesToDraw could then be passed to paintTranslucentBorderSides() and paintBorderSides().

[Tom Hudson]

Created attachment 174464 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 174464 [details]
Patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1963
> +    BorderEdgeFlags edgesToDraw = 0;
> +    for (int i = BSTop; i <= BSLeft; ++i)
> +        if (edges[i].shouldRender())
> +            edgesToDraw |= edgeFlagForSide(static_cast<BoxSide>(i));

I don't like changing the value of input parameters. Why is this necessary here?

[Tom Hudson]

(In reply to comment #10)
> (From update of attachment 174464 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174464&action=review
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1963
> > +    BorderEdgeFlags edgesToDraw = 0;
> > +    for (int i = BSTop; i <= BSLeft; ++i)
> > +        if (edges[i].shouldRender())
> > +            edgesToDraw |= edgeFlagForSide(static_cast<BoxSide>(i));
> 
> I don't like changing the value of input parameters. Why is this necessary here?

Simon, I'm not sure I understand your comment.

edgesToDraw is not an input to paintBorder().
It was previously not an input to paintTransparentBorderSides(); you r-ed my previous the version in #8 and requested that I compute it in paintBorder() and pass it down.
We were previously always passing AllBorderEdges to paintBorderSides(); I thought in #8 you were asking me to pass down the more-precise version.

[Simon Fraser (smfr)]

Sorry the review diff page confused me. This seems OK, but please upload a patch that applies cleanly.

[Tom Hudson]

Created attachment 174948 [details]
Patch

[Tom Hudson]

Comment on attachment 174948 [details]
Patch

Simon, patch applies this time and seems to be doing well in EWS.

[WebKit Review Bot]

Comment on attachment 174948 [details]
Patch

Clearing flags on attachment: 174948

Committed r135167: <http://trac.webkit.org/changeset/135167>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.