[webkit-reviews] review denied: [Bug 56493] Drag-scrolling overlay scrollbars thumb in overflow regions does not work : [Attachment 88941] Patch that uses temporary clip rects

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Beth Dakin
<bdakin at apple.com>'s request for review:
Bug 56493: Drag-scrolling overlay scrollbars thumb in overflow regions does not
work
https://bugs.webkit.org/show_bug.cgi?id=56493

Attachment 88941: Patch that uses temporary clip rects
https://bugs.webkit.org/attachment.cgi?id=88941&action=review

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

r- because I think this is still not quite right.

I wish we could have some layout tests for this.

> Source/WebCore/rendering/RenderLayer.cpp:2188
> +	   if (renderer()->isPositioned() || renderer()->isRelPositioned() ||
renderer()->hasClip())
> +	       rootLayer->setOverlayScrollbarsRequireTemporaryClipRects(true);

Why hasClip(), rather than hasOverflowClip()?

I don't really see the need to cache this value on the rootLayer (which, BTW,
is just the painting root, so it changes with transforms and compositing). So I
think it's wrong to stuff it on rootLayer.

>>> Source/WebCore/rendering/RenderLayer.cpp:2868
>>> +	 useTemporaryClipRects =
rootLayer->overlayScrollbarsRequireTemporaryClipRects();//ScrollbarTheme::nativ
eTheme()->usesOverlayScrollbars();
>> 
>> One space before end of line comments  [whitespace/comments] [5]
> 
> Should have a space between // and comment  [whitespace/comments] [4]

This needs to be an |=, otherwise you're clobbering the
compositor()->inCompositingMode() setting.

> Source/WebCore/rendering/RenderLayer.cpp:3205
> +void RenderLayer::updateClipRects(const RenderLayer* rootLayer,
OverlayScrollbarSizeRelevancy relevancy)

Maybe just call the enum ScrollBarSizeIncluded?