[webkit-reviews] review denied: [Bug 107618] Fix painting phases for composited scrolling : [Attachment 188625] Patch

Mon Mar 4 11:15:18 PST 2013

Simon Fraser (smfr) <simon.fraser at apple.com> has denied vollick at chromium.org's
request for review:
Bug 107618: Fix painting phases for composited scrolling
https://bugs.webkit.org/show_bug.cgi?id=107618

Attachment 188625: Patch
https://bugs.webkit.org/attachment.cgi?id=188625&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=188625&action=review

> Source/WebCore/rendering/RenderLayer.cpp:3770
> -	   // Now walk the sorted list of children with negative z-indices.
> -	   paintList(negZOrderList(), context, localPaintingInfo,
localPaintFlags);
> +	   if (!usesCompositedScrolling()) {
> +	       // Now walk the sorted list of children with negative z-indices.

> +	       paintList(negZOrderList(), context, localPaintingInfo,
localPaintFlags);
> +	   }
> +
> +	   // When using composited scrolling, outlines paint into the
background.
> +	   if (usesCompositedScrolling() && shouldPaintOutline &&
!outlineRect.isEmpty()) {
> +	       // Paint our own outline
> +	       PaintInfo paintInfo(context,
pixelSnappedIntRect(outlineRect.rect()), PaintPhaseSelfOutline, paintBehavior,
paintingRootForRenderer, localPaintingInfo.region);
> +	       clipToRect(localPaintingInfo.rootLayer, context,
localPaintingInfo.paintDirtyRect, outlineRect,
DoNotIncludeSelfForBorderRadius);
> +	       renderer()->paint(paintInfo, paintOffset);
> +	       restoreClip(context, localPaintingInfo.paintDirtyRect,
outlineRect);
> +	   }
>      }
>      
>      if (localPaintFlags & PaintLayerPaintingCompositingForegroundPhase) {
> +	   if (usesCompositedScrolling()) {
> +	       // Now walk the sorted list of children with negative z-indices.

> +	       paintList(negZOrderList(), context, localPaintingInfo,
localPaintFlags);
> +	   }

Seems wrong to do it like this. RenderLayer shouldn't really care about what
compositing layers exist; the behavior should be expressible via paint phases.

> LayoutTests/ChangeLog:32
> +	   *
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.ht
ml: Added.
> +	   *
compositing/overflow/paint-neg-z-order-descendants-into-scrolling-contents-laye
r.html: Added.
> +	   * platform/chromium/TestExpectations:
> +	   * platform/mac-wk2/TestExpectations:

I'd like to see a test with layerTeeAsText output here.

>
LayoutTests/compositing/overflow/do-not-paint-outline-into-composited-scrolling
-contents.html:3
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
> +   "http://www.w3.org/TR/html4/loose.dtd">
> +<html lang="en">

Please use an HTML5-style DOCTYPE.