[webkit-reviews] review denied: [Bug 109560] Implement coordinated scrollbar for subframes and sublayers : [Attachment 187822] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Tien-Ren Chen
<trchen at chromium.org>'s request for review:
Bug 109560: Implement coordinated scrollbar for subframes and sublayers
https://bugs.webkit.org/show_bug.cgi?id=109560

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

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


r- for abuse of setContentsToMedia.

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:125
> +    scrollbarGraphicsLayer->setContentsToMedia(scrollbarLayer->layer());

This seems like an abuse of setContentsToMedia.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1015
>      if (needsHorizontalScrollbarLayer) {
>	   if (!m_layerForHorizontalScrollbar) {
>	       m_layerForHorizontalScrollbar = createGraphicsLayer("horizontal
scrollbar");
>	       layersChanged = true;
> +	       if (scrollingCoordinator)
> +		  
scrollingCoordinator->scrollableAreaScrollbarLayerDidChange(m_owningLayer,
HorizontalScrollbar);
>	   }
>      } else if (m_layerForHorizontalScrollbar) {
>	   m_layerForHorizontalScrollbar.clear();
>	   layersChanged = true;
> +	   if (scrollingCoordinator)
> +	      
scrollingCoordinator->scrollableAreaScrollbarLayerDidChange(m_owningLayer,
HorizontalScrollbar);

I'd prefer that you change the code so that you only need to call
scrollableAreaScrollbarLayerDidChange in one or two places in this function,
not 4 times.