[webkit-reviews] review denied: [Bug 129301] [CSS Regions] Scrollable regions : [Attachment 225465] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Radu Stavila <stavila at adobe.com>'s
request for review:
Bug 129301: [CSS Regions] Scrollable regions
https://bugs.webkit.org/show_bug.cgi?id=129301

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225465&action=review


r-

> Source/WebCore/rendering/RenderBlock.cpp:2343
> -    if (hasOverflowClip())
> -	   scrolledOffset.move(-scrolledContentOffset());
> +    if (scrollParent->hasOverflowClip())
> +	   scrolledOffset.move(-scrollParent->scrolledContentOffset());

A better way to do this would be to add a virtual function that takes the
paintoffset and returns the scrolledOffset. Then you could override that
virtual function in RenderNamedFlowThread to do the correct adjustment. That
avoids putting isRenderNamedFlowThread queries right into RenderBlock.

Is patching only painting (and not repainting invalidation) really sufficient?
It looks suspicious that painting is being patched but not repainting.

> Source/WebCore/rendering/RenderLayer.cpp:5481
> +	   if (clipRectsContext.region->isRenderNamedFlowFragment()) {
> +	       RenderBlockFlow& fragmentContainer =
toRenderNamedFlowFragment(clipRectsContext.region)->fragmentContainer();
> +	       if (fragmentContainer.hasOverflowClip()) {
> +		   IntSize scrolledContentOffset =
fragmentContainer.scrolledContentOffset();
> +		  
layerBoundsWithVisualOverflow.inflateX(scrolledContentOffset.width());
> +		  
layerBoundsWithVisualOverflow.inflateY(scrolledContentOffset.height());
> +	       }
> +	   }

Not really happy with this special case sitting right in the calculateRects
code. Maybe it could be moved to a helper or something.

> Source/WebCore/rendering/RenderNamedFlowFragment.cpp:76
> +    // Regions do not inherit the overflow:scroll style,
> +    // scrolling is performed by the region's container.
> +    if (parentStyle.overflowX() != OSCROLL)
> +	   style.get().setOverflowX(parentStyle.overflowX());
> +    else
> +	   style.get().setOverflowX(OVISIBLE);
> +    
> +    if (parentStyle.overflowY() != OSCROLL)
> +	   style.get().setOverflowY(parentStyle.overflowY());
> +    else
> +	   style.get().setOverflowY(OVISIBLE);

This doesn't seem right. What about overflow:auto? We have helper methods for
this, i.e., scrollsOverflow that you should be using.

> Source/WebCore/rendering/RenderNamedFlowFragment.cpp:267
> +bool RenderNamedFlowFragment::shouldClipFlowThreadContent() const
> +{
> +    if (RenderRegion::shouldClipFlowThreadContent())
> +	   return true;
> +    
> +    return isLastRegion() && (style().regionFragment() ==
BreakRegionFragment || fragmentContainer().style().overflowY() == OSCROLL);
> +}

Checking the style directly once you have a renderer isn't the way you should
be doing this. Overflow isn't always applied from style. You should use the
scrollsOverflow renderer methods.

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:307
> +    // Take the scrolled offset of the region into consideration.
> +    RenderBlockFlow& fragmentContainer = fragment.fragmentContainer();
> +    if (fragmentContainer.hasOverflowClip()) {
> +	   IntSize scrolledContentOffset =
fragmentContainer.scrolledContentOffset();
> +	   visualOverflowRect.inflateX(scrolledContentOffset.width());
> +	   visualOverflowRect.inflateY(scrolledContentOffset.height());
> +    }

This placement looks suspicious to me with flipped block writing modes.